| Message ID | 20231224213629.395741-7-hdegoede@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand |
On Sunday 24 December 2023 22:36:22 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. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> I would really like to hear Dell opinion about this change, and if there is really no way to get i2c address. Could you ask Dell people about it? Always it is better to use official / vendor provided steps of hardware detection, instead of inventing something new / own which would be there for a long time...
Hi Pali, On 12/24/23 23:07, Pali Rohár wrote: > On Sunday 24 December 2023 22:36:22 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. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > I would really like to hear Dell opinion about this change, and if there > is really no way to get i2c address. Could you ask Dell people about it? > Always it is better to use official / vendor provided steps of hardware > detection, instead of inventing something new / own which would be there > for a long time... Unfortunately I no longer have any contacts inside Dell for this and given Dell's non response in the original thread which started this I'm not hopefull for help from Dell here. Also there original reaction indicated that the info is not available in ACPI, so probing + extending the DMI match list seems to be the only way. Regards, Hans
On Friday 05 January 2024 17:36:03 Hans de Goede wrote: > Hi Pali, > > On 12/24/23 23:07, Pali Rohár wrote: > > On Sunday 24 December 2023 22:36:22 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. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > I would really like to hear Dell opinion about this change, and if there > > is really no way to get i2c address. Could you ask Dell people about it? > > Always it is better to use official / vendor provided steps of hardware > > detection, instead of inventing something new / own which would be there > > for a long time... > > Unfortunately I no longer have any contacts inside Dell for > this and given Dell's non response in the original thread > which started this I'm not hopefull for help from Dell here. Well, writing an email to hundred of receivers, or writing 10 or more emails at the same time is nowadays an example how to get your email into spam box in lot of companies. > Also there original reaction indicated that the info is not > available in ACPI, so probing + extending the DMI match > list seems to be the only way. I have verified this statement years ago and therefore it applies only for old models (about 10 years old). So using this statement is not valid for new models anymore. > Regards, > > Hans > >
Hi, On 1/5/24 19:51, Pali Rohár wrote: > On Friday 05 January 2024 17:36:03 Hans de Goede wrote: >> Hi Pali, >> >> On 12/24/23 23:07, Pali Rohár wrote: >>> On Sunday 24 December 2023 22:36:22 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. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> >>> I would really like to hear Dell opinion about this change, and if there >>> is really no way to get i2c address. Could you ask Dell people about it? >>> Always it is better to use official / vendor provided steps of hardware >>> detection, instead of inventing something new / own which would be there >>> for a long time... >> >> Unfortunately I no longer have any contacts inside Dell for >> this and given Dell's non response in the original thread >> which started this I'm not hopefull for help from Dell here. > > Well, writing an email to hundred of receivers, or writing 10 or more > emails at the same time is nowadays an example how to get your email > into spam box in lot of companies. > >> Also there original reaction indicated that the info is not >> available in ACPI, so probing + extending the DMI match >> list seems to be the only way. > > I have verified this statement years ago and therefore it applies only > for old models (about 10 years old). So using this statement is not > valid for new models anymore. The latest Dell model which I have which still has a SM088xx ACPI device is a Dell XPS 15 9550 from ~2016 this one still only has an interrupt resource for the SMO8810 ACPI device and no I2cSerialBus[V2] resource. On my Dell XPS 15 9550 the SMO8810 ACPI device is disabled, still the accelerometer is actually there, but the IRQ is not setup by the BIOS. So it can only work as an accelerometer and not for /dev/freefall, likely because my model has the big battery which covers the room for a 2.5" sata disk, so the freefall function is disabled in the BIOS. I have a patch series to still make the accelerometer work on the XPS 15 9550 even though it is disabled in the BIOS, but first lets get this series merged and then I'll send the XPS 15 9550 series to be applied on top later. Regards, Hans
Dear Greg, Am 05.01.24 um 17:36 schrieb Hans de Goede: > On 12/24/23 23:07, Pali Rohár wrote: >> On Sunday 24 December 2023 22:36:22 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 models >>> 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. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> I would really like to hear Dell opinion about this change, and if there >> is really no way to get i2c address. Could you ask Dell people about it? >> Always it is better to use official / vendor provided steps of hardware >> detection, instead of inventing something new / own which would be there >> for a long time... > > Unfortunately I no longer have any contacts inside Dell for > this and given Dell's non response in the original thread > which started this I'm not hopeful for help from Dell here. Unfortunately, since Mario Limonciello left Dell and works at AMD now, despite adding Dell.Client.Kernel@dell.com to Cc: I never received a reply from them. Do you have any contacts? (Dell ships Ubuntu on the “developer machines”, but I could never figure out, how the relationship works. At least the Dell support always said, GNU/Linux support is provided by “the community”.) Kind regards, Paul
On Mon, Jan 08, 2024 at 02:22:09PM +0100, Paul Menzel wrote: > Dear Greg, > > > Am 05.01.24 um 17:36 schrieb Hans de Goede: > > > On 12/24/23 23:07, Pali Rohár wrote: > > > On Sunday 24 December 2023 22:36:22 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 models > > > > 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. > > > > > > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > > > I would really like to hear Dell opinion about this change, and if there > > > is really no way to get i2c address. Could you ask Dell people about it? > > > Always it is better to use official / vendor provided steps of hardware > > > detection, instead of inventing something new / own which would be there > > > for a long time... > > > > Unfortunately I no longer have any contacts inside Dell for > > this and given Dell's non response in the original thread > > which started this I'm not hopeful for help from Dell here. > > Unfortunately, since Mario Limonciello left Dell and works at AMD now, > despite adding Dell.Client.Kernel@dell.com to Cc: I never received a reply > from them. Do you have any contacts? I do not, sorry.
diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c index bb1d3e439761..60ce2695ce01 100644 --- a/drivers/platform/x86/dell/dell-smo8800.c +++ b/drivers/platform/x86/dell/dell-smo8800.c @@ -29,6 +29,10 @@ static bool use_misc_lis3lv02d; module_param(use_misc_lis3lv02d, bool, 0444); MODULE_PARM_DESC(use_misc_lis3lv02d, "Use /dev/freefall chardev + evdev joystick emulation instead of iio accel driver"); +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"); + struct smo8800_device { u32 irq; /* acpi device irq */ atomic_t counter; /* count after last read */ @@ -146,6 +150,82 @@ static int smo8800_find_i801(struct device *dev, void *data) return 1; } +/* + * This is the kernel version of the single register device sanity checks from + * the i2c_safety_check function from lm_sensors sensor-detect script: + * This is meant to prevent access to 1-register-only devices, + * which are designed to be accessed with SMBus receive byte and SMBus send + * byte transactions (i.e. short reads and short writes) and treat SMBus + * read byte as a real write followed by a read. The device detection + * routines would write random values to the chip with possibly very nasty + * results for the hardware. Note that this function won't catch all such + * chips, as it assumes that reads and writes relate to the same register, + * but that's the best we can do. + */ +static int i2c_safety_check(struct smo8800_device *smo8800, struct i2c_adapter *adap, u8 addr) +{ + union i2c_smbus_data smbus_data; + int err; + u8 data; + + /* + * First receive a byte from the chip, and remember it. This + * also checks if there is a device at the address at all. + */ + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, + I2C_SMBUS_BYTE, &smbus_data); + if (err < 0) + return err; + + data = smbus_data.byte; + + /* + * Receive a byte again; very likely to be the same for + * 1-register-only devices. + */ + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0, + I2C_SMBUS_BYTE, &smbus_data); + if (err < 0) + return err; + + if (smbus_data.byte != data) + return 0; /* Not a 1-register-only device. */ + + /* + * Then try a standard byte read, with a register offset equal to + * the read byte; for 1-register-only device this should read + * the same byte value in return. + */ + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data, + I2C_SMBUS_BYTE_DATA, &smbus_data); + if (err < 0) + return err; + + if (smbus_data.byte != data) + return 0; /* Not a 1-register-only device. */ + + /* + * Then try a standard byte read, with a slightly different register + * offset; this should again read the register offset in return. + */ + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01, + I2C_SMBUS_BYTE_DATA, &smbus_data); + if (err < 0) + return err; + + if (smbus_data.byte != (data ^ 0x01)) + return 0; /* Not a 1-register-only device. */ + + /* + * Apparently this is a 1-register-only device, restore the original + * register value and leave it alone. + */ + i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data, + I2C_SMBUS_BYTE, NULL); + dev_warn(smo8800->dev, "I2C safety check for address 0x%02x failed, skipping\n", addr); + return -ENODEV; +} + /* * Set label to let iio-sensor-proxy know these freefall sensors are located in * the laptop base (not the display) and are not intended for screen rotation. @@ -161,12 +241,19 @@ const struct software_node smo8800_accel_node = { static int smo8800_detect_accel(struct smo8800_device *smo8800, struct i2c_adapter *adap, u8 addr, - struct i2c_board_info *info) + struct i2c_board_info *info, bool probe) { union i2c_smbus_data smbus_data; const char *type; int err; + if (probe) { + dev_info(smo8800->dev, "Probing for accelerometer on address 0x%02x\n", addr); + err = i2c_safety_check(smo8800, adap, addr); + if (err < 0) + return err; + } + err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I, I2C_SMBUS_BYTE_DATA, &smbus_data); if (err < 0) { @@ -253,17 +340,25 @@ static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800) } } - if (!addr) { + if (addr) { + /* Always detect the accel-type, this also checks the accel is actually there */ + err = smo8800_detect_accel(smo8800, adap, addr, &info, false); + if (err) + goto put_adapter; + } else if (probe_i2c_addr) { + /* First try address 0x29 (most used) and then try 0x1d */ + if (smo8800_detect_accel(smo8800, adap, 0x29, &info, true) != 0 && + smo8800_detect_accel(smo8800, adap, 0x1d, &info, true) != 0) { + dev_warn(smo8800->dev, "No accelerometer found\n"); + goto put_adapter; + } + } else { dev_warn(smo8800->dev, "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n"); + dev_info(smo8800->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n"); goto put_adapter; } - /* Always detect the accel-type, this also checks the accel is actually there */ - err = smo8800_detect_accel(smo8800, adap, addr, &info); - if (err) - goto put_adapter; - /* * If requested override detected type with "lis3lv02d" i2c_client_id, * for the old drivers/misc/lis3lv02d/lis3lv02d.c driver. @@ -281,6 +376,9 @@ static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800) } else { dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n", info.type, info.addr); + if (!addr) + dev_info(smo8800->dev, + "Please report this address upstream together with the output of 'cat /sys/class/dmi/id/product_name'\n"); } put_adapter: i2c_put_adapter(adap);
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. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/dell/dell-smo8800.c | 112 +++++++++++++++++++++-- 1 file changed, 105 insertions(+), 7 deletions(-)