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

["Pali Rohár" <pali@kernel.org>]

On Saturday 22 June 2024 16:26:13 Hans de Goede wrote:
> 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 ...

Why?

Currently in i2c-i801.c file was called just
register_dell_lis3lv02d_i2c_device() function and there was no binding
to anything, no binding to DMI structure and neither no binding to ACPI
structures.

And if I'm looking correctly at your new function
smo8800_instantiate_i2c_client() it does not bind device neither.
And smo8800_i2c_bus_notify() does not depend on binding.

So I do not see where is that binding requirement.

> 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.
> > 
>