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
