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 Andy,

On 6/21/24 5:24 PM, Andy Shevchenko wrote:
> On Fri, Jun 21, 2024 at 2:25 PM Hans de Goede <hdegoede@redhat.com> 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.
>>
>> 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.
> 
> ...
> 
> 
>> +static int smo8800_find_i801(struct device *dev, void *data)
>> +{
>> +       struct i2c_adapter *adap, **adap_ret = data;
>> +
>> +       adap = i2c_verify_adapter(dev);
>> +       if (!adap)
>> +               return 0;
>> +
>> +       if (!strstarts(adap->name, "SMBus I801 adapter"))
> 
> With the comment on the previous patch I'm wondering if it makes sense
> to have this to be as simple as strstr("I801") or strstr("I801 IDF")?

We want the non IDF one, strstr("I801") would match both and
strstr("I801 IDF") would match the one we don't want.

> 
>> +               return 0;
>> +
>> +       *adap_ret = i2c_get_adapter(adap->nr);
>> +       return 1;
>> +}
> 
> ...
> 
>> +       info.addr = (long)lis3lv02d_dmi_id->driver_data;
> 
> Hmm... Usually we use uintptr_t, but okay.
> 
> ...
> 
>> +               if (strstarts(adap->name, "SMBus I801 adapter"))
> 
> A dup? Is there a possibility it may go desynchronized?

That is a good point I'll add a small i2c_adapter_is_main_i801(adap)
helper for this for the next version.

Regards,

Hans