| Message ID | cb2e65f8-066a-41ea-ae3b-03950714f33c@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFT] i2c: i801: Scan for Dell accelerometer i2c address | expand |
On Friday 03 May 2024 16:09:03 Heiner Kallweit wrote: > So far each new Dell device with an accelerometer requires a patch. > All devices, with one older system as an exception, use address 0x29. > So I think we can safely scan for the correct address This is too risky. Poking random smbus address can lead to the lockup or crash of the whole system. I really dislike such change which is going to unconditionally on every system to scan or access fixed smbus address. Crashing kernel on some new model in future or on some older models is the worst user experience which can happen. The hardcoded table with model name and address is the safest option to prevent crashes or other unexpected behavior. Instead, I would suggest to contact Dell if they can provide a way to read accelerometer's i2c/smbus address from ACPI/WMI/DMI. And if there is not any way right now then ask Dell to include it for new products. So we can avoid having hardcoded table for new products released in future.
On 03.05.2024 16:39, Pali Rohár wrote: > On Friday 03 May 2024 16:09:03 Heiner Kallweit wrote: >> So far each new Dell device with an accelerometer requires a patch. >> All devices, with one older system as an exception, use address 0x29. >> So I think we can safely scan for the correct address > > This is too risky. Poking random smbus address can lead to the lockup or > crash of the whole system. I really dislike such change which is going > to unconditionally on every system to scan or access fixed smbus address. > Crashing kernel on some new model in future or on some older models is > the worst user experience which can happen. > The scan is protected by is_dell_system_with_lis3lv02d(). So we know it's a Dell system with an accelerometer. The only potentially problematic scenario would be: - It's a Dell system with an accelerometer and - Accelerometer is at an address different from 0x29 and - System has some other device at address 0x29 If there ever should be such a case, we still would have the option to provide a more specific probe function to i2c_new_scanned_device(). At least for now I don't really see a risk. > The hardcoded table with model name and address is the safest option to > prevent crashes or other unexpected behavior. > > Instead, I would suggest to contact Dell if they can provide a way to > read accelerometer's i2c/smbus address from ACPI/WMI/DMI. And if there See comment in the driver: "Accelerometer's I2C address is not specified in DMI nor ACPI" > is not any way right now then ask Dell to include it for new products. > So we can avoid having hardcoded table for new products released in > future. This is a good point. Not sure whether anybody tried this yet. Therefore +Dell.Client.Kernel@dell.com
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 5ff7ab08d..a8e3a21d9 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1217,57 +1217,20 @@ static bool is_dell_system_with_lis3lv02d(void) } /* - * Accelerometer's I2C address is not specified in DMI nor ACPI, - * so it is needed to define mapping table based on DMI product names. + * Accelerometer's I2C address is not specified in DMI nor ACPI. + * With one exception all systems use address 0x29. */ -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 }, - { "Precision 3540", 0x29 }, - { "Vostro V131", 0x1d }, - { "Vostro 5568", 0x29 }, - { "XPS 15 7590", 0x29 }, -}; +static const unsigned short dell_lis3lv02d_addr_list[] = { 0x29, 0x1d, I2C_CLIENT_END }; static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv) { - struct i2c_board_info info; - const char *dmi_product_name; - int i; - - dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); - for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { - if (strcmp(dmi_product_name, - dell_lis3lv02d_devices[i].dmi_product_name) == 0) - break; - } + struct i2c_board_info info = { .type = "lis3lv02d" }; + struct i2c_client *cl; - if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) { + cl = i2c_new_scanned_device(&priv->adapter, &info, dell_lis3lv02d_addr_list, NULL); + if (IS_ERR(cl)) dev_warn(&priv->pci_dev->dev, - "Accelerometer lis3lv02d is present on SMBus but its" - " address is unknown, skipping registration\n"); - return; - } - - memset(&info, 0, sizeof(struct i2c_board_info)); - info.addr = dell_lis3lv02d_devices[i].i2c_addr; - strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE); - i2c_new_client_device(&priv->adapter, &info); + "Accelerometer is present, but couldn't be registered at any known address\n"); } /* Register optional slaves */
So far each new Dell device with an accelerometer requires a patch. All devices, with one older system as an exception, use address 0x29. So I think we can safely scan for the correct address, and avoid the need for a patch per device. Last but not least this allows to significantly simplify the code. I don't have such a Dell system, therefore patch is compile-tested only. If you have a Dell system with accelerometer, testing the patch would be appreciated. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 53 ++++++----------------------------- 1 file changed, 8 insertions(+), 45 deletions(-)