[PATCH v9 0/4] i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d

[PATCH v9 0/4] i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d

[Hans de Goede]

Hi All,

Here is v9 of my patch series to move the manual instantation of lis3lv02d
i2c_client-s for SMO88xx ACPI device from the generic i2c-i801.c code to
a SMO88xx specific dell-lis3lv02d driver.

The i2c-core and i2c-i801 dependencies have both been merged into 6.13-rc1
so I believe that this series is ready to be merged now .

Patch 2/4 does still touch the i2c-i801 code removing the quirk code which
is moved to the pdx86 dell-smo8800 code. I think it would be best if Ilpo
prepares an immutable branch with this series to be merged into both
pdx86/for-next and the i2c-subsystem. Andi can we get your Ack for merging
the i2c-i801 changes through the pdx86 tree ?

Moving the i2c_client instantiation has the following advantages:

1. This moves the SMO88xx ACPI device quirk handling away from the generic
i2c-i801 module which is loaded on all Intel x86 machines to a module
which will only be loaded when there is an ACPI SMO88xx device.

2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
between the i2c-i801 and dell-smo8800 drivers.

3. This allows extending the quirk handling by adding new code and related
module parameters to the dell-lis3lv02d driver, without needing to modify
the i2c-i801 code.

This series also extends the i2c_client instantiation with support for
probing for the i2c-address of the lis3lv02d chip on devices which
are not yet listed in the DMI table with i2c-addresses for known models.
This probing is only done when requested through a module parameter.

Changes in v9:
- Rebase on top of v6.13-rc1
- Drop already merged i2c-core and i2c-i801 dependencies

Changes in v8:
- Address some minor review remarks from Andy

Changes in v7:
- Rebase on v6.11-rc1

Changes in v6:
- Use i2c_new_scanned_device() instead of re-inventing it

Changes in v5:
- Make match_acpi_device_ids() and match_acpi_device_ids[] __init[const]
- Add "Depends on I2C" to Kconfig (to fix kernel-test-robot reported issues)
- Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr)

Changes in v4:
- Move the i2c_client instantiation to a new dell-lis3lv02d driver instead
  of adding it to the dell-smo8800 driver
- Address a couple of other minor review comments

Changes in v3:
- Use an i2c bus notifier so that the i2c_client will still be instantiated if
  the i801 i2c_adapter shows up later or is re-probed (removed + added again).
  This addresses the main concern / review-comments made during review of v2.
- Add 2 prep patches to the i2c-core / the i2c-i801 driver to allow bus-notifier
  use / to avoid the need to duplicate the PCI-ids of IDF i2c-i801 adapters.
- Switch to standard dmi_system_id matching to check both sys-vendor +
  product-name DMI fields
- Drop the patch to alternatively use the st_accel IIO driver instead of
  drivers/misc/lis3lv02d/lis3lv02d.c

Changes in v2:
- Drop "[PATCH 1/6] platform/x86: dell-smo8800: Only load on Dell laptops"
- Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
- Add a comment documenting the IDF PCI device ids
- Keep using drivers/misc/lis3lv02d/lis3lv02d.c by default
- Rename the module-parameter to use_iio_driver which can be set to
  use the IIO st_accel driver instead
- Add a new patch adding the accelerometer address for the 2 models
  I have tested this on to dell_lis3lv02d_devices[]

Since this touches files under both drivers/i2c and drivers/platform/x86
some subsystem coordination is necessary. I think it would be best to just
merge the entire series through the i2c subsystem since this touches some
core i2c files. As pdx86 subsys co-maintainer I'm fine with doing so.

Regards,

Hans


Hans de Goede (4):
  platform/x86: dell-smo8800: Move SMO88xx acpi_device_ids to
    dell-smo8800-ids.h
  platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client
    from i2c-i801 to dell-lis3lv02d
  platform/x86: dell-smo8800: Add a couple more models to
    lis3lv02d_devices[]
  platform/x86: dell-smo8800: Add support for probing for the
    accelerometer i2c address

 drivers/i2c/busses/i2c-i801.c                | 124 ---------
 drivers/platform/x86/dell/Kconfig            |   1 +
 drivers/platform/x86/dell/Makefile           |   1 +
 drivers/platform/x86/dell/dell-lis3lv02d.c   | 252 +++++++++++++++++++
 drivers/platform/x86/dell/dell-smo8800-ids.h |  27 ++
 drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
 6 files changed, 282 insertions(+), 139 deletions(-)
 create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
 create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h

-- 
2.47.1

[PATCH v9 1/4] platform/x86: dell-smo8800: Move SMO88xx acpi_device_ids to dell-smo8800-ids.h

[Hans de Goede]

Move the SMO88xx acpi_device_ids to a new dell-smo8800-ids.h header,
so that these can be shared with the new dell-lis3lv02d code.

Reviewed-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v8:
- Add #include <linux/module.h> for MODULE_DEVICE_TABLE()

Changes in v4:
- This is a new patch in v3 of this patch-set
---
 drivers/platform/x86/dell/dell-smo8800-ids.h | 27 ++++++++++++++++++++
 drivers/platform/x86/dell/dell-smo8800.c     | 16 +-----------
 2 files changed, 28 insertions(+), 15 deletions(-)
 create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h

diff --git a/drivers/platform/x86/dell/dell-smo8800-ids.h b/drivers/platform/x86/dell/dell-smo8800-ids.h
new file mode 100644
index 000000000000..ec58e229ba7a
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-smo8800-ids.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ *  ACPI SMO88XX lis3lv02d freefall / accelerometer device-ids.
+ *
+ *  Copyright (C) 2012 Sonal Santan <sonal.santan@gmail.com>
+ *  Copyright (C) 2014 Pali Rohár <pali@kernel.org>
+ */
+#ifndef _DELL_SMO8800_IDS_H_
+#define _DELL_SMO8800_IDS_H_
+
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+
+static const struct acpi_device_id smo8800_ids[] = {
+	{ "SMO8800" },
+	{ "SMO8801" },
+	{ "SMO8810" },
+	{ "SMO8811" },
+	{ "SMO8820" },
+	{ "SMO8821" },
+	{ "SMO8830" },
+	{ "SMO8831" },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, smo8800_ids);
+
+#endif
diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 87fe03f23f24..8872f9b57fce 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -14,10 +14,10 @@
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
-#include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
+#include "dell-smo8800-ids.h"
 
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
@@ -163,20 +163,6 @@ static void smo8800_remove(struct platform_device *device)
 	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
 }
 
-/* NOTE: Keep this list in sync with drivers/i2c/busses/i2c-i801.c */
-static const struct acpi_device_id smo8800_ids[] = {
-	{ "SMO8800", 0 },
-	{ "SMO8801", 0 },
-	{ "SMO8810", 0 },
-	{ "SMO8811", 0 },
-	{ "SMO8820", 0 },
-	{ "SMO8821", 0 },
-	{ "SMO8830", 0 },
-	{ "SMO8831", 0 },
-	{ "", 0 },
-};
-MODULE_DEVICE_TABLE(acpi, smo8800_ids);
-
 static struct platform_driver smo8800_driver = {
 	.probe = smo8800_probe,
 	.remove = smo8800_remove,
-- 
2.47.1

[PATCH v9 2/4] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d

[Hans de Goede]

Various Dell laptops have an lis3lv02d freefall/accelerometer sensor.
The lis3lv02d chip has an interrupt line as well as an I2C connection to
the system's main SMBus.

The lis3lv02d is described in the ACPI tables by an SMO88xx ACPI device,
but the SMO88xx ACPI fwnodes are incomplete and only list an IRQ resource.

So far this has been worked around with some SMO88xx specific quirk code
in the generic i2c-i801 driver, but it is not necessary to handle the Dell
specific instantiation of i2c_client-s for SMO88xx ACPI devices there.

The kernel already instantiates platform_device-s for these with an
acpi:SMO88xx modalias. The drivers/platform/x86/dell/dell-smo8800.c
driver binds to this platform device but this only deals with
the interrupt resource. Add a drivers/platform/x86/dell/dell-lis3lv02d.c
which will matches on the same acpi:SMO88xx modaliases and move
the i2c_client instantiation from the generic i2c-i801 driver there.

Moving the i2c_client instantiation has the following advantages:

1. This moves the SMO88xx ACPI device quirk handling away from the generic
i2c-i801 module which is loaded on all Intel x86 machines to a module
which will only be loaded when there is an ACPI SMO88xx device.

2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
between the i2c-i801 and dell-smo8800 drivers.

3. This allows extending the quirk handling by adding new code and related
module parameters to the dell-lis3lv02d driver, without needing to modify
the i2c-i801 code.

Reviewed-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v8:
- Drop unnecessary ids helper vatiable from match_acpi_device_ids() helper
- Use dev_err() / dev_dbg() where possible using &adap->dev as the device
  for logging
- Add a comment about why bus_for_each_dev() and not i2c_for_each_dev()

Changes in v5:
- Make match_acpi_device_ids() and match_acpi_device_ids[] __init[const]
- Add "Depends on I2C" to Kconfig

Changes in v4:
- Move the i2c_client instantiation to a new dell-lis3lv02d driver instead
  of adding it to the dell-smo8800 driver

Changes in v3:
- Use an i2c bus notifier so that the i2c_client will still be instantiated if
  the i801 i2c_adapter shows up later or is re-probed (removed + added again)
- Switch to standard dmi_system_id matching to check both sys-vendor +
  product-name DMI fields
- Use unique i2c_adapter->name prefix for primary i2c_801 controller
  to avoid needing to duplicate PCI ids for extra IDF i2c_801 i2c_adapter-s
- Drop MODULE_SOFTDEP("pre: i2c-i801"), this is now no longer necessary
- Rebase on Torvalds master for recent additions of extra models in
  the dell_lis3lv02d_devices[] list

Changes in v2:
- Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
- Add a comment documenting the IDF PCI device ids
---
 drivers/i2c/busses/i2c-i801.c              | 124 -------------
 drivers/platform/x86/dell/Kconfig          |   1 +
 drivers/platform/x86/dell/Makefile         |   1 +
 drivers/platform/x86/dell/dell-lis3lv02d.c | 204 +++++++++++++++++++++
 4 files changed, 206 insertions(+), 124 deletions(-)
 create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 75dab01d43a7..b72712743cd7 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1162,127 +1162,6 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
 	}
 }
 
-/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */
-static const char *const acpi_smo8800_ids[] = {
-	"SMO8800",
-	"SMO8801",
-	"SMO8810",
-	"SMO8811",
-	"SMO8820",
-	"SMO8821",
-	"SMO8830",
-	"SMO8831",
-};
-
-static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
-					     u32 nesting_level,
-					     void *context,
-					     void **return_value)
-{
-	struct acpi_device_info *info;
-	acpi_status status;
-	char *hid;
-	int i;
-
-	status = acpi_get_object_info(obj_handle, &info);
-	if (ACPI_FAILURE(status))
-		return AE_OK;
-
-	if (!(info->valid & ACPI_VALID_HID))
-		goto smo88xx_not_found;
-
-	hid = info->hardware_id.string;
-	if (!hid)
-		goto smo88xx_not_found;
-
-	i = match_string(acpi_smo8800_ids, ARRAY_SIZE(acpi_smo8800_ids), hid);
-	if (i < 0)
-		goto smo88xx_not_found;
-
-	kfree(info);
-
-	*return_value = NULL;
-	return AE_CTRL_TERMINATE;
-
-smo88xx_not_found:
-	kfree(info);
-	return AE_OK;
-}
-
-static bool is_dell_system_with_lis3lv02d(void)
-{
-	void *err = ERR_PTR(-ENOENT);
-
-	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
-		return false;
-
-	/*
-	 * Check that ACPI device SMO88xx is present and is functioning.
-	 * Function acpi_get_devices() already filters all ACPI devices
-	 * which are not present or are not functioning.
-	 * ACPI device SMO88xx represents our ST microelectronics lis3lv02d
-	 * accelerometer but unfortunately ACPI does not provide any other
-	 * information (like I2C address).
-	 */
-	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
-
-	return !IS_ERR(err);
-}
-
-/*
- * Accelerometer's I2C address is not specified in DMI nor ACPI,
- * so it is needed to define mapping table based on DMI product names.
- */
-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 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;
-	}
-
-	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
-		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);
-}
-
 /* Register optional targets */
 static void i801_probe_optional_targets(struct i801_priv *priv)
 {
@@ -1302,9 +1181,6 @@ static void i801_probe_optional_targets(struct i801_priv *priv)
 	if (dmi_name_in_vendors("FUJITSU"))
 		dmi_walk(dmi_check_onboard_devices, &priv->adapter);
 
-	if (is_dell_system_with_lis3lv02d())
-		register_dell_lis3lv02d_i2c_device(priv);
-
 	/* Instantiate SPD EEPROMs unless the SMBus is multiplexed */
 #ifdef CONFIG_I2C_I801_MUX
 	if (!priv->mux_pdev)
diff --git a/drivers/platform/x86/dell/Kconfig b/drivers/platform/x86/dell/Kconfig
index 2dddafb3f7fa..d09060aedd3f 100644
--- a/drivers/platform/x86/dell/Kconfig
+++ b/drivers/platform/x86/dell/Kconfig
@@ -152,6 +152,7 @@ config DELL_SMBIOS_SMM
 config DELL_SMO8800
 	tristate "Dell Latitude freefall driver (ACPI SMO88XX)"
 	default m
+	depends on I2C
 	depends on ACPI || COMPILE_TEST
 	help
 	  Say Y here if you want to support SMO88XX freefall devices
diff --git a/drivers/platform/x86/dell/Makefile b/drivers/platform/x86/dell/Makefile
index 79d60f1bf4c1..bb3cbd470a46 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -15,6 +15,7 @@ dell-smbios-objs			:= dell-smbios-base.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_WMI)	+= dell-smbios-wmi.o
 dell-smbios-$(CONFIG_DELL_SMBIOS_SMM)	+= dell-smbios-smm.o
 obj-$(CONFIG_DELL_SMO8800)		+= dell-smo8800.o
+obj-$(CONFIG_DELL_SMO8800)		+= dell-lis3lv02d.o
 obj-$(CONFIG_DELL_UART_BACKLIGHT)	+= dell-uart-backlight.o
 obj-$(CONFIG_DELL_WMI)			+= dell-wmi.o
 dell-wmi-objs				:= dell-wmi-base.o
diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
new file mode 100644
index 000000000000..6dc04c89e6c9
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-lis3lv02d.c
@@ -0,0 +1,204 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * lis3lv02d i2c-client instantiation for ACPI SMO88xx devices without I2C resources.
+ *
+ *  Copyright (C) 2024 Hans de Goede <hansg@kernel.org>
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/device/bus.h>
+#include <linux/dmi.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/notifier.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+#include "dell-smo8800-ids.h"
+
+#define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr)                 \
+	{                                                                \
+		.matches = {                                             \
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Dell Inc."),    \
+			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, product_name), \
+		},                                                       \
+		.driver_data = (void *)(uintptr_t)(i2c_addr),            \
+	}
+
+/*
+ * Accelerometer's I2C address is not specified in DMI nor ACPI,
+ * so it is needed to define mapping table based on DMI product names.
+ */
+static const struct dmi_system_id lis3lv02d_devices[] __initconst = {
+	/*
+	 * Dell platform team told us that these Latitude devices have
+	 * ST microelectronics accelerometer at I2C address 0x29.
+	 */
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250",     0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450",     0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5550",     0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude E6440",     0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude E6440 ATG", 0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude E6540",     0x29),
+	/*
+	 * Additional individual entries were added after verification.
+	 */
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude 5480",      0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Precision 3540",     0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Vostro V131",        0x1d),
+	DELL_LIS3LV02D_DMI_ENTRY("Vostro 5568",        0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("XPS 15 7590",        0x29),
+	{ }
+};
+
+static u8 i2c_addr;
+static struct i2c_client *i2c_dev;
+static bool notifier_registered;
+
+static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
+{
+	/*
+	 * Only match the main I801 adapter and reject secondary adapters
+	 * which names start with "SMBus I801 IDF adapter".
+	 */
+	return strstarts(adap->name, "SMBus I801 adapter");
+}
+
+static int find_i801(struct device *dev, void *data)
+{
+	struct i2c_adapter *adap, **adap_ret = data;
+
+	adap = i2c_verify_adapter(dev);
+	if (!adap)
+		return 0;
+
+	if (!i2c_adapter_is_main_i801(adap))
+		return 0;
+
+	*adap_ret = i2c_get_adapter(adap->nr);
+	return 1;
+}
+
+static void instantiate_i2c_client(struct work_struct *work)
+{
+	struct i2c_board_info info = { };
+	struct i2c_adapter *adap = NULL;
+
+	if (i2c_dev)
+		return;
+
+	/*
+	 * bus_for_each_dev() and not i2c_for_each_dev() to avoid
+	 * a deadlock when find_i801() calls i2c_get_adapter().
+	 */
+	bus_for_each_dev(&i2c_bus_type, NULL, &adap, find_i801);
+	if (!adap)
+		return;
+
+	info.addr = i2c_addr;
+	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+
+	i2c_dev = i2c_new_client_device(adap, &info);
+	if (IS_ERR(i2c_dev)) {
+		dev_err(&adap->dev, "error %ld registering i2c_client\n", PTR_ERR(i2c_dev));
+		i2c_dev = NULL;
+	} else {
+		dev_dbg(&adap->dev, "registered lis3lv02d on address 0x%02x\n", info.addr);
+	}
+
+	i2c_put_adapter(adap);
+}
+static DECLARE_WORK(i2c_work, instantiate_i2c_client);
+
+static int i2c_bus_notify(struct notifier_block *nb, unsigned long action, void *data)
+{
+	struct device *dev = data;
+	struct i2c_client *client;
+	struct i2c_adapter *adap;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		adap = i2c_verify_adapter(dev);
+		if (!adap)
+			break;
+
+		if (i2c_adapter_is_main_i801(adap))
+			queue_work(system_long_wq, &i2c_work);
+		break;
+	case BUS_NOTIFY_REMOVED_DEVICE:
+		client = i2c_verify_client(dev);
+		if (!client)
+			break;
+
+		if (i2c_dev == client) {
+			dev_dbg(&client->adapter->dev, "lis3lv02d i2c_client removed\n");
+			i2c_dev = NULL;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+static struct notifier_block i2c_nb = { .notifier_call = i2c_bus_notify };
+
+static int __init match_acpi_device_ids(struct device *dev, const void *data)
+{
+	return acpi_match_device(data, dev) ? 1 : 0;
+}
+
+static int __init dell_lis3lv02d_init(void)
+{
+	const struct dmi_system_id *lis3lv02d_dmi_id;
+	struct device *dev;
+	int err;
+
+	/*
+	 * First check for a matching platform_device. This protects against
+	 * SMO88xx ACPI fwnodes which actually do have an I2C resource, which
+	 * will already have an i2c_client instantiated (not a platform_device).
+	 */
+	dev = bus_find_device(&platform_bus_type, NULL, smo8800_ids, match_acpi_device_ids);
+	if (!dev) {
+		pr_debug("No SMO88xx platform-device found\n");
+		return 0;
+	}
+	put_device(dev);
+
+	lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices);
+	if (!lis3lv02d_dmi_id) {
+		pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n");
+		return 0;
+	}
+
+	i2c_addr = (long)lis3lv02d_dmi_id->driver_data;
+
+	/*
+	 * Register i2c-bus notifier + queue initial scan for lis3lv02d
+	 * i2c_client instantiation.
+	 */
+	err = bus_register_notifier(&i2c_bus_type, &i2c_nb);
+	if (err)
+		return err;
+
+	notifier_registered = true;
+
+	queue_work(system_long_wq, &i2c_work);
+	return 0;
+}
+module_init(dell_lis3lv02d_init);
+
+static void __exit dell_lis3lv02d_module_exit(void)
+{
+	if (!notifier_registered)
+		return;
+
+	bus_unregister_notifier(&i2c_bus_type, &i2c_nb);
+	cancel_work_sync(&i2c_work);
+	i2c_unregister_device(i2c_dev);
+}
+module_exit(dell_lis3lv02d_module_exit);
+
+MODULE_DESCRIPTION("lis3lv02d i2c-client instantiation for ACPI SMO88xx devices");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_LICENSE("GPL");
-- 
2.47.1

[PATCH v9 3/4] platform/x86: dell-smo8800: Add a couple more models to lis3lv02d_devices[]

[Hans de Goede]

Add the accelerometer address for the following laptop models
to lis3lv02d_devices[]:

Dell Latitude E6330
Dell Latitude E6430
Dell XPS 15 9550

Tested-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Pali Rohár <pali@kernel.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell/dell-lis3lv02d.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
index 6dc04c89e6c9..d2b34e10c5eb 100644
--- a/drivers/platform/x86/dell/dell-lis3lv02d.c
+++ b/drivers/platform/x86/dell/dell-lis3lv02d.c
@@ -43,10 +43,13 @@ static const struct dmi_system_id lis3lv02d_devices[] __initconst = {
 	 * Additional individual entries were added after verification.
 	 */
 	DELL_LIS3LV02D_DMI_ENTRY("Latitude 5480",      0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude E6330",     0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("Latitude E6430",     0x29),
 	DELL_LIS3LV02D_DMI_ENTRY("Precision 3540",     0x29),
 	DELL_LIS3LV02D_DMI_ENTRY("Vostro V131",        0x1d),
 	DELL_LIS3LV02D_DMI_ENTRY("Vostro 5568",        0x29),
 	DELL_LIS3LV02D_DMI_ENTRY("XPS 15 7590",        0x29),
+	DELL_LIS3LV02D_DMI_ENTRY("XPS 15 9550",        0x29),
 	{ }
 };
 
-- 
2.47.1

[PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Hans de Goede]

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(-)

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
-- 
2.47.1

Re: [PATCH v9 0/4] i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d

[Pali Rohár]

All patches except the last one are reviewed and should be ready, that
it fine. The last patch is basically no-go, as it was discussed many
times, it should not be accepted at all. As Hans said that would not
respond to my emails, I'm also not going to discuss more with him.

On Monday 09 December 2024 19:35:53 Hans de Goede wrote:
> Hi All,
> 
> Here is v9 of my patch series to move the manual instantation of lis3lv02d
> i2c_client-s for SMO88xx ACPI device from the generic i2c-i801.c code to
> a SMO88xx specific dell-lis3lv02d driver.
> 
> The i2c-core and i2c-i801 dependencies have both been merged into 6.13-rc1
> so I believe that this series is ready to be merged now .
> 
> Patch 2/4 does still touch the i2c-i801 code removing the quirk code which
> is moved to the pdx86 dell-smo8800 code. I think it would be best if Ilpo
> prepares an immutable branch with this series to be merged into both
> pdx86/for-next and the i2c-subsystem. Andi can we get your Ack for merging
> the i2c-i801 changes through the pdx86 tree ?
> 
> Moving the i2c_client instantiation has the following advantages:
> 
> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
> i2c-i801 module which is loaded on all Intel x86 machines to a module
> which will only be loaded when there is an ACPI SMO88xx device.
> 
> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
> between the i2c-i801 and dell-smo8800 drivers.
> 
> 3. This allows extending the quirk handling by adding new code and related
> module parameters to the dell-lis3lv02d driver, without needing to modify
> the i2c-i801 code.
> 
> This series also extends the i2c_client instantiation with support for
> probing for the i2c-address of the lis3lv02d chip on devices which
> are not yet listed in the DMI table with i2c-addresses for known models.
> This probing is only done when requested through a module parameter.
> 
> Changes in v9:
> - Rebase on top of v6.13-rc1
> - Drop already merged i2c-core and i2c-i801 dependencies
> 
> Changes in v8:
> - Address some minor review remarks from Andy
> 
> Changes in v7:
> - Rebase on v6.11-rc1
> 
> Changes in v6:
> - Use i2c_new_scanned_device() instead of re-inventing it
> 
> Changes in v5:
> - Make match_acpi_device_ids() and match_acpi_device_ids[] __init[const]
> - Add "Depends on I2C" to Kconfig (to fix kernel-test-robot reported issues)
> - Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr)
> 
> Changes in v4:
> - Move the i2c_client instantiation to a new dell-lis3lv02d driver instead
>   of adding it to the dell-smo8800 driver
> - Address a couple of other minor review comments
> 
> Changes in v3:
> - Use an i2c bus notifier so that the i2c_client will still be instantiated if
>   the i801 i2c_adapter shows up later or is re-probed (removed + added again).
>   This addresses the main concern / review-comments made during review of v2.
> - Add 2 prep patches to the i2c-core / the i2c-i801 driver to allow bus-notifier
>   use / to avoid the need to duplicate the PCI-ids of IDF i2c-i801 adapters.
> - Switch to standard dmi_system_id matching to check both sys-vendor +
>   product-name DMI fields
> - Drop the patch to alternatively use the st_accel IIO driver instead of
>   drivers/misc/lis3lv02d/lis3lv02d.c
> 
> Changes in v2:
> - Drop "[PATCH 1/6] platform/x86: dell-smo8800: Only load on Dell laptops"
> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
> - Add a comment documenting the IDF PCI device ids
> - Keep using drivers/misc/lis3lv02d/lis3lv02d.c by default
> - Rename the module-parameter to use_iio_driver which can be set to
>   use the IIO st_accel driver instead
> - Add a new patch adding the accelerometer address for the 2 models
>   I have tested this on to dell_lis3lv02d_devices[]
> 
> Since this touches files under both drivers/i2c and drivers/platform/x86
> some subsystem coordination is necessary. I think it would be best to just
> merge the entire series through the i2c subsystem since this touches some
> core i2c files. As pdx86 subsys co-maintainer I'm fine with doing so.
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (4):
>   platform/x86: dell-smo8800: Move SMO88xx acpi_device_ids to
>     dell-smo8800-ids.h
>   platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client
>     from i2c-i801 to dell-lis3lv02d
>   platform/x86: dell-smo8800: Add a couple more models to
>     lis3lv02d_devices[]
>   platform/x86: dell-smo8800: Add support for probing for the
>     accelerometer i2c address
> 
>  drivers/i2c/busses/i2c-i801.c                | 124 ---------
>  drivers/platform/x86/dell/Kconfig            |   1 +
>  drivers/platform/x86/dell/Makefile           |   1 +
>  drivers/platform/x86/dell/dell-lis3lv02d.c   | 252 +++++++++++++++++++
>  drivers/platform/x86/dell/dell-smo8800-ids.h |  27 ++
>  drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
>  6 files changed, 282 insertions(+), 139 deletions(-)
>  create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
>  create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h
> 
> -- 
> 2.47.1
> 

Re: [PATCH v9 0/4] i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5846 bytes --]

On Mon, 9 Dec 2024, Pali Rohár wrote:

> All patches except the last one are reviewed and should be ready, that
> it fine. The last patch is basically no-go, as it was discussed many
> times, it should not be accepted at all. As Hans said that would not
> respond to my emails, I'm also not going to discuss more with him.

I could merge the non-controversial patches 1-3 through pdx86 tree. Would
a I2C drivers maintainer want to give an ack for the patch 2 whose diff is 
simply removing the code from I2C side?

(I'll have to lookup what's up with the last patch later when I've more 
time to go through archives.)

-- 
 i.


> On Monday 09 December 2024 19:35:53 Hans de Goede wrote:
> > Hi All,
> > 
> > Here is v9 of my patch series to move the manual instantation of lis3lv02d
> > i2c_client-s for SMO88xx ACPI device from the generic i2c-i801.c code to
> > a SMO88xx specific dell-lis3lv02d driver.
> > 
> > The i2c-core and i2c-i801 dependencies have both been merged into 6.13-rc1
> > so I believe that this series is ready to be merged now .
> > 
> > Patch 2/4 does still touch the i2c-i801 code removing the quirk code which
> > is moved to the pdx86 dell-smo8800 code. I think it would be best if Ilpo
> > prepares an immutable branch with this series to be merged into both
> > pdx86/for-next and the i2c-subsystem. Andi can we get your Ack for merging
> > the i2c-i801 changes through the pdx86 tree ?
> > 
> > Moving the i2c_client instantiation has the following advantages:
> > 
> > 1. This moves the SMO88xx ACPI device quirk handling away from the generic
> > i2c-i801 module which is loaded on all Intel x86 machines to a module
> > which will only be loaded when there is an ACPI SMO88xx device.
> > 
> > 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
> > between the i2c-i801 and dell-smo8800 drivers.
> > 
> > 3. This allows extending the quirk handling by adding new code and related
> > module parameters to the dell-lis3lv02d driver, without needing to modify
> > the i2c-i801 code.
> > 
> > This series also extends the i2c_client instantiation with support for
> > probing for the i2c-address of the lis3lv02d chip on devices which
> > are not yet listed in the DMI table with i2c-addresses for known models.
> > This probing is only done when requested through a module parameter.
> > 
> > Changes in v9:
> > - Rebase on top of v6.13-rc1
> > - Drop already merged i2c-core and i2c-i801 dependencies
> > 
> > Changes in v8:
> > - Address some minor review remarks from Andy
> > 
> > Changes in v7:
> > - Rebase on v6.11-rc1
> > 
> > Changes in v6:
> > - Use i2c_new_scanned_device() instead of re-inventing it
> > 
> > Changes in v5:
> > - Make match_acpi_device_ids() and match_acpi_device_ids[] __init[const]
> > - Add "Depends on I2C" to Kconfig (to fix kernel-test-robot reported issues)
> > - Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr)
> > 
> > Changes in v4:
> > - Move the i2c_client instantiation to a new dell-lis3lv02d driver instead
> >   of adding it to the dell-smo8800 driver
> > - Address a couple of other minor review comments
> > 
> > Changes in v3:
> > - Use an i2c bus notifier so that the i2c_client will still be instantiated if
> >   the i801 i2c_adapter shows up later or is re-probed (removed + added again).
> >   This addresses the main concern / review-comments made during review of v2.
> > - Add 2 prep patches to the i2c-core / the i2c-i801 driver to allow bus-notifier
> >   use / to avoid the need to duplicate the PCI-ids of IDF i2c-i801 adapters.
> > - Switch to standard dmi_system_id matching to check both sys-vendor +
> >   product-name DMI fields
> > - Drop the patch to alternatively use the st_accel IIO driver instead of
> >   drivers/misc/lis3lv02d/lis3lv02d.c
> > 
> > Changes in v2:
> > - Drop "[PATCH 1/6] platform/x86: dell-smo8800: Only load on Dell laptops"
> > - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
> > - Add a comment documenting the IDF PCI device ids
> > - Keep using drivers/misc/lis3lv02d/lis3lv02d.c by default
> > - Rename the module-parameter to use_iio_driver which can be set to
> >   use the IIO st_accel driver instead
> > - Add a new patch adding the accelerometer address for the 2 models
> >   I have tested this on to dell_lis3lv02d_devices[]
> > 
> > Since this touches files under both drivers/i2c and drivers/platform/x86
> > some subsystem coordination is necessary. I think it would be best to just
> > merge the entire series through the i2c subsystem since this touches some
> > core i2c files. As pdx86 subsys co-maintainer I'm fine with doing so.
> > 
> > Regards,
> > 
> > Hans
> > 
> > 
> > Hans de Goede (4):
> >   platform/x86: dell-smo8800: Move SMO88xx acpi_device_ids to
> >     dell-smo8800-ids.h
> >   platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client
> >     from i2c-i801 to dell-lis3lv02d
> >   platform/x86: dell-smo8800: Add a couple more models to
> >     lis3lv02d_devices[]
> >   platform/x86: dell-smo8800: Add support for probing for the
> >     accelerometer i2c address
> > 
> >  drivers/i2c/busses/i2c-i801.c                | 124 ---------
> >  drivers/platform/x86/dell/Kconfig            |   1 +
> >  drivers/platform/x86/dell/Makefile           |   1 +
> >  drivers/platform/x86/dell/dell-lis3lv02d.c   | 252 +++++++++++++++++++
> >  drivers/platform/x86/dell/dell-smo8800-ids.h |  27 ++
> >  drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
> >  6 files changed, 282 insertions(+), 139 deletions(-)
> >  create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
> >  create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h
> > 
> > -- 
> > 2.47.1
> > 
> 

Re: [PATCH v9 2/4] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1815 bytes --]

On Mon, Dec 09, 2024 at 07:35:55PM +0100, Hans de Goede wrote:
> Various Dell laptops have an lis3lv02d freefall/accelerometer sensor.
> The lis3lv02d chip has an interrupt line as well as an I2C connection to
> the system's main SMBus.
> 
> The lis3lv02d is described in the ACPI tables by an SMO88xx ACPI device,
> but the SMO88xx ACPI fwnodes are incomplete and only list an IRQ resource.
> 
> So far this has been worked around with some SMO88xx specific quirk code
> in the generic i2c-i801 driver, but it is not necessary to handle the Dell
> specific instantiation of i2c_client-s for SMO88xx ACPI devices there.
> 
> The kernel already instantiates platform_device-s for these with an
> acpi:SMO88xx modalias. The drivers/platform/x86/dell/dell-smo8800.c
> driver binds to this platform device but this only deals with
> the interrupt resource. Add a drivers/platform/x86/dell/dell-lis3lv02d.c
> which will matches on the same acpi:SMO88xx modaliases and move
> the i2c_client instantiation from the generic i2c-i801 driver there.
> 
> Moving the i2c_client instantiation has the following advantages:
> 
> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
> i2c-i801 module which is loaded on all Intel x86 machines to a module
> which will only be loaded when there is an ACPI SMO88xx device.
> 
> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
> between the i2c-i801 and dell-smo8800 drivers.
> 
> 3. This allows extending the quirk handling by adding new code and related
> module parameters to the dell-lis3lv02d driver, without needing to modify
> the i2c-i801 code.
> 
> Reviewed-by: Pali Rohár <pali@kernel.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Wolfram Sang <wsa@kernel.org>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v9 0/4] i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d

[Ilpo Järvinen]

On Mon, 9 Dec 2024, Hans de Goede wrote:

> Hi All,
> 
> Here is v9 of my patch series to move the manual instantation of lis3lv02d
> i2c_client-s for SMO88xx ACPI device from the generic i2c-i801.c code to
> a SMO88xx specific dell-lis3lv02d driver.
> 
> The i2c-core and i2c-i801 dependencies have both been merged into 6.13-rc1
> so I believe that this series is ready to be merged now .
> 
> Patch 2/4 does still touch the i2c-i801 code removing the quirk code which
> is moved to the pdx86 dell-smo8800 code. I think it would be best if Ilpo
> prepares an immutable branch with this series to be merged into both
> pdx86/for-next and the i2c-subsystem. Andi can we get your Ack for merging
> the i2c-i801 changes through the pdx86 tree ?
> 
> Moving the i2c_client instantiation has the following advantages:
> 
> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
> i2c-i801 module which is loaded on all Intel x86 machines to a module
> which will only be loaded when there is an ACPI SMO88xx device.
> 
> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
> between the i2c-i801 and dell-smo8800 drivers.
> 
> 3. This allows extending the quirk handling by adding new code and related
> module parameters to the dell-lis3lv02d driver, without needing to modify
> the i2c-i801 code.
> 
> This series also extends the i2c_client instantiation with support for
> probing for the i2c-address of the lis3lv02d chip on devices which
> are not yet listed in the DMI table with i2c-addresses for known models.
> This probing is only done when requested through a module parameter.
> 
> Changes in v9:
> - Rebase on top of v6.13-rc1
> - Drop already merged i2c-core and i2c-i801 dependencies
> 
> Changes in v8:
> - Address some minor review remarks from Andy
> 
> Changes in v7:
> - Rebase on v6.11-rc1
> 
> Changes in v6:
> - Use i2c_new_scanned_device() instead of re-inventing it
> 
> Changes in v5:
> - Make match_acpi_device_ids() and match_acpi_device_ids[] __init[const]
> - Add "Depends on I2C" to Kconfig (to fix kernel-test-robot reported issues)
> - Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr)
> 
> Changes in v4:
> - Move the i2c_client instantiation to a new dell-lis3lv02d driver instead
>   of adding it to the dell-smo8800 driver
> - Address a couple of other minor review comments
> 
> Changes in v3:
> - Use an i2c bus notifier so that the i2c_client will still be instantiated if
>   the i801 i2c_adapter shows up later or is re-probed (removed + added again).
>   This addresses the main concern / review-comments made during review of v2.
> - Add 2 prep patches to the i2c-core / the i2c-i801 driver to allow bus-notifier
>   use / to avoid the need to duplicate the PCI-ids of IDF i2c-i801 adapters.
> - Switch to standard dmi_system_id matching to check both sys-vendor +
>   product-name DMI fields
> - Drop the patch to alternatively use the st_accel IIO driver instead of
>   drivers/misc/lis3lv02d/lis3lv02d.c
> 
> Changes in v2:
> - Drop "[PATCH 1/6] platform/x86: dell-smo8800: Only load on Dell laptops"
> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
> - Add a comment documenting the IDF PCI device ids
> - Keep using drivers/misc/lis3lv02d/lis3lv02d.c by default
> - Rename the module-parameter to use_iio_driver which can be set to
>   use the IIO st_accel driver instead
> - Add a new patch adding the accelerometer address for the 2 models
>   I have tested this on to dell_lis3lv02d_devices[]
> 
> Since this touches files under both drivers/i2c and drivers/platform/x86
> some subsystem coordination is necessary. I think it would be best to just
> merge the entire series through the i2c subsystem since this touches some
> core i2c files. As pdx86 subsys co-maintainer I'm fine with doing so.
> 
> Regards,
> 
> Hans
> 
> 
> Hans de Goede (4):
>   platform/x86: dell-smo8800: Move SMO88xx acpi_device_ids to
>     dell-smo8800-ids.h
>   platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client
>     from i2c-i801 to dell-lis3lv02d
>   platform/x86: dell-smo8800: Add a couple more models to
>     lis3lv02d_devices[]
>   platform/x86: dell-smo8800: Add support for probing for the
>     accelerometer i2c address
> 
>  drivers/i2c/busses/i2c-i801.c                | 124 ---------
>  drivers/platform/x86/dell/Kconfig            |   1 +
>  drivers/platform/x86/dell/Makefile           |   1 +
>  drivers/platform/x86/dell/dell-lis3lv02d.c   | 252 +++++++++++++++++++
>  drivers/platform/x86/dell/dell-smo8800-ids.h |  27 ++
>  drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
>  6 files changed, 282 insertions(+), 139 deletions(-)
>  create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
>  create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h

Patches 1-3 applied into review-ilpo-next branch.

-- 
 i.

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Ilpo Järvinen]

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?

-- 
 i.

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Hans de Goede]

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

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 8743 bytes --]

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.

-- 
 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@vger.kernel.org so that a quirk can be added\n",
> 		 addr);
> 
> to address this.
> 
> Regards,
> 
> Hans
> 
> 

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Andy Shevchenko]

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.

Just my 2c.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Hans de Goede]

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

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Pali Rohár]

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.

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Andy Shevchenko]

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!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5554 bytes --]

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.

-- 
 i.

Re: [PATCH v9 4/4] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 6462 bytes --]

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.

-- 
 i.