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

Hans de Goede
2024-02-17T10:33:28Z

Hi Jean,

On 2/13/24 17:30, Jean Delvare wrote:
> Hi Pali, Hans,
>
> On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
>> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
>>> It is not necessary to handle the Dell specific instantiation of
>>> i2c_client-s for SMO8xxx 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 Dell specific dell-smo8800 driver.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> 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 | 126 +----------------------
>>> drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++-
>>> 2 files changed, 123 insertions(+), 124 deletions(-)
>>
>> I'm looking at this change again and I'm not not sure if it is a good
>> direction to do this movement. (...)
>
> Same feeling here. Having to lookup the parent i2c bus, which may or
> may not be present yet, doesn't feel good.
>
> I wouldn't object if everybody was happy with the move and moving the
> code was solving an actual issue, but that doesn't seem to be the case.

I thought you would actually like getting this somewhat clunky code
which basically works around the hw not being properly described in
the ACPI tables out of the generic i2c-i801 code.

I didn't get around to answer's Pali's concerns yet, so let me
start by addressing those since you indicate that you share Pali's
concerns:

Pali wrote:
> Now after looking at this change again I see there a problem. If i2c-801
> driver initialize i2c-801 device after this smo8800 is called then
> accelerometer i2c device would not happen.

That is a good point (which Jean also points out). But this can simply
be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER
if the i2c-i801 i2c-bus is not present yet (all designs using the
dell-smo8800 driver will have an i2c-bus so waiting for this to show
up should not cause regressions).

If we can agree to move forward this series I'll fix this.

Pali wrote:
> Also it has same problem if PCI i801 device is reloaded or reset.

The i801 device is not hotplugable, so normally this will never
happen. If the user manually unbinds + rebinds the i2c-i801 driver
them the i2c_client for the smo88xx device will indeed get removed
and not re-added. But this will normally never happen and if
a user is manually poking things then the user can also unbind +
rebind the dell-mso8800 driver after the i2c-i801 rebind.
So I don't really see this as an issue.

With those remarks addressed let me try to explain why I think
that moving this to the dell-smo8800 code is a good idea:

1. It is a SMO88xx ACPI device specific kludge and as such IMHO
thus belongs in the driver for the SMO88xx ACPI platform_device.

The i2c-i801 driver gets loaded on every x86 system and it is
undesirable to have this extra code and the DMI table in RAM
on all those other systems.

2. Further changes in this series, like adding support for
probing for the i2c address of the lis3lv02d device on models
not yet in the DMI table, will add a bunch of more code specific
to SMO88xx ACPI devices. Making the problem of having SMO88xx
specific code in the generic i2c-i801 driver even bigger.
The current amount of SMO88xx specific code in the
generic i2c-i801 driver might be considered acceptable but I'm
afraid that the amount of code after this series will not be
acceptable.

3. Some of the changes in this series are harder to implement inside
the i2c-i801 code, like optionally instantiating an i2c_client for
the IIO st_accel driver (*) so that the accelerometer gets presented
to userspace as a standard IIO device like all modern accelerometer
drivers do.

This requires setting i2c_client.irq and that IRQ comes from
the SMO88xx ACPI device. So this would require the i2c-i801 code
to lookup the ACPI device and get the IRQ from there. Where as
in the SMO88xx ACPI platform_device driver the IRQ is readily
available.

TL;DR: IMHO all this SMO88xx quirk/glue handling belongs in
the SMO88xx specific dell-smo8800 driver rather then in
the generic i2c-i801 code.

Regards,

Hans

*) Instead of an i2c_client for the somewhat weird (but still
default for backward compat) drivers/misc/lis3lv02d/lis3lv02d.c
driver