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

Pali Rohár
2024-01-07T17:10:59Z

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. Some of the issues which this change
introduces I described in the previous email. I somehow have not caught
up why to do it.

ACPI smo8800 device and i2c lis3lv02d are from the OS resource point of
view totally different devices, not connected together in any way. In
ACPI tables there is no link information that smo8800 belongs to
lis3lv02d, and neither that it is on i2c. System tree of the devices
structures also handle it in this way.

If I'm looking at the current device design, it is a bus who instantiate
devices (as children of the bus). In this case, this i2c has no
information what is there connected (no Device Tree, no ACPI), so only
static data hardcoded in kernel are available. And therefore it should
be the bus who create or delete devices.

If the whole idea of this patch series was to merge smo8800 device and
lis3lv02d device into one device, the question is why to do it and why
it is a good idea in this case? (Specially when firmware provide to use
very little information). I just do not see this motivation. If it is
going to fix some bug or required for some new feature or something
else. I would like to know this one. Maybe I'm completely something
missing and hence I'm wrong...

I know that it is just a one device which provides interrupt and
accelerometer axes, but these two things are from OS persepctive totally
separated and there can be all combinations which of them are available
and which not (BIOS has select option to disable ACPI device=interrupt,
can be ON/OFF and kernel has or does not have DMI information of i2c bus
for acelerometer axes).

I can understand motivation to replace one i2c driver by another, if
there is a new style of it. In this it is just needed to teach new iio
lis driver to support some joystick emulation layer (can be generic, or
only for lis, or only available for HP and Dell machines) and switch can
be done without issue.

I can also understand motivation that freefall code is in two drivers
(old i2c lis driver and acpi smo8800). In this case it can be extracted
somwhere into helper function, or maybe completely moves into
platform/x86 as it is IIRC only for HP and Dell machines, and can ripped
out from the old i2c lis driver (unless there is some other usage for
it).

I also know that it is not a clean to have some Dell DMI data list in
the i801 bus driver and helper code not related to i801. So why not to
move this code from i2c-i801.c source file to some other helper library
and call just the helper function from i801.

I would rather let i2c lis device and ACPI smo8800 device separated,
this concept is less prone to errors, matches linux device model and is
already in use for many years and verified that works fine.