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