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

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

Hi Andy,

Thank you for the reviews.

On 8/5/24 10:48 PM, Andy Shevchenko wrote:
> On Mon, Aug 5, 2024 at 3:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Various Dell laptops have an lis3lv02d freefall/accelerometer sensor.
>> The lis3lv02d chip has an interrupt line as well as an I2C connection to
>> the system's main SMBus.
>>
>> The lis3lv02d is described in the ACPI tables by an SMO88xx ACPI device,
>> but the SMO88xx ACPI fwnodes are incomplete and only list an IRQ resource.
>>
>> So far this has been worked around with some SMO88xx specific quirk code
>> in the generic i2c-i801 driver, but it is not necessary to handle the Dell
>> specific instantiation of i2c_client-s for SMO88xx ACPI devices there.
>>
>> The kernel already instantiates platform_device-s for these with an
>> acpi:SMO88xx modalias. The drivers/platform/x86/dell/dell-smo8800.c
>> driver binds to this platform device but this only deals with
>> the interrupt resource. Add a drivers/platform/x86/dell/dell-lis3lv02d.c
>> which will matches on the same acpi:SMO88xx modaliases and move
>> the i2c_client instantiation from the generic i2c-i801 driver there.
>>
>> Moving the i2c_client instantiation 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 a module
>> which will only be loaded when there is an ACPI SMO88xx device.
>>
>> 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-lis3lv02d driver, without needing to modify
>> the i2c-i801 code.
> 
> ...
> 
>> +static void instantiate_i2c_client(struct work_struct *work)
>> +{
>> +       struct i2c_board_info info = { };
>> +       struct i2c_adapter *adap = NULL;
>> +
>> +       if (i2c_dev)
>> +               return;
>> +
>> +       bus_for_each_dev(&i2c_bus_type, NULL, &adap, find_i801);
>> +       if (!adap)
>> +               return;
> 
> May i2c_for_each_dev() be used here?

The main difference between i2c_for_each_dev() and
bus_for_each_dev() is that i2c_for_each_dev() holds
the i2c core mutex while it is calling the passed
in callback.

And find_i801() calls i2c_get_adapter() which also
takes the i2c core mutex, so i2c_for_each_dev()
cannot be used here since then things deadlock.

> 
>> +       info.addr = i2c_addr;
>> +       strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>> +
>> +       i2c_dev = i2c_new_client_device(adap, &info);
>> +       if (IS_ERR(i2c_dev)) {
>> +               pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev));
>> +               i2c_dev = NULL;
>> +       } else {
>> +               pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr);
>> +       }
>> +
>> +       i2c_put_adapter(adap);
>> +}
> 
> ...
> 
>> +static int __init match_acpi_device_ids(struct device *dev, const void *data)
>> +{
>> +       const struct acpi_device_id *ids = data;
> 
> Wondering if this is needed. Can the compiler implicitly cast const
> void * to the const something * ?

I have just tried this and the compiler is happy with the ids variable
dropped, so I'll use that for v8.

Regards,

Hans