Re: [PATCH v3 3/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800

[Hans de Goede <hdegoede@redhat.com>]

Hi Pali,

On 6/22/24 6:43 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:
>> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
>>>> Also does the whole device table has to be such verbose with lot of
>>>> duplicated information (which probably also increase size of every linux
>>>> image which includes this driver into it)?
>>>
>>> struct dmi_system_id is the default way to specify DMI matches in
>>> the kernel. This avoids code duplication in the form of writing
>>> a DYI function to do the matching.
>>>
>>> In v2 of the patch-set I only matched on product-name, but you asked
>>> in the review of v2 to also match on sys-vendor and you mentioned
>>> we may want to support other sys-vendors too, since some other brands
>>> have SMO88xx ACPI devices too. This more or less automatically leads
>>> to using the kernel's standard, existing, DMI matching mechanism.
>>>
>>> We really want to avoid coming up with something "new" ourselves here
>>> leading to unnecessary code duplication.
>>>
>>> Regards,
>>>
>>> Hans
>>
>> Ok, then let that table as you have it now.
> 
> Definition of the table can be simplified by defining a macro which
> expand to those verbose parts which are being repeating, without need to
> introduce something "new". E.g.:
> 
> #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \
> 	{ \
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), \
> 			DMI_MATCH(DMI_PRODUCT_NAME, product_name), \
> 		}, \
> 		.driver_data = (void *)(i2c_addr), \
> 	}
> 
> static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> 	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250", 0x29),
> 	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450", 0x29),
> 	...
> 	{ }
> };
> 
> Any opinion about this?

Thank you that is a good idea. I'll do as you suggest for v4 with
the addition of Andy's suggestion to use const in the cast.

Regards,

Hans