| Message ID | 20241209183557.7560-5-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d | expand |
On Mon, 9 Dec 2024, 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> So what was the result of the private inquiry to Dell? Did they respond? Did they provide useful info? > Changes in v8: > - Use dev_err() / dev_dbg() where possible using &adap->dev as the device > for logging > > 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 | 53 ++++++++++++++++++++-- > 1 file changed, 49 insertions(+), 4 deletions(-) > > diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c > index d2b34e10c5eb..8d9dc59c7d8c 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,39 @@ 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; > + > + dev_info(&adap->dev, "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: > + dev_warn(&adap->dev, "Unknown who-am-i register value 0x%02x\n", > + smbus_data.byte); > + return 0; /* Not found */ > + } > + > + dev_dbg(&adap->dev, "Detected lis3lv02d on address 0x%02x\n", addr); > + return 1; /* Found */ > +} > + > static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap) > { > /* > @@ -97,10 +132,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)) { > dev_err(&adap->dev, "error %ld registering i2c_client\n", PTR_ERR(i2c_dev)); > i2c_dev = NULL; > @@ -169,12 +212,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; If somebody enables this parameter and it successfully finds a device, shouldn't the user be instructed to report the info so that new entries can be added and the probe parameter is no longer needed in those case?
Hi Ilpo, Thank you for taking a look a this patch. On 17-Dec-24 5:48 PM, Ilpo Järvinen wrote: > On Mon, 9 Dec 2024, 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> > > So what was the result of the private inquiry to Dell? On July 5th I send the following email to Prasanth Ksr <prasanth.ksr@dell.com> which is the only dell.com address I could find in MAINTAINERS other then Dell.Client.Kernel@dell.com which does not seem to be monitored very actively: """ Hello Prasanth, I'm contacting you about a question lis3lv02d freelfall sensors / accelerometers used on many (older) Dell laptop models. There has been a question about this last December and a patch-set trying to address part of this with Dell.Client.Kernel@dell.com in the Cc but no-one seems to be responding to that email address which is why I'm contacting you directly: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ https://lore.kernel.org/platform-driver-x86/20240704125643.22946-1-hdegoede@redhat.com/ If you are not the right person to ask these questions to, then please forward this email to the right person. The lis3lv02d sensors are I2C devices and are described in the ACPI tables with an SMO88xx ACPI device node. The problem is that these ACPI device nodes do not have an ACPI I2cResouce in there resource (_CRS) list, so the I2C address of the sensor is unknown. When support was first added for these Dell provided a list of model-name to I2C address mappings for the then current generation of laptops, see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 And later the community added a few more mappings. Paul Menzel, the author of the email starting the discussion on this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 did a search for the kernel message which is printed when an SMO88xx ACPI device is found but the i2c-address is unknown and Paul found many models are missing from the mapping table (see Paul's email). Which leads us to the following questions: 1. Is there another, uniform (so not using a model name table) way to find out the I2C address of the SMO88xx freefall sensor from the ACPI or SMBIOS tables ? 2. If we need to keep using the model-name to I2C-address mapping table can you help us complete it by providing the sensor's I2C address for all models Paul has found where this is currently missing ? Regards, Hans """ Pali and Paul Menzel where in the Cc of this email. > Did they respond? I got a reply from Prasanth that they would forward my request to the correct team. Then I got on off-list reply to the v6 patch-set from David Wang from Dell with as relevant content "We are working on it." > Did they provide useful info? No further info was received after the "We are working on it." email. >> Changes in v8: >> - Use dev_err() / dev_dbg() where possible using &adap->dev as the device >> for logging >> >> 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 | 53 ++++++++++++++++++++-- >> 1 file changed, 49 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c >> index d2b34e10c5eb..8d9dc59c7d8c 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,39 @@ 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; >> + >> + dev_info(&adap->dev, "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: >> + dev_warn(&adap->dev, "Unknown who-am-i register value 0x%02x\n", >> + smbus_data.byte); >> + return 0; /* Not found */ >> + } >> + >> + dev_dbg(&adap->dev, "Detected lis3lv02d on address 0x%02x\n", addr); >> + return 1; /* Found */ >> +} >> + >> static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap) >> { >> /* >> @@ -97,10 +132,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)) { >> dev_err(&adap->dev, "error %ld registering i2c_client\n", PTR_ERR(i2c_dev)); >> i2c_dev = NULL; >> @@ -169,12 +212,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; > > If somebody enables this parameter and it successfully finds a device, > shouldn't the user be instructed to report the info so that new entries > can be added and the probe parameter is no longer needed in those case? Ah, IIRC that used to be there, but I guess that was lost when I switched from DIY probing code to using the i2c_new_scanned_device() helper for this in v6 of the series. I'll prepare a v10 of this patch changing: dev_dbg(&adap->dev, "Detected lis3lv02d on address 0x%02x\n", addr); to: dev_info(&adap->dev, "Detected lis3lv02d on address 0x%02x, please report this upstream to platform-driver-x86@vger.kernel.org so that a quirk can be added\n", addr); to address this. Regards, Hans
On Sat, 21 Dec 2024, Hans de Goede wrote: > Hi Ilpo, > > Thank you for taking a look a this patch. > > On 17-Dec-24 5:48 PM, Ilpo Järvinen wrote: > > On Mon, 9 Dec 2024, 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> > > > > So what was the result of the private inquiry to Dell? > > On July 5th I send the following email to Prasanth Ksr > <prasanth.ksr@dell.com> which is the only dell.com address I could > find in MAINTAINERS other then Dell.Client.Kernel@dell.com which > does not seem to be monitored very actively: > > """ > Hello Prasanth, > > I'm contacting you about a question lis3lv02d freelfall sensors / > accelerometers used on many (older) Dell laptop models. There > has been a question about this last December and a patch-set > trying to address part of this with Dell.Client.Kernel@dell.com > in the Cc but no-one seems to be responding to that email address > which is why I'm contacting you directly: > > https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ > https://lore.kernel.org/platform-driver-x86/20240704125643.22946-1-hdegoede@redhat.com/ > > If you are not the right person to ask these questions to, then > please forward this email to the right person. > > The lis3lv02d sensors are I2C devices and are described in the ACPI > tables with an SMO88xx ACPI device node. The problem is that these > ACPI device nodes do not have an ACPI I2cResouce in there resource > (_CRS) list, so the I2C address of the sensor is unknown. > > When support was first added for these Dell provided a list of > model-name to I2C address mappings for the then current generation > of laptops, see: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > > And later the community added a few more mappings. > > Paul Menzel, the author of the email starting the discussion on this: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > > did a search for the kernel message which is printed when an SMO88xx > ACPI device is found but the i2c-address is unknown and Paul found > many models are missing from the mapping table (see Paul's email). > > Which leads us to the following questions: > > 1. Is there another, uniform (so not using a model name table) > way to find out the I2C address of the SMO88xx freefall sensor > from the ACPI or SMBIOS tables ? > > 2. If we need to keep using the model-name to I2C-address mapping > table can you help us complete it by providing the sensor's I2C > address for all models Paul has found where this is currently missing ? > > Regards, > > Hans > """ > > Pali and Paul Menzel where in the Cc of this email. > > > Did they respond? > > I got a reply from Prasanth that they would forward my request to the > correct team. Then I got on off-list reply to the v6 patch-set from > David Wang from Dell with as relevant content "We are working on it." > > > Did they provide useful info? > > No further info was received after the "We are working on it." email. Hi Hans, So you didn't try to remind them after that at all? This kind of sounds a low priority item they just forgot to do and might have had an intention to follow through.
Hi, On 13-Jan-25 4:36 PM, Andy Shevchenko wrote: > On Mon, Jan 13, 2025 at 05:17:43PM +0200, Ilpo Järvinen wrote: >> On Sat, 21 Dec 2024, Hans de Goede wrote: >>> On 17-Dec-24 5:48 PM, Ilpo Järvinen wrote: >>>> On Mon, 9 Dec 2024, Hans de Goede wrote: > > ... > >>>> So what was the result of the private inquiry to Dell? >>> >>> On July 5th I send the following email to Prasanth Ksr >>> <prasanth.ksr@dell.com> which is the only dell.com address I could >>> find in MAINTAINERS other then Dell.Client.Kernel@dell.com which >>> does not seem to be monitored very actively: >>> >>> """ >>> Hello Prasanth, >>> >>> I'm contacting you about a question lis3lv02d freelfall sensors / >>> accelerometers used on many (older) Dell laptop models. There >>> has been a question about this last December and a patch-set >>> trying to address part of this with Dell.Client.Kernel@dell.com >>> in the Cc but no-one seems to be responding to that email address >>> which is why I'm contacting you directly: >>> >>> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ >>> https://lore.kernel.org/platform-driver-x86/20240704125643.22946-1-hdegoede@redhat.com/ >>> >>> If you are not the right person to ask these questions to, then >>> please forward this email to the right person. >>> >>> The lis3lv02d sensors are I2C devices and are described in the ACPI >>> tables with an SMO88xx ACPI device node. The problem is that these >>> ACPI device nodes do not have an ACPI I2cResouce in there resource >>> (_CRS) list, so the I2C address of the sensor is unknown. >>> >>> When support was first added for these Dell provided a list of >>> model-name to I2C address mappings for the then current generation >>> of laptops, see: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 >>> >>> And later the community added a few more mappings. >>> >>> Paul Menzel, the author of the email starting the discussion on this: >>> >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 >>> >>> did a search for the kernel message which is printed when an SMO88xx >>> ACPI device is found but the i2c-address is unknown and Paul found >>> many models are missing from the mapping table (see Paul's email). >>> >>> Which leads us to the following questions: >>> >>> 1. Is there another, uniform (so not using a model name table) >>> way to find out the I2C address of the SMO88xx freefall sensor >>> from the ACPI or SMBIOS tables ? >>> >>> 2. If we need to keep using the model-name to I2C-address mapping >>> table can you help us complete it by providing the sensor's I2C >>> address for all models Paul has found where this is currently missing ? >>> >>> Regards, >>> >>> Hans >>> """ >>> >>> Pali and Paul Menzel where in the Cc of this email. >>> >>>> Did they respond? >>> >>> I got a reply from Prasanth that they would forward my request to the >>> correct team. Then I got on off-list reply to the v6 patch-set from >>> David Wang from Dell with as relevant content "We are working on it." >>> >>>> Did they provide useful info? >>> >>> No further info was received after the "We are working on it." email. >> >> Hi Hans, >> >> So you didn't try to remind them after that at all? >> >> This kind of sounds a low priority item they just forgot to do and might have >> had an intention to follow through. > > Talking from my experience with other companies that could have done something > better I dare to say that this entire buzz for them is no-priority at all, like > "no money stuff", hence no attention given. That said, I believe ping won't > change anything here, however I agree that it _was_ worth to try to acquire any > response from them. Basically what Andy says above. Note that Dell's client team has been on the Cc for all the versions of this patch-set many of which were posted after the "We are working on it." email. For completeness sake I have just send a request for a status update on this to Prasanth and David from Dell. In the mean time it would be good IMO to merge v11 of this patch, if we get useful info from Dell after all we can modify the driver for this later. Regards, Hans
On Monday 13 January 2025 17:49:19 Hans de Goede wrote: > Hi, > > On 13-Jan-25 4:36 PM, Andy Shevchenko wrote: > > On Mon, Jan 13, 2025 at 05:17:43PM +0200, Ilpo Järvinen wrote: > >> On Sat, 21 Dec 2024, Hans de Goede wrote: > >>> On 17-Dec-24 5:48 PM, Ilpo Järvinen wrote: > >>>> On Mon, 9 Dec 2024, Hans de Goede wrote: > > > > ... > > > >>>> So what was the result of the private inquiry to Dell? > >>> > >>> On July 5th I send the following email to Prasanth Ksr > >>> <prasanth.ksr@dell.com> which is the only dell.com address I could > >>> find in MAINTAINERS other then Dell.Client.Kernel@dell.com which > >>> does not seem to be monitored very actively: > >>> > >>> """ > >>> Hello Prasanth, > >>> > >>> I'm contacting you about a question lis3lv02d freelfall sensors / > >>> accelerometers used on many (older) Dell laptop models. There > >>> has been a question about this last December and a patch-set > >>> trying to address part of this with Dell.Client.Kernel@dell.com > >>> in the Cc but no-one seems to be responding to that email address > >>> which is why I'm contacting you directly: > >>> > >>> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ > >>> https://lore.kernel.org/platform-driver-x86/20240704125643.22946-1-hdegoede@redhat.com/ > >>> > >>> If you are not the right person to ask these questions to, then > >>> please forward this email to the right person. > >>> > >>> The lis3lv02d sensors are I2C devices and are described in the ACPI > >>> tables with an SMO88xx ACPI device node. The problem is that these > >>> ACPI device nodes do not have an ACPI I2cResouce in there resource > >>> (_CRS) list, so the I2C address of the sensor is unknown. > >>> > >>> When support was first added for these Dell provided a list of > >>> model-name to I2C address mappings for the then current generation > >>> of laptops, see: > >>> > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > >>> > >>> And later the community added a few more mappings. > >>> > >>> Paul Menzel, the author of the email starting the discussion on this: > >>> > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > >>> > >>> did a search for the kernel message which is printed when an SMO88xx > >>> ACPI device is found but the i2c-address is unknown and Paul found > >>> many models are missing from the mapping table (see Paul's email). > >>> > >>> Which leads us to the following questions: > >>> > >>> 1. Is there another, uniform (so not using a model name table) > >>> way to find out the I2C address of the SMO88xx freefall sensor > >>> from the ACPI or SMBIOS tables ? > >>> > >>> 2. If we need to keep using the model-name to I2C-address mapping > >>> table can you help us complete it by providing the sensor's I2C > >>> address for all models Paul has found where this is currently missing ? > >>> > >>> Regards, > >>> > >>> Hans > >>> """ > >>> > >>> Pali and Paul Menzel where in the Cc of this email. > >>> > >>>> Did they respond? > >>> > >>> I got a reply from Prasanth that they would forward my request to the > >>> correct team. Then I got on off-list reply to the v6 patch-set from > >>> David Wang from Dell with as relevant content "We are working on it." > >>> > >>>> Did they provide useful info? > >>> > >>> No further info was received after the "We are working on it." email. > >> > >> Hi Hans, > >> > >> So you didn't try to remind them after that at all? > >> > >> This kind of sounds a low priority item they just forgot to do and might have > >> had an intention to follow through. > > > > Talking from my experience with other companies that could have done something > > better I dare to say that this entire buzz for them is no-priority at all, like > > "no money stuff", hence no attention given. That said, I believe ping won't > > change anything here, however I agree that it _was_ worth to try to acquire any > > response from them. > > Basically what Andy says above. > > Note that Dell's client team has been on the Cc for all the versions of > this patch-set many of which were posted after the "We are working on it." email. > > For completeness sake I have just send a request for a status update on > this to Prasanth and David from Dell. > > In the mean time it would be good IMO to merge v11 of this patch, if we > get useful info from Dell after all we can modify the driver for this > later. > > Regards, > > Hans > > No, this change should not be taken at all. This change has a chance to break booting or brick future dell devices. I'm not going to discuss it again, but saying that it is good just because you do not have anything better is not an argument to take such change. Also it is not an excuse to hide dangerous things behind module parameter. And if you have been doing to everything to ensure that companies would not want to tak with you then sorry it is only your problem, so please do not complain here.
On Mon, Jan 13, 2025 at 9:47 PM Pali Rohár <pali@kernel.org> wrote: > On Monday 13 January 2025 17:49:19 Hans de Goede wrote: > > On 13-Jan-25 4:36 PM, Andy Shevchenko wrote: > > > On Mon, Jan 13, 2025 at 05:17:43PM +0200, Ilpo Järvinen wrote: > > >> On Sat, 21 Dec 2024, Hans de Goede wrote: > > >>> On 17-Dec-24 5:48 PM, Ilpo Järvinen wrote: > > >>>> On Mon, 9 Dec 2024, Hans de Goede wrote: ... > > >>>> So what was the result of the private inquiry to Dell? > > >>> > > >>> On July 5th I send the following email to Prasanth Ksr > > >>> <prasanth.ksr@dell.com> which is the only dell.com address I could > > >>> find in MAINTAINERS other then Dell.Client.Kernel@dell.com which > > >>> does not seem to be monitored very actively: > > >>> > > >>> """ > > >>> Hello Prasanth, > > >>> > > >>> I'm contacting you about a question lis3lv02d freelfall sensors / > > >>> accelerometers used on many (older) Dell laptop models. There > > >>> has been a question about this last December and a patch-set > > >>> trying to address part of this with Dell.Client.Kernel@dell.com > > >>> in the Cc but no-one seems to be responding to that email address > > >>> which is why I'm contacting you directly: > > >>> > > >>> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ > > >>> https://lore.kernel.org/platform-driver-x86/20240704125643.22946-1-hdegoede@redhat.com/ > > >>> > > >>> If you are not the right person to ask these questions to, then > > >>> please forward this email to the right person. > > >>> > > >>> The lis3lv02d sensors are I2C devices and are described in the ACPI > > >>> tables with an SMO88xx ACPI device node. The problem is that these > > >>> ACPI device nodes do not have an ACPI I2cResouce in there resource > > >>> (_CRS) list, so the I2C address of the sensor is unknown. > > >>> > > >>> When support was first added for these Dell provided a list of > > >>> model-name to I2C address mappings for the then current generation > > >>> of laptops, see: > > >>> > > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > > >>> > > >>> And later the community added a few more mappings. > > >>> > > >>> Paul Menzel, the author of the email starting the discussion on this: > > >>> > > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > > >>> > > >>> did a search for the kernel message which is printed when an SMO88xx > > >>> ACPI device is found but the i2c-address is unknown and Paul found > > >>> many models are missing from the mapping table (see Paul's email). > > >>> > > >>> Which leads us to the following questions: > > >>> > > >>> 1. Is there another, uniform (so not using a model name table) > > >>> way to find out the I2C address of the SMO88xx freefall sensor > > >>> from the ACPI or SMBIOS tables ? > > >>> > > >>> 2. If we need to keep using the model-name to I2C-address mapping > > >>> table can you help us complete it by providing the sensor's I2C > > >>> address for all models Paul has found where this is currently missing ? > > >>> > > >>> Regards, > > >>> > > >>> Hans > > >>> """ > > >>> > > >>> Pali and Paul Menzel where in the Cc of this email. > > >>> > > >>>> Did they respond? > > >>> > > >>> I got a reply from Prasanth that they would forward my request to the > > >>> correct team. Then I got on off-list reply to the v6 patch-set from > > >>> David Wang from Dell with as relevant content "We are working on it." > > >>> > > >>>> Did they provide useful info? > > >>> > > >>> No further info was received after the "We are working on it." email. > > >> > > >> Hi Hans, > > >> > > >> So you didn't try to remind them after that at all? > > >> > > >> This kind of sounds a low priority item they just forgot to do and might have > > >> had an intention to follow through. > > > > > > Talking from my experience with other companies that could have done something > > > better I dare to say that this entire buzz for them is no-priority at all, like > > > "no money stuff", hence no attention given. That said, I believe ping won't > > > change anything here, however I agree that it _was_ worth to try to acquire any > > > response from them. > > > > Basically what Andy says above. > > > > Note that Dell's client team has been on the Cc for all the versions of > > this patch-set many of which were posted after the "We are working on it." email. > > > > For completeness sake I have just send a request for a status update on > > this to Prasanth and David from Dell. > > > > In the mean time it would be good IMO to merge v11 of this patch, if we > > get useful info from Dell after all we can modify the driver for this > > later. > > No, this change should not be taken at all. This change has a chance to > break booting or brick future dell devices. I'm not going to discuss it > again, but saying that it is good just because you do not have anything > better is not an argument to take such change. Also it is not an excuse > to hide dangerous things behind module parameter. And if you have been > doing to everything to ensure that companies would not want to tak with > you then sorry it is only your problem, so please do not complain here. With all respect, this is not how we should treat the Linux kernel contributors and users (who want this feature to enable their devices). We have a ton of dangerous and DANGEROUS parameters and other algorithms here and there (in Linux kernel source tree), moreover, users with all responsibility may kill themselves with a laptop just by hitting their head or igniting Li-ion battery to set off a blast or heavy fire. Is there any _practical_ protection for that? No. Do you suggest we should ban Li-ion because of this? I don't see it, but be consistent, do it!
On Mon, 13 Jan 2025, Pali Rohár wrote: > On Monday 13 January 2025 17:49:19 Hans de Goede wrote: > > On 13-Jan-25 4:36 PM, Andy Shevchenko wrote: > > > On Mon, Jan 13, 2025 at 05:17:43PM +0200, Ilpo Järvinen wrote: > > >> On Sat, 21 Dec 2024, Hans de Goede wrote: > > >>> On 17-Dec-24 5:48 PM, Ilpo Järvinen wrote: > > >>>> On Mon, 9 Dec 2024, Hans de Goede wrote: > > > > > > ... > > > > > >>>> So what was the result of the private inquiry to Dell? > > >>> > > >>> On July 5th I send the following email to Prasanth Ksr > > >>> <prasanth.ksr@dell.com> which is the only dell.com address I could > > >>> find in MAINTAINERS other then Dell.Client.Kernel@dell.com which > > >>> does not seem to be monitored very actively: > > >>> > > >>> """ > > >>> Hello Prasanth, > > >>> > > >>> I'm contacting you about a question lis3lv02d freelfall sensors / > > >>> accelerometers used on many (older) Dell laptop models. There > > >>> has been a question about this last December and a patch-set > > >>> trying to address part of this with Dell.Client.Kernel@dell.com > > >>> in the Cc but no-one seems to be responding to that email address > > >>> which is why I'm contacting you directly: > > >>> > > >>> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ > > >>> https://lore.kernel.org/platform-driver-x86/20240704125643.22946-1-hdegoede@redhat.com/ > > >>> > > >>> If you are not the right person to ask these questions to, then > > >>> please forward this email to the right person. > > >>> > > >>> The lis3lv02d sensors are I2C devices and are described in the ACPI > > >>> tables with an SMO88xx ACPI device node. The problem is that these > > >>> ACPI device nodes do not have an ACPI I2cResouce in there resource > > >>> (_CRS) list, so the I2C address of the sensor is unknown. > > >>> > > >>> When support was first added for these Dell provided a list of > > >>> model-name to I2C address mappings for the then current generation > > >>> of laptops, see: > > >>> > > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > > >>> > > >>> And later the community added a few more mappings. > > >>> > > >>> Paul Menzel, the author of the email starting the discussion on this: > > >>> > > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > > >>> > > >>> did a search for the kernel message which is printed when an SMO88xx > > >>> ACPI device is found but the i2c-address is unknown and Paul found > > >>> many models are missing from the mapping table (see Paul's email). > > >>> > > >>> Which leads us to the following questions: > > >>> > > >>> 1. Is there another, uniform (so not using a model name table) > > >>> way to find out the I2C address of the SMO88xx freefall sensor > > >>> from the ACPI or SMBIOS tables ? > > >>> > > >>> 2. If we need to keep using the model-name to I2C-address mapping > > >>> table can you help us complete it by providing the sensor's I2C > > >>> address for all models Paul has found where this is currently missing ? > > >>> > > >>> Regards, > > >>> > > >>> Hans > > >>> """ > > >>> > > >>> Pali and Paul Menzel where in the Cc of this email. > > >>> > > >>>> Did they respond? > > >>> > > >>> I got a reply from Prasanth that they would forward my request to the > > >>> correct team. Then I got on off-list reply to the v6 patch-set from > > >>> David Wang from Dell with as relevant content "We are working on it." > > >>> > > >>>> Did they provide useful info? > > >>> > > >>> No further info was received after the "We are working on it." email. > > >> > > >> Hi Hans, > > >> > > >> So you didn't try to remind them after that at all? > > >> > > >> This kind of sounds a low priority item they just forgot to do and might have > > >> had an intention to follow through. > > > > > > Talking from my experience with other companies that could have done something > > > better I dare to say that this entire buzz for them is no-priority at all, like > > > "no money stuff", hence no attention given. That said, I believe ping won't > > > change anything here, however I agree that it _was_ worth to try to acquire any > > > response from them. > > > > Basically what Andy says above. > > > > Note that Dell's client team has been on the Cc for all the versions of > > this patch-set many of which were posted after the "We are working on it." email. > > > > For completeness sake I have just send a request for a status update on > > this to Prasanth and David from Dell. > > > > In the mean time it would be good IMO to merge v11 of this patch, if we > > get useful info from Dell after all we can modify the driver for this > > later. > > No, this change should not be taken at all. This change has a chance to > break booting or brick future dell devices. I'm not going to discuss it > again, but saying that it is good just because you do not have anything > better is not an argument to take such change. Also it is not an excuse > to hide dangerous things behind module parameter. And if you have been > doing to everything to ensure that companies would not want to tak with > you then sorry it is only your problem, so please do not complain here. I'd appreciate if you would refrain on making such personal attacks. We cannot force other people to answer, be they volunteers or employees of a company.
On Mon, 13 Jan 2025, Andy Shevchenko wrote: > On Mon, Jan 13, 2025 at 9:47 PM Pali Rohár <pali@kernel.org> wrote: > > On Monday 13 January 2025 17:49:19 Hans de Goede wrote: > > > On 13-Jan-25 4:36 PM, Andy Shevchenko wrote: > > > > On Mon, Jan 13, 2025 at 05:17:43PM +0200, Ilpo Järvinen wrote: > > > >> On Sat, 21 Dec 2024, Hans de Goede wrote: > > > >>> On 17-Dec-24 5:48 PM, Ilpo Järvinen wrote: > > > >>>> On Mon, 9 Dec 2024, Hans de Goede wrote: > > ... > > > > >>>> So what was the result of the private inquiry to Dell? > > > >>> > > > >>> On July 5th I send the following email to Prasanth Ksr > > > >>> <prasanth.ksr@dell.com> which is the only dell.com address I could > > > >>> find in MAINTAINERS other then Dell.Client.Kernel@dell.com which > > > >>> does not seem to be monitored very actively: > > > >>> > > > >>> """ > > > >>> Hello Prasanth, > > > >>> > > > >>> I'm contacting you about a question lis3lv02d freelfall sensors / > > > >>> accelerometers used on many (older) Dell laptop models. There > > > >>> has been a question about this last December and a patch-set > > > >>> trying to address part of this with Dell.Client.Kernel@dell.com > > > >>> in the Cc but no-one seems to be responding to that email address > > > >>> which is why I'm contacting you directly: > > > >>> > > > >>> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/ > > > >>> https://lore.kernel.org/platform-driver-x86/20240704125643.22946-1-hdegoede@redhat.com/ > > > >>> > > > >>> If you are not the right person to ask these questions to, then > > > >>> please forward this email to the right person. > > > >>> > > > >>> The lis3lv02d sensors are I2C devices and are described in the ACPI > > > >>> tables with an SMO88xx ACPI device node. The problem is that these > > > >>> ACPI device nodes do not have an ACPI I2cResouce in there resource > > > >>> (_CRS) list, so the I2C address of the sensor is unknown. > > > >>> > > > >>> When support was first added for these Dell provided a list of > > > >>> model-name to I2C address mappings for the then current generation > > > >>> of laptops, see: > > > >>> > > > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > > > >>> > > > >>> And later the community added a few more mappings. > > > >>> > > > >>> Paul Menzel, the author of the email starting the discussion on this: > > > >>> > > > >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/busses/i2c-i801.c#n1227 > > > >>> > > > >>> did a search for the kernel message which is printed when an SMO88xx > > > >>> ACPI device is found but the i2c-address is unknown and Paul found > > > >>> many models are missing from the mapping table (see Paul's email). > > > >>> > > > >>> Which leads us to the following questions: > > > >>> > > > >>> 1. Is there another, uniform (so not using a model name table) > > > >>> way to find out the I2C address of the SMO88xx freefall sensor > > > >>> from the ACPI or SMBIOS tables ? > > > >>> > > > >>> 2. If we need to keep using the model-name to I2C-address mapping > > > >>> table can you help us complete it by providing the sensor's I2C > > > >>> address for all models Paul has found where this is currently missing ? > > > >>> > > > >>> Regards, > > > >>> > > > >>> Hans > > > >>> """ > > > >>> > > > >>> Pali and Paul Menzel where in the Cc of this email. > > > >>> > > > >>>> Did they respond? > > > >>> > > > >>> I got a reply from Prasanth that they would forward my request to the > > > >>> correct team. Then I got on off-list reply to the v6 patch-set from > > > >>> David Wang from Dell with as relevant content "We are working on it." > > > >>> > > > >>>> Did they provide useful info? > > > >>> > > > >>> No further info was received after the "We are working on it." email. > > > >> > > > >> Hi Hans, > > > >> > > > >> So you didn't try to remind them after that at all? > > > >> > > > >> This kind of sounds a low priority item they just forgot to do and might have > > > >> had an intention to follow through. > > > > > > > > Talking from my experience with other companies that could have done something > > > > better I dare to say that this entire buzz for them is no-priority at all, like > > > > "no money stuff", hence no attention given. That said, I believe ping won't > > > > change anything here, however I agree that it _was_ worth to try to acquire any > > > > response from them. > > > > > > Basically what Andy says above. > > > > > > Note that Dell's client team has been on the Cc for all the versions of > > > this patch-set many of which were posted after the "We are working on it." email. > > > > > > For completeness sake I have just send a request for a status update on > > > this to Prasanth and David from Dell. > > > > > > In the mean time it would be good IMO to merge v11 of this patch, if we > > > get useful info from Dell after all we can modify the driver for this > > > later. > > > > No, this change should not be taken at all. This change has a chance to > > break booting or brick future dell devices. I'm not going to discuss it > > again, but saying that it is good just because you do not have anything > > better is not an argument to take such change. Also it is not an excuse > > to hide dangerous things behind module parameter. And if you have been > > doing to everything to ensure that companies would not want to tak with > > you then sorry it is only your problem, so please do not complain here. > > With all respect, this is not how we should treat the Linux kernel > contributors and users (who want this feature to enable their > devices). We have a ton of dangerous and DANGEROUS parameters and > other algorithms here and there (in Linux kernel source tree), I agree that more dangerous things pre-exist within the kernel already, and the danger seem exaggrated. In any case, it is behind a parameter requiring conscious decision to enable and even if enabled, it looks low risk. It seems Pali too expects them to not answer so I don't see much point in waiting as all are in agreement the best solution is not going to be available despite Hans' attempts to get the necessary info out from Dell. Thus, I'll take this patch into review-ilpo-next branch now.
diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c index d2b34e10c5eb..8d9dc59c7d8c 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,39 @@ 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; + + dev_info(&adap->dev, "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: + dev_warn(&adap->dev, "Unknown who-am-i register value 0x%02x\n", + smbus_data.byte); + return 0; /* Not found */ + } + + dev_dbg(&adap->dev, "Detected lis3lv02d on address 0x%02x\n", addr); + return 1; /* Found */ +} + static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap) { /* @@ -97,10 +132,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)) { dev_err(&adap->dev, "error %ld registering i2c_client\n", PTR_ERR(i2c_dev)); i2c_dev = NULL; @@ -169,12 +212,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 v8: - Use dev_err() / dev_dbg() where possible using &adap->dev as the device for logging 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 | 53 ++++++++++++++++++++-- 1 file changed, 49 insertions(+), 4 deletions(-)