| Message ID | 20240106160935.45487-1-hdegoede@redhat.com |
|---|---|
| Headers | show |
| Series | i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand |
On Saturday 06 January 2024 17:09:29 Hans de Goede wrote: > +/* > + * Accelerometer's I2C address is not specified in DMI nor ACPI, > + * so it is needed to define mapping table based on DMI product names. > + */ > +static const struct { > + const char *dmi_product_name; > + unsigned short i2c_addr; > +} dell_lis3lv02d_devices[] = { > + /* > + * Dell platform team told us that these Latitude devices have > + * ST microelectronics accelerometer at I2C address 0x29. > + */ > + { "Latitude E5250", 0x29 }, > + { "Latitude E5450", 0x29 }, > + { "Latitude E5550", 0x29 }, > + { "Latitude E6440", 0x29 }, > + { "Latitude E6440 ATG", 0x29 }, > + { "Latitude E6540", 0x29 }, > + /* > + * Additional individual entries were added after verification. > + */ > + { "Latitude 5480", 0x29 }, > + { "Vostro V131", 0x1d }, > + { "Vostro 5568", 0x29 }, > +}; > + > +static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800) > +{ > + struct i2c_board_info info = { }; > + struct i2c_adapter *adap = NULL; > + const char *dmi_product_name; > + u8 addr = 0; > + int i; > + > + bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801); > + if (!adap) > + return; > + > + dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); > + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { Before doing this array iteration it is needed to check for Dell vendor like it was before: if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) return false; Or put vendor name into the devices list and check for it (in case you want to extend list also for non-Dell machines). > + if (strcmp(dmi_product_name, > + dell_lis3lv02d_devices[i].dmi_product_name) == 0) { > + addr = dell_lis3lv02d_devices[i].i2c_addr; > + break; > + } > + } > + > + if (!addr) { > + dev_warn(smo8800->dev, > + "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); > + goto put_adapter; > + } > + > + info.addr = addr; > + strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); > + > + smo8800->i2c_dev = i2c_new_client_device(adap, &info); > + if (IS_ERR(smo8800->i2c_dev)) { > + dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev), > + "registering accel i2c_client\n"); > + smo8800->i2c_dev = NULL; > + } else { > + dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n", > + info.type, info.addr); > + } > +put_adapter: > + i2c_put_adapter(adap); > +} > + > static int smo8800_probe(struct platform_device *device) > { > int err; > @@ -126,10 +237,12 @@ static int smo8800_probe(struct platform_device *device) > return err; > smo8800->irq = err; > > + smo8800_instantiate_i2c_client(smo8800); 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. Also it has same problem if PCI i801 device is reloaded or reset. With the current approach this was not an issue as during i801 initialization was smo i2c device automatically created and lis driver was able to bind and initialize it at any time. Before parent driver created its own direct children devices. After this change, the child driver is trying to find who is the parent of its device and injects its device to the parent in the device tree hierarchy. > + > err = misc_register(&smo8800->miscdev); > if (err) { > dev_err(&device->dev, "failed to register misc dev: %d\n", err); > - return err; > + goto error_unregister_i2c_client; > } > > err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick, > @@ -150,6 +263,8 @@ static int smo8800_probe(struct platform_device *device) > > error: > misc_deregister(&smo8800->miscdev); > +error_unregister_i2c_client: > + i2c_unregister_device(smo8800->i2c_dev); > return err; > } > > @@ -160,9 +275,9 @@ static void smo8800_remove(struct platform_device *device) > free_irq(smo8800->irq, smo8800); > misc_deregister(&smo8800->miscdev); > dev_dbg(&device->dev, "device /dev/freefall unregistered\n"); > + i2c_unregister_device(smo8800->i2c_dev); > } > > -/* NOTE: Keep this list in sync with drivers/i2c/busses/i2c-i801.c */ > static const struct acpi_device_id smo8800_ids[] = { > { "SMO8800", 0 }, > { "SMO8801", 0 }, > @@ -189,3 +304,5 @@ module_platform_driver(smo8800_driver); > MODULE_DESCRIPTION("Dell Latitude freefall driver (ACPI SMO88XX)"); > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Sonal Santan, Pali Rohár"); > +/* Ensure the i2c-801 driver is loaded for i2c_client instantiation */ > +MODULE_SOFTDEP("pre: i2c-i801"); > -- > 2.43.0 >
On Sat, 6 Jan 2024 18:24:34 +0200 Andy Shevchenko <andy@kernel.org> wrote: > > + if (!strstarts(adap->name, "SMBus I801 adapter")) > > + return 0; > > Bah, we have str_has_prefix() and this, much older one... > Steven? Others? I mean we can do something about this duplication, right? They are not really duplicate functions. Note that strstarts() is just a boolean (does this start with something) where as str_has_prefix() returns the length of the prefix. Yes, strstarts() can be swapped to str_has_prefix() but it can't go the other way around. One use case of the str_has_prefix() feature is in the histogram parsing: for (i = 0; i < hist_data->attrs->n_actions; i++) { str = hist_data->attrs->action_str[i]; if ((len = str_has_prefix(str, "onmatch("))) { char *action_str = str + len; data = onmatch_parse(tr, action_str); if (IS_ERR(data)) { ret = PTR_ERR(data); break; } } else if ((len = str_has_prefix(str, "onmax("))) { char *action_str = str + len; data = track_data_parse(hist_data, action_str, HANDLER_ONMAX); if (IS_ERR(data)) { ret = PTR_ERR(data); break; } } else if ((len = str_has_prefix(str, "onchange("))) { char *action_str = str + len; data = track_data_parse(hist_data, action_str, HANDLER_ONCHANGE); if (IS_ERR(data)) { ret = PTR_ERR(data); break; } Where we get the length of the prefix if there's a match, and use that to skip over the prefix. If you just need to know if something starts with a string, then "strstarts()" is perfectly fine to use. -- Steve
On Sun, 7 Jan 2024 11:09:17 -0500 Steven Rostedt <rostedt@goodmis.org> wrote: > for (i = 0; i < hist_data->attrs->n_actions; i++) { > str = hist_data->attrs->action_str[i]; > > if ((len = str_has_prefix(str, "onmatch("))) { > char *action_str = str + len; > > data = onmatch_parse(tr, action_str); > if (IS_ERR(data)) { > ret = PTR_ERR(data); > break; > } > } else if ((len = str_has_prefix(str, "onmax("))) { > char *action_str = str + len; > > data = track_data_parse(hist_data, action_str, > HANDLER_ONMAX); > if (IS_ERR(data)) { > ret = PTR_ERR(data); > break; > } > } else if ((len = str_has_prefix(str, "onchange("))) { > char *action_str = str + len; > > data = track_data_parse(hist_data, action_str, > HANDLER_ONCHANGE); > if (IS_ERR(data)) { > ret = PTR_ERR(data); > break; > } And this could possibly be consolidated to: for (i = 0; i < hist_data->attrs->n_actions; i++) { char *action_str; enum handler_id hid; ret = -EINVAL; str = hist_data->attrs->action_str[i]; if ((len = str_has_prefix(str, "onmatch("))) hid = HANDLER_ONMATCH; else if ((len = str_has_prefix(str, "onmax("))) hid = HANDLER_ONMAX; else if ((len = str_has_prefix(str, "onchange("))) hid = HANDLER_ONCHANGE; else break; action_str = str + len; switch (hid) { case HANDLER_ONMATCH: data = onmatch_parse(tr, action_str); break; default: data = track_data_parse(hist_data, action_str, hid); } if (IS_ERR(data)) { ret = PTR_ERR(data); break; } hist_data->actions[hist_data->n_actions++] = data; ret = 0; } I think I'll go make that change! -- Steve
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.
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@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. (...) 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.
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@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. (...) > > 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
On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote: > 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. Location of the quirks can be moved outside of the i2c-i801.c source code relatively easily without need to change the way how parent--child relationship currently works. Relevant functions is_dell_system_with_lis3lv02d() and register_dell_lis3lv02d_i2c_device() does not use internals of i2c-i801 and could be moved into new file, lets say drivers/platform/x86/dell/dell-smo8800-plat.c Put this file under a new hidden "bool" config option which is auto enabled when CONFIG_DELL_SMO8800 is used. i2c-i801.c currently has code: if (is_dell_system_with_lis3lv02d()) register_dell_lis3lv02d_i2c_device(priv); This can be put into a new exported function, e.g. void dell_smo8800_scan_i2c(struct i2c_adapter *adapter); And i2c-i801.c would call it instead. register_dell_lis3lv02d_i2c_device just needs "adapter", it does not need whole i801 priv struct. With this simple change all dell smo8800 code would be in its subdir drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. This approach does not change any functionality, so should be absolutely safe. Future changes will be done only in drivers/platform/x86/dell/ subdir, touching i801 would not be needed at all.
On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: > On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote: > > 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. > > Location of the quirks can be moved outside of the i2c-i801.c source > code relatively easily without need to change the way how parent--child > relationship currently works. > > Relevant functions is_dell_system_with_lis3lv02d() and > register_dell_lis3lv02d_i2c_device() does not use internals of > i2c-i801 and could be moved into new file, lets say > drivers/platform/x86/dell/dell-smo8800-plat.c > Put this file under a new hidden "bool" config option which is auto > enabled when CONFIG_DELL_SMO8800 is used. > > i2c-i801.c currently has code: > > if (is_dell_system_with_lis3lv02d()) > register_dell_lis3lv02d_i2c_device(priv); > > This can be put into a new exported function, e.g. > void dell_smo8800_scan_i2c(struct i2c_adapter *adapter); > And i2c-i801.c would call it instead. > > register_dell_lis3lv02d_i2c_device just needs "adapter", it does not > need whole i801 priv struct. I'm wondering why we need all this. We have notifiers when a device is added / removed. We can provide a board_info for the device and attach it to the proper adapter, no? > With this simple change all dell smo8800 code would be in its subdir > drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. > > This approach does not change any functionality, so should be absolutely > safe. > > Future changes will be done only in drivers/platform/x86/dell/ subdir, > touching i801 would not be needed at all. Still these exported functions are not the best solution we can do, right? We should be able to decouple them without need for the custom APIs.
On Saturday 17 February 2024 11:33:21 Hans de Goede wrote: > 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@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. (...) > > > > 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). Adding EPROBE_DEFER just complicates the dependency and state model. I would really suggest to come up with a simpler solution, not too complicated where it is required to think a lot if is is correct and if all edge-cases are handled. > 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. Well, rmmod & modprobe is not the rare cases. Whatever developers say about rmmod (or modprobe -r or whatever is the way for unloading modules), this is something which is used by a lot of users and would be used. > 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. I'm not sure if it belongs to "SMO88xx ACPI platform_device" but for sure I agree with you that it does not belong to i801 code. I would say that it belongs to some SMO8800 glue code -- because it is not the classic ACPI driver too. But I'm not against to have SMO glue code and SMO ACPI driver in one file (maybe it is even better to have it). > 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. I think we can take an assumption that ACPI SMO device does not change it existence or ACPI enabled/disabled state during runtime. So we can scan for ACPI SMO device just once in function stored in __init section called during the kernel/module initialization and cache the result (bool if device was found + its i2c address). After function marked as __init finish its job then together with DMI tables can be discarded from RAM. With this way it does take extra memory on every x86 system. Also we can combine this with an SMO config option, so the whole code "glue" code would not be compiled when SMO driver is not enabled via Kconfig. > 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. I think alternative approach which I described in the other email in this thread could be useful for this issue too (to move SMO code from i2c-i801.c source file). Together with above __init section approach it can also decrease memory usage. > 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 is something about which I'm not very convinced. IIO st_accel driver does not support freefall interface (or any other for signalling hard disk falls, which is used by userspace) and in dell systems, this hard disk protection is the primary usage of the accelerometer. In last two months I talked with two people, users of the accelerometers axis on dell and thinkpad machines. They were using it in games which were joystick-based (one game was tuxracer, second I do not remember name). So I'm not sure that replacing joystick driver by some new API would be really useful for users of accelerometer axis. Before such change I would propose to teach IIO st_accel driver (or what would be the replacement) to support joystick API for userspace. > 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. I understand this problem. But I would like to ask a question: WHY it is needed at all? The IRQ represents the free fall / hard disk fall event, which is slightly different thing than reporting accelerometer axis. Why IIO st_accel (or lis3lv02d) needs free fall IRQ? Hard disk fall interrupt on Dell machines can be handled by separate driver, not related to ACPI SMO8800 device. It would be much more easier to split these two different functionalities (reporting axes; and reporting hard disk fall event) into two separate drivers. And it would simplify whole logic related to instantiating free fall hard disk driver and accelerometer axes driver (either IIO st_accel or lis3lv02d or some other...). So from my side, I do not see a reason to "inject" IRQ number into driver which reads accelerometer axes. > 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. I agree, that it does not belong to the i2c-i801.c source file. And I also would like to see movement. That is why I proposed alternative and simpler solution. > Regards, > > Hans > > > *) Instead of an i2c_client for the somewhat weird (but still > default for backward compat) drivers/misc/lis3lv02d/lis3lv02d.c > driver > > > > > > >
On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote: > On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: > > On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote: > > > 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. > > > > Location of the quirks can be moved outside of the i2c-i801.c source > > code relatively easily without need to change the way how parent--child > > relationship currently works. > > > > Relevant functions is_dell_system_with_lis3lv02d() and > > register_dell_lis3lv02d_i2c_device() does not use internals of > > i2c-i801 and could be moved into new file, lets say > > drivers/platform/x86/dell/dell-smo8800-plat.c > > Put this file under a new hidden "bool" config option which is auto > > enabled when CONFIG_DELL_SMO8800 is used. > > > > i2c-i801.c currently has code: > > > > if (is_dell_system_with_lis3lv02d()) > > register_dell_lis3lv02d_i2c_device(priv); > > > > This can be put into a new exported function, e.g. > > void dell_smo8800_scan_i2c(struct i2c_adapter *adapter); > > And i2c-i801.c would call it instead. > > > > register_dell_lis3lv02d_i2c_device just needs "adapter", it does not > > need whole i801 priv struct. > > I'm wondering why we need all this. We have notifiers when a device is > added / removed. We can provide a board_info for the device and attach > it to the proper adapter, no? I do not know how flexible are notifiers. Can notifier call our callback when new "struct i2c_adapter *adapter" was instanced? > > With this simple change all dell smo8800 code would be in its subdir > > drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. > > > > This approach does not change any functionality, so should be absolutely > > safe. > > > > Future changes will be done only in drivers/platform/x86/dell/ subdir, > > touching i801 would not be needed at all. > > Still these exported functions are not the best solution we can do, > right? We should be able to decouple them without need for the custom > APIs. Well, what I described here is a simple change which get rid of the one problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx logic (like adding a new device id) requires touching unrelated i2c-i801.c source file. I like small changes which can be easily reviewed and address one problem. Step by step. That is why I proposed it here. For decoupling it is needed to get newly instanced adapter (if the mentioned notifier is able to tell this information) and also it is needed to check if the adapter is the i801.
On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote: > On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote: > > On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: ... > > I'm wondering why we need all this. We have notifiers when a device is > > added / removed. We can provide a board_info for the device and attach > > it to the proper adapter, no? > > I do not know how flexible are notifiers. Can notifier call our callback > when new "struct i2c_adapter *adapter" was instanced? You can follow notifications of *an* I2C adapter being added / removed. With that, you can filter which one is that. Based on that you may attach a saved (at __init as you talked about in the reply to Hans) board_info with all necessary information. Something like this (combined) https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515 https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194 > > > With this simple change all dell smo8800 code would be in its subdir > > > drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. > > > > > > This approach does not change any functionality, so should be absolutely > > > safe. > > > > > > Future changes will be done only in drivers/platform/x86/dell/ subdir, > > > touching i801 would not be needed at all. > > > > Still these exported functions are not the best solution we can do, > > right? We should be able to decouple them without need for the custom > > APIs. > > Well, what I described here is a simple change which get rid of the one > problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx > logic (like adding a new device id) requires touching unrelated > i2c-i801.c source file. `get rid of one problem` --> `replace one by another (but maybe less critical, dunno) problem`. The new one is the spread of custom APIs for a single user, which also requires an additional, shared header file and all hell with the Kconfig dependencies. > I like small changes which can be easily reviewed and address one > problem. Step by step. That is why I proposed it here. > > For decoupling it is needed to get newly instanced adapter (if the > mentioned notifier is able to tell this information) and also it is > needed to check if the adapter is the i801. Yes.
Hi, On 2/27/24 23:37, Andy Shevchenko wrote: > On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote: >> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote: >>> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote: > > ... > >>> I'm wondering why we need all this. We have notifiers when a device is >>> added / removed. We can provide a board_info for the device and attach >>> it to the proper adapter, no? >> >> I do not know how flexible are notifiers. Can notifier call our callback >> when new "struct i2c_adapter *adapter" was instanced? > > You can follow notifications of *an* I2C adapter being added / > removed. With that, you can filter which one is that. Based on that > you may attach a saved (at __init as you talked about in the reply to > Hans) board_info with all necessary information. > > Something like this (combined) > https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515 > https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194 drivers/platform/x86/touchscreen_dmi.c actually already does something like this for i2c-clients. The problem is that this brings probe-ordering problems with it. If the i801 driver is loaded before the dell-smo8800 driver then the notifiers will not trigger since the i2c-adapter has already been created (1). So we would still need a "cold-plug" manual scan in smo8800_probe() anyways at which point we might as well just return -EPROBE_DEFER when the adapter is not there. As for Pali's suggestion of having the i2c-i801 code call a symbol exported by dell-smo8800 that will cause the dell-smo8800 driver to load on all x86 devices with an i2c-i801 controller (pretty much all of them). Slowing the boot and eating memory. So IMHO just doing the scan for the i2c-i801 adapter as already done in this version of the patch-set, extended with returning -EPROBE_DEFER if it is not found is the best solution. Yes this means losing the i2c_client for the lis3lv02d device if the i2c-i801 driver is manually unbound or rmmod-ed. But that requires explicit root user action and putting just the i801 driver back in place again also requires implicit root action. IMHO it is acceptable that in this exceptional case, which normal users will never hit, a rmmod + modprobe of dell-smo8800 is required to re-gain the lis3lv02d i2c_client. Regards, Hans 1) touchscreen_dmi is builtin only because of this and we really want to avoid adding more of that. Actually thinking more about this it would be nice to modify touchscreen_dmi to use a mix of registering the notifier + doing a scan after registering it for matching devices which are already present, so that touchscreen_dmi can become a module auto-loaded using the DMI info which it already contains. > >>>> With this simple change all dell smo8800 code would be in its subdir >>>> drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code. >>>> >>>> This approach does not change any functionality, so should be absolutely >>>> safe. >>>> >>>> Future changes will be done only in drivers/platform/x86/dell/ subdir, >>>> touching i801 would not be needed at all. >>> >>> Still these exported functions are not the best solution we can do, >>> right? We should be able to decouple them without need for the custom >>> APIs. >> >> Well, what I described here is a simple change which get rid of the one >> problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx >> logic (like adding a new device id) requires touching unrelated >> i2c-i801.c source file. > > `get rid of one problem` --> `replace one by another (but maybe less > critical, dunno) problem`. The new one is the spread of custom APIs > for a single user, which also requires an additional, shared header > file and all hell with the Kconfig dependencies. > >> I like small changes which can be easily reviewed and address one >> problem. Step by step. That is why I proposed it here. >> >> For decoupling it is needed to get newly instanced adapter (if the >> mentioned notifier is able to tell this information) and also it is >> needed to check if the adapter is the i801. > > Yes. > >
Hi Pali, On 2/27/24 22:40, Pali Rohár wrote: > On Saturday 17 February 2024 11:33:21 Hans de Goede wrote: >> 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@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. (...) >>> >>> 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). > > Adding EPROBE_DEFER just complicates the dependency and state model. > I would really suggest to come up with a simpler solution, not too > complicated where it is required to think a lot if is is correct and if > all edge-cases are handled. I'm not sure what you are worried about here. dell-smo8800 is a leave driver, nothing inside the kernel depends on it being loaded or not. So there are no -EPROBE_DEFER complexities here, yes -EPROBE_DEFER can become a bit hairy with complex dependency graphs, but this is a very KISS case. Are there any specific scenarios you are actually worried about in this specific case? >> 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. > > Well, rmmod & modprobe is not the rare cases. Whatever developers say > about rmmod (or modprobe -r or whatever is the way for unloading > modules), this is something which is used by a lot of users and would be > used. Many modules actually have bugs in there remove paths and crash, this is really not a common case. Sometimes users use rmmod + modprobe surrounding suspend/resume for e.g. a wifi driver to work around suspend/resume problems but I have never heard of this being used for a driver like i2c-i801. And again if users are manually meddling with this, the they can also rmmod + modprobe dell-smo8800 after re-modprobing i2c-i801. >> 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. > > I'm not sure if it belongs to "SMO88xx ACPI platform_device" but for > sure I agree with you that it does not belong to i801 code. I would say > that it belongs to some SMO8800 glue code -- because it is not the > classic ACPI driver too. But I'm not against to have SMO glue code and > SMO ACPI driver in one file (maybe it is even better to have it). We are trying to get rid of acpi_driver drivers using only platform_driver drivers for the platform_devices created for ACPI devices / fwnodes which do not have another physical device. Also we only want this workaround when the SMO88xx ACPI fwnode is missing the I2cSerialBusV2 resource for the i2c_client and conveniently the platform_device will only be created for ACPI fwnodes without the I2cSerialBusV2 resource for proper ACPI fwnodes which have the I2C resource an i2c_client will be created instead. So putting the workaround in the platform_driver automatically ensures that it only runs when the ACPI fwnode is incomplete. > >> 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. > > I think we can take an assumption that ACPI SMO device does not change > it existence or ACPI enabled/disabled state during runtime. So we can > scan for ACPI SMO device just once in function stored in __init section > called during the kernel/module initialization and cache the result > (bool if device was found + its i2c address). After function marked as > __init finish its job then together with DMI tables can be discarded > from RAM. With this way it does take extra memory on every x86 system. > Also we can combine this with an SMO config option, so the whole code > "glue" code would not be compiled when SMO driver is not enabled via > Kconfig. This approach does not work because i2c-i801.c registers a PCI driver, there is no guarantee that the adapter has already been probed and an i2c_adapter has been created before it. A PCI driver's probe() function must not be __init and thus any code which it calls also must not be __init. So the majority of the smo88xx handling can not be __init. Regards, Hans
On Wed, Feb 28, 2024 at 3:10 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 2/27/24 22:40, Pali Rohár wrote: > > On Saturday 17 February 2024 11:33:21 Hans de Goede wrote: > >> On 2/13/24 17:30, Jean Delvare wrote: ... > >> 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. > > > > Well, rmmod & modprobe is not the rare cases. Whatever developers say > > about rmmod (or modprobe -r or whatever is the way for unloading > > modules), this is something which is used by a lot of users and would be > > used. > > Many modules actually have bugs in there remove paths and crash, > this is really not a common case. Sometimes users use rmmod + modprobe > surrounding suspend/resume for e.g. a wifi driver to work around > suspend/resume problems but I have never heard of this being used > for a driver like i2c-i801. Hmm... The whole thing of reworking the p2sb was done due to rebounding the i2c-i801 IIUC.
On Wednesday 28 February 2024 14:10:14 Hans de Goede 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). > > > > Adding EPROBE_DEFER just complicates the dependency and state model. > > I would really suggest to come up with a simpler solution, not too > > complicated where it is required to think a lot if is is correct and if > > all edge-cases are handled. > > I'm not sure what you are worried about here. dell-smo8800 is > a leave driver, nothing inside the kernel depends on it being > loaded or not. So there are no -EPROBE_DEFER complexities here, > yes -EPROBE_DEFER can become a bit hairy with complex dependency > graphs, but this is a very KISS case. > > Are there any specific scenarios you are actually worried about > in this specific case? -EPROBE_DEFER restarts and schedule probing of the device later. It does not inform device manager when it can try do it. So it can try probing it many more times until it decide to not try it again. This asynchronous design is broken and I do not see reason why to use it in another driver, in case we have a cleaner solution how to initialize and probe device without -EPROBE_DEFER. This is for sure not a KISS case but a case with lot of corner cases... and your proposed patch is just an example of it as you forgot about at least one corner case. Current solution does not have edge cases... this can be marked as KISS design. > >> 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. > > > > Well, rmmod & modprobe is not the rare cases. Whatever developers say > > about rmmod (or modprobe -r or whatever is the way for unloading > > modules), this is something which is used by a lot of users and would be > > used. > > Many modules actually have bugs in there remove paths and crash, > this is really not a common case. Sometimes users use rmmod + modprobe > surrounding suspend/resume for e.g. a wifi driver to work around > suspend/resume problems but I have never heard of this being used > for a driver like i2c-i801. > > And again if users are manually meddling with this, the they can > also rmmod + modprobe dell-smo8800 after re-modprobing i2c-i801. Argument that other modules have bugs in some code path does not mean to introduce bugs also into other modules. I do not take it. > >> 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. > > > > I think we can take an assumption that ACPI SMO device does not change > > it existence or ACPI enabled/disabled state during runtime. So we can > > scan for ACPI SMO device just once in function stored in __init section > > called during the kernel/module initialization and cache the result > > (bool if device was found + its i2c address). After function marked as > > __init finish its job then together with DMI tables can be discarded > > from RAM. With this way it does take extra memory on every x86 system. > > Also we can combine this with an SMO config option, so the whole code > > "glue" code would not be compiled when SMO driver is not enabled via > > Kconfig. > > This approach does not work because i2c-i801.c registers a PCI driver, > there is no guarantee that the adapter has already been probed and > an i2c_adapter has been created before it. A PCI driver's probe() > function must not be __init and thus any code which it calls also > must not be __init. > > So the majority of the smo88xx handling can not be __init. This argument is wrong. smo88xx has nothing with PCI, has even nothing with i2c. The detection is purely ACPI based and this can be called at any time after ACPI initialization. Detection does not need PCI. There is no reason why it cannot be called in __init section after ACPI is done.
On Saturday 02 March 2024 12:02:39 Hans de Goede wrote: > But the point is that moving the code does not help because > since there is a symbol used from the new code it will still > get loaded on all machines were the i2c-i801.c driver gets > loaded. So it will still be taking up RAM and increases > boot time (loading an extra module consumes time) on machines > which do not have any SMO88xx devices. > > And that point is still valid even independent of the code > is moved to the existing dell-smo8800 module or to a new > dell-smo8800-plat module. This is silly argument if you are opposing to adding trivial exported function with input argument struct i2c_adapter *adapter and with body if (smo88xx_detected) i2c_new_client_device(adapter, &info); with two static global variables: struct i2c_board_info info; bool smo88xx_detected; will be compiled and loaded on all x86 machines and taking too much RAM. Because that design with notifiers and other things would eat much more RAM and would be also slower. As I said in previous emails, detection (and so filling those two above static global variables) can be filled in the __init section and so would not take after bootup. For detection it is is needed to just call dmi_match(), acpi_get_devices() and dmi_get_system_info() which can be done in __init section. I do not see reason why not.
Hi All, Here is a patch series which implements my suggestions from: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ to improve the lis3lv02d accel support on Dell laptops. Jean, Andi the actual move is in patch 2/6 after 1 small prep patch on the dell-smo8800 side. My plan for merging this is to create an immutable branch based on 6.8-rc1 (once it is out) + these 6 patches and then send a pull-request for this. Can I have your Ack for the i2c-i801 changes in patch 2/6? I think you'll like the changes there since they only remove code on the i2c-i801.c side :) Changes in v2: - Drop "[PATCH 1/6] platform/x86: dell-smo8800: Only load on Dell laptops" - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses - Add a comment documenting the IDF PCI device ids - Keep using drivers/misc/lis3lv02d/lis3lv02d.c by default - Rename the module-parameter to use_iio_driver which can be set to use the IIO st_accel driver instead - Add a new patch adding the accelerometer address for the 2 models I have tested this on to dell_lis3lv02d_devices[] Regards, Hans Hans de Goede (6): platform/x86: dell-smo8800: Change probe() ordering a bit platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 platform/x86: dell-smo8800: Pass the IRQ to the lis3lv02d i2c_client platform/x86: dell-smo8800: Allow using the IIO st_accel driver platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address platform/x86: dell-smo8800: Add a couple more models to dell_lis3lv02d_devices[] drivers/i2c/busses/i2c-i801.c | 122 -------- drivers/platform/x86/dell/dell-smo8800.c | 341 +++++++++++++++++++++-- 2 files changed, 319 insertions(+), 144 deletions(-)