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

[Andy Shevchenko <andy@kernel.org>]

On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote:
> On 2/13/24 17:30, Jean Delvare wrote:
> > 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:

FWIW, I agree with Hans on the location of the HW quirks.
If there is a possible way to make the actual driver cleaner
and collect quirks somewhere else, I'm full support for that.

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

-- 
With Best Regards,
Andy Shevchenko