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 4:20 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
>> Hi Pali,
>>
>> On 6/22/24 3:16 PM, Pali Rohár wrote:
>>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
>>>> It is not necessary to handle the Dell specific instantiation of
>>>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
>>>> inside the generic i801 I2C adapter driver.
>>>>
>>>> The kernel already instantiates platform_device-s for these ACPI devices
>>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
>>>> platform drivers.
>>>>
>>>> Move the i2c_client instantiation from the generic i2c-i801 driver to
>>>> the SMO88xx specific dell-smo8800 driver.
>>>
>>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
>>> and freefall code for smo88xx are basically independent.
>>>
>>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
>>> detection. The only thing which have these "drivers" common is the ACPI
>>> detection mechanism based on presence of SMO88?? identifiers from
>>> acpi_smo8800_ids[] array.
>>>
>>> I think it makes both "drivers" cleaner if they are put into separate
>>> files as they are independent of each one.
>>>
>>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
>>> instead (or similar name)? And just share list of ACPI ids via some
>>> header file (or something like that).
>>
>> Interesting idea, but that will not work, only 1 driver can bind to
>> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
> 
> And it is required to bind lis3 device to ACPI code? What is needed is
> just to check if system matches DMI strings and ACPI strings. You are
> not binding device to DMI strings, so I think there is no need to bind
> it neither to ACPI strings.

The driver needs to bind to something ...

This is code for special handling required for SMO88xx ACPI devices,
dell-smo8800 is *the* driver for those ACPI devices. So this code clearly
belongs here.

According to diffstat this adds about 400 lines of code that is really not
that big, so I see no urgent reason to introduce weird tricks instead of
standard driver binding for this.

Regards,

Hans






> 
>>>> Moving the i2c_client instantiation here has the following advantages:
>>>>
>>>> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
>>>> i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
>>>> specific dell-smo8800 module where it belongs.
>>>>
>>>> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
>>>> between the i2c-i801 and dell-smo8800 drivers.
>>>>
>>>> 3. This allows extending the quirk handling by adding new code and related
>>>> module parameters to the dell-smo8800 driver, without needing to modify
>>>> the i2c-i801 code.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Note the goto out_put_adapter, which can be avoided by moving the DMI check
>>>> up, is there deliberately as preparation for adding support to probe for
>>>> the i2c address in case there is no DMI match.
>>>> ---
>>>> Changes in v3:
>>>> - Use an i2c bus notifier so that the i2c_client will still be instantiated if
>>>>   the i801 i2c_adapter shows up later or is re-probed (removed + added again)
>>>> - Switch to standard dmi_system_id matching to check both sys-vendor +
>>>>   product-name DMI fields
>>>> - Use unique i2c_adapter->name prefix for primary i2c_801 controller
>>>>   to avoid needing to duplicate PCI ids for extra IDF i2c_801 i2c_adapter-s
>>>> - Drop MODULE_SOFTDEP("pre: i2c-i801"), this is now no longer necessary
>>>> - Rebase on Torvalds master for recent additions of extra models in
>>>>   the dell_lis3lv02d_devices[] list
>>>>
>>>> Changes in v2:
>>>> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
>>>> - Add a comment documenting the IDF PCI device ids
>>>> ---
>>>>  drivers/i2c/busses/i2c-i801.c            | 124 -------------
>>>>  drivers/platform/x86/dell/dell-smo8800.c | 214 ++++++++++++++++++++++-
>>>>  2 files changed, 213 insertions(+), 125 deletions(-)
>>
>> <snip>
>>
>>>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
>>>> index f7ec17c56833..cd2e48405859 100644
>>>> --- a/drivers/platform/x86/dell/dell-smo8800.c
>>>> +++ b/drivers/platform/x86/dell/dell-smo8800.c
>>
>> ...
>>
>>>> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
>>>>  	.release = smo8800_misc_release,
>>>>  };
>>>>  
>>>> +/*
>>>> + * Accelerometer's I2C address is not specified in DMI nor ACPI,
>>>> + * so it is needed to define mapping table based on DMI product names.
>>>> + */
>>>> +static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
>>>> +	/*
>>>> +	 * Dell platform team told us that these Latitude devices have
>>>> +	 * ST microelectronics accelerometer at I2C address 0x29.
>>>> +	 */
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	/*
>>>> +	 * Additional individual entries were added after verification.
>>>> +	 */
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
>>>> +		},
>>>> +		.driver_data = (void *)0x1dL,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>
>>> At least for me, casting i2c address to LONG and then to pointer looks
>>> very strange. If I look at this code without knowing what the number
>>> 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
>>> address.
>>>
>>> Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
>>> instead of ".something = (void *)0x29L" to make it readable?
>>
>> struct dmi_system_id is an existing structure and we cannot just go adding
>> fields to it. driver_data is intended to tie driver specific data to
>> each DMI match, often pointing to some struct, so it is a void *, but
> 
> Yes, I know it.
> 
>> in this case we only need a single integer, so we store that in the
>> pointer. That is is the address becomes obvious when looking at the code
>> which consumes the data.
> 
> Ok, this makes sense. Anyway, is explicit void* cast and L suffix
> required?
> 
>>> 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.
>