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@xxxxxxxxxxxxx/ > >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx> > > > > So what was the result of the private inquiry to Dell? > > On July 5th I send the following email to Prasanth Ksr > <prasanth.ksr@xxxxxxxx> which is the only dell.com address I could > find in MAINTAINERS other then Dell.Client.Kernel@xxxxxxxx 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@xxxxxxxx > 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@xxxxxxxxxxxxx/ > https://lore.kernel.org/platform-driver-x86/20240704125643.22946-1-hdegoede@xxxxxxxxxx/ > > 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. -- i. > >> 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@xxxxxxxxxxxxxxx so that a quirk can be added\n", > addr); > > to address this. > > Regards, > > Hans > >
