| Message ID | 20240704125643.22946-7-hdegoede@redhat.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d | expand |
On Thursday 04 July 2024 14:56:43 Hans de Goede wrote: > Unfortunately the SMOxxxx ACPI device does not contain the i2c-address > of the accelerometer. So a DMI product-name to address mapping table > is used. > > At support to have the kernel probe for the i2c-address for modesl > which are not on the list. > > The new probing code sits behind a new probe_i2c_addr module parameter, > which is disabled by default because probing might be dangerous. > > Link: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ > Signed-off-by: Hans de Goede <hdegoede@redhat.com> NAK. This is a hack, which should be avoided as specified in previous discussions (e.g. it can cause regression for future or also existing products). Author refused to improve the code, also refused to ask vendor about the details and proper implementation and author also refused to do any future discussion about it. Based on this state, this patch 6/6 should not be merged at all. > --- > Changes in v6: > - Use i2c_new_scanned_device() instead of re-inventing it > > Changes in v5: > - Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr) > --- > drivers/platform/x86/dell/dell-lis3lv02d.c | 52 ++++++++++++++++++++-- > 1 file changed, 48 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c > index ab02ad93758a..21390e6302a0 100644 > --- a/drivers/platform/x86/dell/dell-lis3lv02d.c > +++ b/drivers/platform/x86/dell/dell-lis3lv02d.c > @@ -15,6 +15,8 @@ > #include <linux/workqueue.h> > #include "dell-smo8800-ids.h" > > +#define LIS3_WHO_AM_I 0x0f > + > #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \ > { \ > .matches = { \ > @@ -57,6 +59,38 @@ static u8 i2c_addr; > static struct i2c_client *i2c_dev; > static bool notifier_registered; > > +static bool probe_i2c_addr; > +module_param(probe_i2c_addr, bool, 0444); > +MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown, this may be dangerous."); > + > +static int detect_lis3lv02d(struct i2c_adapter *adap, unsigned short addr) > +{ > + union i2c_smbus_data smbus_data; > + int err; > + > + pr_info("Probing for lis3lv02d on address 0x%02x\n", addr); > + > + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I, > + I2C_SMBUS_BYTE_DATA, &smbus_data); > + if (err < 0) > + return 0; /* Not found */ > + > + /* valid who-am-i values are from drivers/misc/lis3lv02d/lis3lv02d.c */ > + switch (smbus_data.byte) { > + case 0x32: > + case 0x33: > + case 0x3a: > + case 0x3b: > + break; > + default: > + pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte); > + return 0; /* Not found */ > + } > + > + pr_debug("Detected lis3lv02d on address 0x%02x\n", addr); > + return 1; /* Found */ > +} > + > static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap) > { > /* > @@ -93,10 +127,18 @@ static void instantiate_i2c_client(struct work_struct *work) > if (!adap) > return; > > - info.addr = i2c_addr; > strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); > > - i2c_dev = i2c_new_client_device(adap, &info); > + if (i2c_addr) { > + info.addr = i2c_addr; > + i2c_dev = i2c_new_client_device(adap, &info); > + } else { > + /* First try address 0x29 (most used) and then try 0x1d */ > + static const unsigned short addr_list[] = { 0x29, 0x1d, I2C_CLIENT_END }; > + > + i2c_dev = i2c_new_scanned_device(adap, &info, addr_list, detect_lis3lv02d); > + } > + > if (IS_ERR(i2c_dev)) { > pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev)); > i2c_dev = NULL; > @@ -167,12 +209,14 @@ static int __init dell_lis3lv02d_init(void) > put_device(dev); > > lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices); > - if (!lis3lv02d_dmi_id) { > + if (!lis3lv02d_dmi_id && !probe_i2c_addr) { > pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n"); > + pr_info("Pass dell_lis3lv02d.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n"); > return 0; > } > > - i2c_addr = (long)lis3lv02d_dmi_id->driver_data; > + if (lis3lv02d_dmi_id) > + i2c_addr = (long)lis3lv02d_dmi_id->driver_data; > > /* > * Register i2c-bus notifier + queue initial scan for lis3lv02d > -- > 2.45.1 >
Hi, On 7/4/24 11:37 PM, Pali Rohár wrote: > On Thursday 04 July 2024 14:56:43 Hans de Goede wrote: >> Unfortunately the SMOxxxx ACPI device does not contain the i2c-address >> of the accelerometer. So a DMI product-name to address mapping table >> is used. >> >> At support to have the kernel probe for the i2c-address for modesl >> which are not on the list. >> >> The new probing code sits behind a new probe_i2c_addr module parameter, >> which is disabled by default because probing might be dangerous. >> >> Link: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > NAK. > > This is a hack This is not a hack, notice the existing i2c_new_scanned_device() i2c-core function exists for a reason. As I have tried to explain before scanning for i2c-devices if we don't know the address is something which the kernel has been doing for a long time already. Current kernels scan for i2c devices on pretty much any Intel PC in the form of i2c_register_spd() running. >, which should be avoided as specified in previous > discussions (e.g. it can cause regression for future or also existing > products). You have provided 0 proof or even any hypothesis / speculations how this can cause regressions. Al you have done is said this may cause regressions without providing any details as to how you believe this would cause regressions please provide details. Also the new code is only activated if a new module option is st. By default this options is not set, so this cannot cause any regressions since it does not change anything for end users unless they explicitly enable it. You have made it plenty clear that you don't like this approach, but claiming it can cause regressions when by default it does not do anything is just dishonest. > Author refused to improve the code, Really? I have gone out of my way to please you, I've moved all of the i2c handling to a separate file because you asked for this, adding a text to both the kernel message informing users about the module-parameter to probe and the module-param help text that this may be dangerous. Also after I last discussion I moved to i2c_new_scanned_device() instead of DIY code. There is a reason why this patch-set is at v6 and it is not because I'm refusing to improve it. > also refused to ask vendor about the > details and proper implementation and author also refused to do any > future discussion about it. As I have explained countless times I have no contacts inside Dell to ask about this. If e.g. Mario was still at Dell I would have asked Mario about this immediately back when the discussion started. Besides the Dell.Client.Kernel@dell.com which no-one seems to be reading, there is only one other @dell.com address in all of MAINTAINERS: Prasanth Ksr <prasanth.ksr@dell.com> To put an end to this whole discussion about contacting Dell I'll email them with you (Pali) in the Cc. I don't expect much to come from this but we will see. > Based on this state, this patch 6/6 should not be merged at all. Lets move forward with merging patches 1-5 and wait to see if we get any response from Dell. But I do very much want to move forward with this if contacting Dell does not result in another solution to allow users to easily find out what the i2c-address is. Regards, Hans > >> --- >> Changes in v6: >> - Use i2c_new_scanned_device() instead of re-inventing it >> >> Changes in v5: >> - Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr) >> --- >> drivers/platform/x86/dell/dell-lis3lv02d.c | 52 ++++++++++++++++++++-- >> 1 file changed, 48 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c >> index ab02ad93758a..21390e6302a0 100644 >> --- a/drivers/platform/x86/dell/dell-lis3lv02d.c >> +++ b/drivers/platform/x86/dell/dell-lis3lv02d.c >> @@ -15,6 +15,8 @@ >> #include <linux/workqueue.h> >> #include "dell-smo8800-ids.h" >> >> +#define LIS3_WHO_AM_I 0x0f >> + >> #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \ >> { \ >> .matches = { \ >> @@ -57,6 +59,38 @@ static u8 i2c_addr; >> static struct i2c_client *i2c_dev; >> static bool notifier_registered; >> >> +static bool probe_i2c_addr; >> +module_param(probe_i2c_addr, bool, 0444); >> +MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown, this may be dangerous."); >> + >> +static int detect_lis3lv02d(struct i2c_adapter *adap, unsigned short addr) >> +{ >> + union i2c_smbus_data smbus_data; >> + int err; >> + >> + pr_info("Probing for lis3lv02d on address 0x%02x\n", addr); >> + >> + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I, >> + I2C_SMBUS_BYTE_DATA, &smbus_data); >> + if (err < 0) >> + return 0; /* Not found */ >> + >> + /* valid who-am-i values are from drivers/misc/lis3lv02d/lis3lv02d.c */ >> + switch (smbus_data.byte) { >> + case 0x32: >> + case 0x33: >> + case 0x3a: >> + case 0x3b: >> + break; >> + default: >> + pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte); >> + return 0; /* Not found */ >> + } >> + >> + pr_debug("Detected lis3lv02d on address 0x%02x\n", addr); >> + return 1; /* Found */ >> +} >> + >> static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap) >> { >> /* >> @@ -93,10 +127,18 @@ static void instantiate_i2c_client(struct work_struct *work) >> if (!adap) >> return; >> >> - info.addr = i2c_addr; >> strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); >> >> - i2c_dev = i2c_new_client_device(adap, &info); >> + if (i2c_addr) { >> + info.addr = i2c_addr; >> + i2c_dev = i2c_new_client_device(adap, &info); >> + } else { >> + /* First try address 0x29 (most used) and then try 0x1d */ >> + static const unsigned short addr_list[] = { 0x29, 0x1d, I2C_CLIENT_END }; >> + >> + i2c_dev = i2c_new_scanned_device(adap, &info, addr_list, detect_lis3lv02d); >> + } >> + >> if (IS_ERR(i2c_dev)) { >> pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev)); >> i2c_dev = NULL; >> @@ -167,12 +209,14 @@ static int __init dell_lis3lv02d_init(void) >> put_device(dev); >> >> lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices); >> - if (!lis3lv02d_dmi_id) { >> + if (!lis3lv02d_dmi_id && !probe_i2c_addr) { >> pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n"); >> + pr_info("Pass dell_lis3lv02d.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n"); >> return 0; >> } >> >> - i2c_addr = (long)lis3lv02d_dmi_id->driver_data; >> + if (lis3lv02d_dmi_id) >> + i2c_addr = (long)lis3lv02d_dmi_id->driver_data; >> >> /* >> * Register i2c-bus notifier + queue initial scan for lis3lv02d >> -- >> 2.45.1 >> >
diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c index ab02ad93758a..21390e6302a0 100644 --- a/drivers/platform/x86/dell/dell-lis3lv02d.c +++ b/drivers/platform/x86/dell/dell-lis3lv02d.c @@ -15,6 +15,8 @@ #include <linux/workqueue.h> #include "dell-smo8800-ids.h" +#define LIS3_WHO_AM_I 0x0f + #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \ { \ .matches = { \ @@ -57,6 +59,38 @@ static u8 i2c_addr; static struct i2c_client *i2c_dev; static bool notifier_registered; +static bool probe_i2c_addr; +module_param(probe_i2c_addr, bool, 0444); +MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown, this may be dangerous."); + +static int detect_lis3lv02d(struct i2c_adapter *adap, unsigned short addr) +{ + union i2c_smbus_data smbus_data; + int err; + + pr_info("Probing for lis3lv02d on address 0x%02x\n", addr); + + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I, + I2C_SMBUS_BYTE_DATA, &smbus_data); + if (err < 0) + return 0; /* Not found */ + + /* valid who-am-i values are from drivers/misc/lis3lv02d/lis3lv02d.c */ + switch (smbus_data.byte) { + case 0x32: + case 0x33: + case 0x3a: + case 0x3b: + break; + default: + pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte); + return 0; /* Not found */ + } + + pr_debug("Detected lis3lv02d on address 0x%02x\n", addr); + return 1; /* Found */ +} + static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap) { /* @@ -93,10 +127,18 @@ static void instantiate_i2c_client(struct work_struct *work) if (!adap) return; - info.addr = i2c_addr; strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); - i2c_dev = i2c_new_client_device(adap, &info); + if (i2c_addr) { + info.addr = i2c_addr; + i2c_dev = i2c_new_client_device(adap, &info); + } else { + /* First try address 0x29 (most used) and then try 0x1d */ + static const unsigned short addr_list[] = { 0x29, 0x1d, I2C_CLIENT_END }; + + i2c_dev = i2c_new_scanned_device(adap, &info, addr_list, detect_lis3lv02d); + } + if (IS_ERR(i2c_dev)) { pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev)); i2c_dev = NULL; @@ -167,12 +209,14 @@ static int __init dell_lis3lv02d_init(void) put_device(dev); lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices); - if (!lis3lv02d_dmi_id) { + if (!lis3lv02d_dmi_id && !probe_i2c_addr) { pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n"); + pr_info("Pass dell_lis3lv02d.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n"); return 0; } - i2c_addr = (long)lis3lv02d_dmi_id->driver_data; + if (lis3lv02d_dmi_id) + i2c_addr = (long)lis3lv02d_dmi_id->driver_data; /* * Register i2c-bus notifier + queue initial scan for lis3lv02d
Unfortunately the SMOxxxx ACPI device does not contain the i2c-address of the accelerometer. So a DMI product-name to address mapping table is used. At support to have the kernel probe for the i2c-address for modesl which are not on the list. The new probing code sits behind a new probe_i2c_addr module parameter, which is disabled by default because probing might be dangerous. Link: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- Changes in v6: - Use i2c_new_scanned_device() instead of re-inventing it Changes in v5: - Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr) --- drivers/platform/x86/dell/dell-lis3lv02d.c | 52 ++++++++++++++++++++-- 1 file changed, 48 insertions(+), 4 deletions(-)