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