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

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

[Hans de Goede]

Hi All,

Here is v4 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.

Moving the i2c_client instantiation there 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.

The probing support adds quite a bit of code which shows why it is good
to move the handling out of the generic i2c-i801 code.

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 (6):
  i2c: core: Setup i2c_adapter runtime-pm before calling device_add()
  i2c: i801: Use a different adapter-name for IDF adapters
  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                | 133 +-------
 drivers/i2c/i2c-core-base.c                  |  18 +-
 drivers/platform/x86/dell/Makefile           |   1 +
 drivers/platform/x86/dell/dell-lis3lv02d.c   | 331 +++++++++++++++++++
 drivers/platform/x86/dell/dell-smo8800-ids.h |  26 ++
 drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
 6 files changed, 379 insertions(+), 146 deletions(-)
 create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
 create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h

-- 
2.45.1

[PATCH v4 1/6] i2c: core: Setup i2c_adapter runtime-pm before calling device_add()

[Hans de Goede]

Platform glue code, which is not build into the kernel and thus cannot
use i2c_register_board_info() may want to use bus_register_notifier()
to listen for i2c-adapters to show up on which the platform code needs
to manually instantiate platform specific i2c_clients.

This results in calling i2c_new_client_device() from the bus notifier
which happens near the device_add() call.

If the i2c-core has not yet setup runtime-pm (specifically the
no-callbacks and ignore-children flags) for the device embedded
inside struct i2c_adapter and the driver for the i2c_client
calls pm_runtime_set_active() this will trigger the following
error inside __pm_runtime_set_status():

"runtime PM trying to activate child device %s but parent (%s) is not active\n"

and the i2c_client's runtime-status will not be updated.

Split the device_register() call for the adapter into device_initialize()
and device_add() and move the pm-runtime init calls inbetween these 2 calls
so that the runtime-status can be correctly set when a driver binds from
the bus-notifier.

Note the moved pm-runtime init calls just override the initial value of
some flags in struct device set by device_initialize() and calling these
before device_add() is safe.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Add a comment explaining why runtime-pm needs to be setup before
  the device_add()

Changes in v3:
- This is a new patch in v3 of this patch-set
---
 drivers/i2c/i2c-core-base.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index db0d1ac82910..c4517d97a49f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1521,7 +1521,18 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	dev_set_name(&adap->dev, "i2c-%d", adap->nr);
 	adap->dev.bus = &i2c_bus_type;
 	adap->dev.type = &i2c_adapter_type;
-	res = device_register(&adap->dev);
+	device_initialize(&adap->dev);
+
+	/*
+	 * This adapter can be used as a parent immediately after device_add(),
+	 * setup runtime-pm (especially ignore-children) before hand.
+	 */
+	device_enable_async_suspend(&adap->dev);
+	pm_runtime_no_callbacks(&adap->dev);
+	pm_suspend_ignore_children(&adap->dev, true);
+	pm_runtime_enable(&adap->dev);
+
+	res = device_add(&adap->dev);
 	if (res) {
 		pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
 		goto out_list;
@@ -1533,11 +1544,6 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 	if (res)
 		goto out_reg;
 
-	device_enable_async_suspend(&adap->dev);
-	pm_runtime_no_callbacks(&adap->dev);
-	pm_suspend_ignore_children(&adap->dev, true);
-	pm_runtime_enable(&adap->dev);
-
 	res = i2c_init_recovery(adap);
 	if (res == -EPROBE_DEFER)
 		goto out_reg;
-- 
2.45.1

[PATCH v4 2/6] i2c: i801: Use a different adapter-name for IDF adapters

[Hans de Goede]

On chipsets with a second 'Integrated Device Function' SMBus controller use
a different adapter-name for the second IDF adapter.

This allows platform glue code which is looking for the primary i801
adapter to manually instantiate i2c_clients on to differentiate
between the 2.

This allows such code to find the primary i801 adapter by name, without
needing to duplicate the PCI-ids to feature-flags mapping from i2c-i801.c.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- Use a single snprintf() with a conditional argument for the 2 names
- Add a comment that the adapter-name is used by platform code

Changes in v3:
- This is a new patch in v3 of this patch-set
---
 drivers/i2c/busses/i2c-i801.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index d2d2a6dbe29f..94265ee300c0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1760,8 +1760,15 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	i801_add_tco(priv);
 
+	/*
+	 * adapter.name is used by platform code to find the main I801 adapter
+	 * to instantiante i2c_clients, do not change.
+	 */
 	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
-		"SMBus I801 adapter at %04lx", priv->smba);
+		 "SMBus %s adapter at %04lx",
+		 (priv->features & FEATURE_IDF) ? "I801 IDF" : "I801",
+		 priv->smba);
+
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		platform_device_unregister(priv->tco_pdev);
-- 
2.45.1

[PATCH v4 3/6] 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
- This is a new patch in v3 of this patch-set
---
 drivers/platform/x86/dell/dell-smo8800-ids.h | 26 ++++++++++++++++++++
 drivers/platform/x86/dell/dell-smo8800.c     | 16 +-----------
 2 files changed, 27 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..f85d8b707d85
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-smo8800-ids.h
@@ -0,0 +1,26 @@
+/* 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>
+
+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 f7ec17c56833..f9119ed2bd92 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_new = smo8800_remove,
-- 
2.45.1

[PATCH v4 4/6] 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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/Makefile         |   1 +
 drivers/platform/x86/dell/dell-lis3lv02d.c | 199 +++++++++++++++++++++
 3 files changed, 200 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 94265ee300c0..375781079e0d 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1153,127 +1153,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 slaves */
 static void i801_probe_optional_slaves(struct i801_priv *priv)
 {
@@ -1293,9 +1172,6 @@ static void i801_probe_optional_slaves(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/Makefile b/drivers/platform/x86/dell/Makefile
index 8176a257d9c3..970409c107b0 100644
--- a/drivers/platform/x86/dell/Makefile
+++ b/drivers/platform/x86/dell/Makefile
@@ -14,6 +14,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..e581b8e2a603
--- /dev/null
+++ b/drivers/platform/x86/dell/dell-lis3lv02d.c
@@ -0,0 +1,199 @@
+// 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[] = {
+	/*
+	 * 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 const struct dmi_system_id *lis3lv02d_dmi_id;
+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(&i2c_bus_type, NULL, &adap, find_i801);
+	if (!adap)
+		return;
+
+	info.addr = (long)lis3lv02d_dmi_id->driver_data;
+	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+
+	i2c_dev = i2c_new_client_device(adap, &info);
+	if (IS_ERR(i2c_dev)) {
+		pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev));
+		i2c_dev = NULL;
+	} else {
+		pr_debug("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) {
+			pr_debug("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 match_acpi_device_ids(struct device *dev, const void *data)
+{
+	const struct acpi_device_id *ids = data;
+
+	return acpi_match_device(ids, dev) ? 1 : 0;
+}
+
+static int __init dell_lis3lv02d_init(void)
+{
+	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;
+	}
+
+	/*
+	 * 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.45.1

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

[Pali Rohár]

On Monday 24 June 2024 13:15:16 Hans de Goede wrote:
> +static int match_acpi_device_ids(struct device *dev, const void *data)
> +{

You can mark this function as __init as it is called only from
dell_lis3lv02d_init to free space.

> +	const struct acpi_device_id *ids = data;
> +
> +	return acpi_match_device(ids, dev) ? 1 : 0;
> +}
> +
> +static int __init dell_lis3lv02d_init(void)
> +{
> +	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;

Is zero return value expected? Should not be it something like -ENODEV?

> +	}
> +	put_device(dev);
> +
> +	lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices);
> +	if (!lis3lv02d_dmi_id) {

You can cache the value lis3lv02d_dmi_id->driver_data instead of caching
lis3lv02d_dmi_id pointer and then you can mark lis3lv02d_devices array
as __initconst to free additional space not needed at runtime on x86
machines without accelerometer where CONFIG_DELL_SMO8800=y.

> +		pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * 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.45.1
> 

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

[Hans de Goede]

Hi,

On 6/24/24 8:14 PM, Pali Rohár wrote:
> On Monday 24 June 2024 13:15:16 Hans de Goede wrote:
>> +static int match_acpi_device_ids(struct device *dev, const void *data)
>> +{
> 
> You can mark this function as __init as it is called only from
> dell_lis3lv02d_init to free space.
> 
>> +	const struct acpi_device_id *ids = data;
>> +
>> +	return acpi_match_device(ids, dev) ? 1 : 0;
>> +}
>> +
>> +static int __init dell_lis3lv02d_init(void)
>> +{
>> +	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;
> 
> Is zero return value expected? Should not be it something like -ENODEV?

This is a module_init() function returning non 0 leads to an error getting
logged and the modprobe command to return a non 0 value which is not what
we want.

>> +	}
>> +	put_device(dev);
>> +
>> +	lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices);
>> +	if (!lis3lv02d_dmi_id) {
> 
> You can cache the value lis3lv02d_dmi_id->driver_data instead of caching
> lis3lv02d_dmi_id pointer and then you can mark lis3lv02d_devices array
> as __initconst to free additional space not needed at runtime on x86
> machines without accelerometer where CONFIG_DELL_SMO8800=y.

This is a good idea, done for v5.

Regards,

Hans


> 
>> +		pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n");
>> +		return 0;
>> +	}
>> +
>> +	/*
>> +	 * 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.45.1
>>
> 

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

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build errors:

[auto build test ERROR on andi-shyti/i2c/i2c-host]
[also build test ERROR on wsa/i2c/for-next linus/master v6.10-rc5 next-20240627]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/i2c-core-Setup-i2c_adapter-runtime-pm-before-calling-device_add/20240626-053449
base:   git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240624111519.15652-5-hdegoede%40redhat.com
patch subject: [PATCH v4 4/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d
config: i386-randconfig-002-20240628 (https://download.01.org/0day-ci/archive/20240628/202406280739.e0s764jH-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240628/202406280739.e0s764jH-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406280739.e0s764jH-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'find_i801':
>> drivers/platform/x86/dell/dell-lis3lv02d.c:77:21: error: implicit declaration of function 'i2c_get_adapter'; did you mean 'i2c_get_adapdata'? [-Werror=implicit-function-declaration]
      77 |         *adap_ret = i2c_get_adapter(adap->nr);
         |                     ^~~~~~~~~~~~~~~
         |                     i2c_get_adapdata
>> drivers/platform/x86/dell/dell-lis3lv02d.c:77:19: warning: assignment to 'struct i2c_adapter *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      77 |         *adap_ret = i2c_get_adapter(adap->nr);
         |                   ^
   drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'instantiate_i2c_client':
>> drivers/platform/x86/dell/dell-lis3lv02d.c:96:19: error: implicit declaration of function 'i2c_new_client_device' [-Werror=implicit-function-declaration]
      96 |         i2c_dev = i2c_new_client_device(adap, &info);
         |                   ^~~~~~~~~~~~~~~~~~~~~
>> drivers/platform/x86/dell/dell-lis3lv02d.c:96:17: warning: assignment to 'struct i2c_client *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
      96 |         i2c_dev = i2c_new_client_device(adap, &info);
         |                 ^
>> drivers/platform/x86/dell/dell-lis3lv02d.c:104:9: error: implicit declaration of function 'i2c_put_adapter' [-Werror=implicit-function-declaration]
     104 |         i2c_put_adapter(adap);
         |         ^~~~~~~~~~~~~~~
   drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'dell_lis3lv02d_module_exit':
>> drivers/platform/x86/dell/dell-lis3lv02d.c:193:9: error: implicit declaration of function 'i2c_unregister_device' [-Werror=implicit-function-declaration]
     193 |         i2c_unregister_device(i2c_dev);
         |         ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +77 drivers/platform/x86/dell/dell-lis3lv02d.c

    65	
    66	static int find_i801(struct device *dev, void *data)
    67	{
    68		struct i2c_adapter *adap, **adap_ret = data;
    69	
    70		adap = i2c_verify_adapter(dev);
    71		if (!adap)
    72			return 0;
    73	
    74		if (!i2c_adapter_is_main_i801(adap))
    75			return 0;
    76	
  > 77		*adap_ret = i2c_get_adapter(adap->nr);
    78		return 1;
    79	}
    80	
    81	static void instantiate_i2c_client(struct work_struct *work)
    82	{
    83		struct i2c_board_info info = { };
    84		struct i2c_adapter *adap = NULL;
    85	
    86		if (i2c_dev)
    87			return;
    88	
    89		bus_for_each_dev(&i2c_bus_type, NULL, &adap, find_i801);
    90		if (!adap)
    91			return;
    92	
    93		info.addr = (long)lis3lv02d_dmi_id->driver_data;
    94		strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
    95	
  > 96		i2c_dev = i2c_new_client_device(adap, &info);
    97		if (IS_ERR(i2c_dev)) {
    98			pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev));
    99			i2c_dev = NULL;
   100		} else {
   101			pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr);
   102		}
   103	
 > 104		i2c_put_adapter(adap);
   105	}
   106	static DECLARE_WORK(i2c_work, instantiate_i2c_client);
   107	
   108	static int i2c_bus_notify(struct notifier_block *nb, unsigned long action, void *data)
   109	{
   110		struct device *dev = data;
   111		struct i2c_client *client;
   112		struct i2c_adapter *adap;
   113	
   114		switch (action) {
   115		case BUS_NOTIFY_ADD_DEVICE:
   116			adap = i2c_verify_adapter(dev);
   117			if (!adap)
   118				break;
   119	
   120			if (i2c_adapter_is_main_i801(adap))
   121				queue_work(system_long_wq, &i2c_work);
   122			break;
   123		case BUS_NOTIFY_REMOVED_DEVICE:
   124			client = i2c_verify_client(dev);
   125			if (!client)
   126				break;
   127	
   128			if (i2c_dev == client) {
   129				pr_debug("lis3lv02d i2c_client removed\n");
   130				i2c_dev = NULL;
   131			}
   132			break;
   133		default:
   134			break;
   135		}
   136	
   137		return 0;
   138	}
   139	static struct notifier_block i2c_nb = { .notifier_call = i2c_bus_notify };
   140	
   141	static int match_acpi_device_ids(struct device *dev, const void *data)
   142	{
   143		const struct acpi_device_id *ids = data;
   144	
   145		return acpi_match_device(ids, dev) ? 1 : 0;
   146	}
   147	
   148	static int __init dell_lis3lv02d_init(void)
   149	{
   150		struct device *dev;
   151		int err;
   152	
   153		/*
   154		 * First check for a matching platform_device. This protects against
   155		 * SMO88xx ACPI fwnodes which actually do have an I2C resource, which
   156		 * will already have an i2c_client instantiated (not a platform_device).
   157		 */
   158		dev = bus_find_device(&platform_bus_type, NULL, smo8800_ids, match_acpi_device_ids);
   159		if (!dev) {
   160			pr_debug("No SMO88xx platform-device found\n");
   161			return 0;
   162		}
   163		put_device(dev);
   164	
   165		lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices);
   166		if (!lis3lv02d_dmi_id) {
   167			pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n");
   168			return 0;
   169		}
   170	
   171		/*
   172		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
   173		 * i2c_client instantiation.
   174		 */
   175		err = bus_register_notifier(&i2c_bus_type, &i2c_nb);
   176		if (err)
   177			return err;
   178	
   179		notifier_registered = true;
   180	
   181		queue_work(system_long_wq, &i2c_work);
   182		return 0;
   183	}
   184	module_init(dell_lis3lv02d_init);
   185	
   186	static void __exit dell_lis3lv02d_module_exit(void)
   187	{
   188		if (!notifier_registered)
   189			return;
   190	
   191		bus_unregister_notifier(&i2c_bus_type, &i2c_nb);
   192		cancel_work_sync(&i2c_work);
 > 193		i2c_unregister_device(i2c_dev);
   194	}
   195	module_exit(dell_lis3lv02d_module_exit);
   196	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v4 5/6] 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>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
q# Please enter the commit message for your changes. Lines starting
---
 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 e581b8e2a603..a7409db0505b 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[] = {
 	 * 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.45.1

Re: [PATCH v4 5/6] platform/x86: dell-smo8800: Add a couple more models to lis3lv02d_devices[]

[Pali Rohár]

On Monday 24 June 2024 13:15:17 Hans de Goede wrote:
> 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>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> q# Please enter the commit message for your changes. Lines starting

Garbage at the end of commit message.

> ---
>  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 e581b8e2a603..a7409db0505b 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[] = {
>  	 * 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.45.1
> 

Re: [PATCH v4 5/6] platform/x86: dell-smo8800: Add a couple more models to lis3lv02d_devices[]

[Hans de Goede]

Hi,

On 6/24/24 8:14 PM, Pali Rohár wrote:
> On Monday 24 June 2024 13:15:17 Hans de Goede wrote:
>> 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>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> q# Please enter the commit message for your changes. Lines starting
> 
> Garbage at the end of commit message.

Thanks, fixed for v5.

Regards,

Hans



> 
>> ---
>>  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 e581b8e2a603..a7409db0505b 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[] = {
>>  	 * 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.45.1
>>
> 

[PATCH v4 6/6] 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>
---
 drivers/platform/x86/dell/dell-lis3lv02d.c | 133 ++++++++++++++++++++-
 1 file changed, 131 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
index a7409db0505b..173615fd2646 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,121 @@ static const struct dmi_system_id *lis3lv02d_dmi_id;
 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 is the kernel version of the single register device sanity checks from
+ * the i2c_safety_check function from lm_sensors sensor-detect script:
+ * This is meant to prevent access to 1-register-only devices,
+ * which are designed to be accessed with SMBus receive byte and SMBus send
+ * byte transactions (i.e. short reads and short writes) and treat SMBus
+ * read byte as a real write followed by a read. The device detection
+ * routines would write random values to the chip with possibly very nasty
+ * results for the hardware. Note that this function won't catch all such
+ * chips, as it assumes that reads and writes relate to the same register,
+ * but that's the best we can do.
+ */
+static int i2c_safety_check(struct i2c_adapter *adap, u8 addr)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+	u8 data;
+
+	/*
+	 * First receive a byte from the chip, and remember it. This
+	 * also checks if there is a device at the address at all.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	data = smbus_data.byte;
+
+	/*
+	 * Receive a byte again; very likely to be the same for
+	 * 1-register-only devices.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a register offset equal to
+	 * the read byte; for 1-register-only device this should read
+	 * the same byte value in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a slightly different register
+	 * offset; this should again read the register offset in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != (data ^ 0x01))
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Apparently this is a 1-register-only device, restore the original
+	 * register value and leave it alone.
+	 */
+	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
+		       I2C_SMBUS_BYTE, NULL);
+	pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr);
+	return -ENODEV;
+}
+
+static int detect_lis3lv02d(struct i2c_adapter *adap, u8 addr,
+			    struct i2c_board_info *info)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+
+	pr_info("Probing for lis3lv02d on address 0x%02x\n", addr);
+	err = i2c_safety_check(adap, addr);
+	if (err < 0)
+		return err;
+
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0) {
+		pr_warn("Failed to read who-am-i register: %d\n", err);
+		return err;
+	}
+
+	/* 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:
+		pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte);
+		return -ENODEV;
+	}
+
+	pr_debug("Detected lis3lv02d on address 0x%02x\n", addr);
+	info->addr = addr;
+	return 0;
+}
+
 static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
 {
 	/*
@@ -93,7 +210,17 @@ static void instantiate_i2c_client(struct work_struct *work)
 	if (!adap)
 		return;
 
-	info.addr = (long)lis3lv02d_dmi_id->driver_data;
+	if (lis3lv02d_dmi_id) {
+		info.addr = (long)lis3lv02d_dmi_id->driver_data;
+	} else {
+		/* First try address 0x29 (most used) and then try 0x1d */
+		if (detect_lis3lv02d(adap, 0x29, &info) != 0 &&
+		    detect_lis3lv02d(adap, 0x1d, &info) != 0) {
+			pr_warn("failed to probe for lis3lv02d I2C address\n");
+			goto out_put_adapter;
+		}
+	}
+
 	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
 
 	i2c_dev = i2c_new_client_device(adap, &info);
@@ -104,6 +231,7 @@ static void instantiate_i2c_client(struct work_struct *work)
 		pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr);
 	}
 
+out_put_adapter:
 	i2c_put_adapter(adap);
 }
 static DECLARE_WORK(i2c_work, instantiate_i2c_client);
@@ -166,8 +294,9 @@ 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;
 	}
 
-- 
2.45.1

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

[Pali Rohár]

On Monday 24 June 2024 13:15:18 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>

My comments from the previous version still apply there. There is no
dangerous warning neither in parameter name and its description, there
is no warning once the parameter was specified. And there is missing
information from Dell how it is going to be handled for new
machines. So first ask Dell about the current state and future.

> ---
>  drivers/platform/x86/dell/dell-lis3lv02d.c | 133 ++++++++++++++++++++-
>  1 file changed, 131 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
> index a7409db0505b..173615fd2646 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,121 @@ static const struct dmi_system_id *lis3lv02d_dmi_id;
>  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 is the kernel version of the single register device sanity checks from
> + * the i2c_safety_check function from lm_sensors sensor-detect script:
> + * This is meant to prevent access to 1-register-only devices,
> + * which are designed to be accessed with SMBus receive byte and SMBus send
> + * byte transactions (i.e. short reads and short writes) and treat SMBus
> + * read byte as a real write followed by a read. The device detection
> + * routines would write random values to the chip with possibly very nasty
> + * results for the hardware. Note that this function won't catch all such
> + * chips, as it assumes that reads and writes relate to the same register,
> + * but that's the best we can do.
> + */
> +static int i2c_safety_check(struct i2c_adapter *adap, u8 addr)
> +{
> +	union i2c_smbus_data smbus_data;
> +	int err;
> +	u8 data;
> +
> +	/*
> +	 * First receive a byte from the chip, and remember it. This
> +	 * also checks if there is a device at the address at all.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +			     I2C_SMBUS_BYTE, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	data = smbus_data.byte;
> +
> +	/*
> +	 * Receive a byte again; very likely to be the same for
> +	 * 1-register-only devices.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +			     I2C_SMBUS_BYTE, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != data)
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Then try a standard byte read, with a register offset equal to
> +	 * the read byte; for 1-register-only device this should read
> +	 * the same byte value in return.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != data)
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Then try a standard byte read, with a slightly different register
> +	 * offset; this should again read the register offset in return.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != (data ^ 0x01))
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Apparently this is a 1-register-only device, restore the original
> +	 * register value and leave it alone.
> +	 */
> +	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
> +		       I2C_SMBUS_BYTE, NULL);
> +	pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr);
> +	return -ENODEV;
> +}
> +
> +static int detect_lis3lv02d(struct i2c_adapter *adap, u8 addr,
> +			    struct i2c_board_info *info)
> +{
> +	union i2c_smbus_data smbus_data;
> +	int err;
> +
> +	pr_info("Probing for lis3lv02d on address 0x%02x\n", addr);
> +	err = i2c_safety_check(adap, addr);
> +	if (err < 0)
> +		return err;
> +
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0) {
> +		pr_warn("Failed to read who-am-i register: %d\n", err);
> +		return err;
> +	}
> +
> +	/* 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:
> +		pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte);
> +		return -ENODEV;
> +	}
> +
> +	pr_debug("Detected lis3lv02d on address 0x%02x\n", addr);
> +	info->addr = addr;
> +	return 0;
> +}
> +
>  static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
>  {
>  	/*
> @@ -93,7 +210,17 @@ static void instantiate_i2c_client(struct work_struct *work)
>  	if (!adap)
>  		return;
>  
> -	info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +	if (lis3lv02d_dmi_id) {
> +		info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +	} else {
> +		/* First try address 0x29 (most used) and then try 0x1d */
> +		if (detect_lis3lv02d(adap, 0x29, &info) != 0 &&
> +		    detect_lis3lv02d(adap, 0x1d, &info) != 0) {
> +			pr_warn("failed to probe for lis3lv02d I2C address\n");
> +			goto out_put_adapter;
> +		}
> +	}
> +
>  	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>  
>  	i2c_dev = i2c_new_client_device(adap, &info);
> @@ -104,6 +231,7 @@ static void instantiate_i2c_client(struct work_struct *work)
>  		pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr);
>  	}
>  
> +out_put_adapter:
>  	i2c_put_adapter(adap);
>  }
>  static DECLARE_WORK(i2c_work, instantiate_i2c_client);
> @@ -166,8 +294,9 @@ 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;
>  	}
>  
> -- 
> 2.45.1
> 

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

[Hans de Goede]

Hi Pali,

On 6/24/24 8:21 PM, Pali Rohár wrote:
> On Monday 24 June 2024 13:15:18 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>
> 
> My comments from the previous version still apply there. There is no
> dangerous warning neither in parameter name and its description, there
> is no warning once the parameter was specified.

I have added the same "this may be dangerous" text from:

  pr_info("Pass dell_lis3lv02d.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");

to the MODULE_PARM_DESC() now too (for v5).

> And there is missing
> information from Dell how it is going to be handled for new
> machines. So first ask Dell about the current state and future.

As mentioned before Dell is on the Cc. And I no longer have any direct
contacts inside Dell. If you know anyone inside Dell feel free to email
them about this.

Regards,

Hans



>> ---
>>  drivers/platform/x86/dell/dell-lis3lv02d.c | 133 ++++++++++++++++++++-
>>  1 file changed, 131 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
>> index a7409db0505b..173615fd2646 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,121 @@ static const struct dmi_system_id *lis3lv02d_dmi_id;
>>  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 is the kernel version of the single register device sanity checks from
>> + * the i2c_safety_check function from lm_sensors sensor-detect script:
>> + * This is meant to prevent access to 1-register-only devices,
>> + * which are designed to be accessed with SMBus receive byte and SMBus send
>> + * byte transactions (i.e. short reads and short writes) and treat SMBus
>> + * read byte as a real write followed by a read. The device detection
>> + * routines would write random values to the chip with possibly very nasty
>> + * results for the hardware. Note that this function won't catch all such
>> + * chips, as it assumes that reads and writes relate to the same register,
>> + * but that's the best we can do.
>> + */
>> +static int i2c_safety_check(struct i2c_adapter *adap, u8 addr)
>> +{
>> +	union i2c_smbus_data smbus_data;
>> +	int err;
>> +	u8 data;
>> +
>> +	/*
>> +	 * First receive a byte from the chip, and remember it. This
>> +	 * also checks if there is a device at the address at all.
>> +	 */
>> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
>> +			     I2C_SMBUS_BYTE, &smbus_data);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	data = smbus_data.byte;
>> +
>> +	/*
>> +	 * Receive a byte again; very likely to be the same for
>> +	 * 1-register-only devices.
>> +	 */
>> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
>> +			     I2C_SMBUS_BYTE, &smbus_data);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	if (smbus_data.byte != data)
>> +		return 0; /* Not a 1-register-only device. */
>> +
>> +	/*
>> +	 * Then try a standard byte read, with a register offset equal to
>> +	 * the read byte; for 1-register-only device this should read
>> +	 * the same byte value in return.
>> +	 */
>> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
>> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	if (smbus_data.byte != data)
>> +		return 0; /* Not a 1-register-only device. */
>> +
>> +	/*
>> +	 * Then try a standard byte read, with a slightly different register
>> +	 * offset; this should again read the register offset in return.
>> +	 */
>> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
>> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	if (smbus_data.byte != (data ^ 0x01))
>> +		return 0; /* Not a 1-register-only device. */
>> +
>> +	/*
>> +	 * Apparently this is a 1-register-only device, restore the original
>> +	 * register value and leave it alone.
>> +	 */
>> +	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
>> +		       I2C_SMBUS_BYTE, NULL);
>> +	pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr);
>> +	return -ENODEV;
>> +}
>> +
>> +static int detect_lis3lv02d(struct i2c_adapter *adap, u8 addr,
>> +			    struct i2c_board_info *info)
>> +{
>> +	union i2c_smbus_data smbus_data;
>> +	int err;
>> +
>> +	pr_info("Probing for lis3lv02d on address 0x%02x\n", addr);
>> +	err = i2c_safety_check(adap, addr);
>> +	if (err < 0)
>> +		return err;
>> +
>> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
>> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
>> +	if (err < 0) {
>> +		pr_warn("Failed to read who-am-i register: %d\n", err);
>> +		return err;
>> +	}
>> +
>> +	/* 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:
>> +		pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte);
>> +		return -ENODEV;
>> +	}
>> +
>> +	pr_debug("Detected lis3lv02d on address 0x%02x\n", addr);
>> +	info->addr = addr;
>> +	return 0;
>> +}
>> +
>>  static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
>>  {
>>  	/*
>> @@ -93,7 +210,17 @@ static void instantiate_i2c_client(struct work_struct *work)
>>  	if (!adap)
>>  		return;
>>  
>> -	info.addr = (long)lis3lv02d_dmi_id->driver_data;
>> +	if (lis3lv02d_dmi_id) {
>> +		info.addr = (long)lis3lv02d_dmi_id->driver_data;
>> +	} else {
>> +		/* First try address 0x29 (most used) and then try 0x1d */
>> +		if (detect_lis3lv02d(adap, 0x29, &info) != 0 &&
>> +		    detect_lis3lv02d(adap, 0x1d, &info) != 0) {
>> +			pr_warn("failed to probe for lis3lv02d I2C address\n");
>> +			goto out_put_adapter;
>> +		}
>> +	}
>> +
>>  	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>>  
>>  	i2c_dev = i2c_new_client_device(adap, &info);
>> @@ -104,6 +231,7 @@ static void instantiate_i2c_client(struct work_struct *work)
>>  		pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr);
>>  	}
>>  
>> +out_put_adapter:
>>  	i2c_put_adapter(adap);
>>  }
>>  static DECLARE_WORK(i2c_work, instantiate_i2c_client);
>> @@ -166,8 +294,9 @@ 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;
>>  	}
>>  
>> -- 
>> 2.45.1
>>
> 

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

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build errors:

[auto build test ERROR on andi-shyti/i2c/i2c-host]
[also build test ERROR on wsa/i2c/for-next linus/master v6.10-rc5 next-20240627]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/i2c-core-Setup-i2c_adapter-runtime-pm-before-calling-device_add/20240626-053449
base:   git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git i2c/i2c-host
patch link:    https://lore.kernel.org/r/20240624111519.15652-7-hdegoede%40redhat.com
patch subject: [PATCH v4 6/6] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address
config: i386-randconfig-002-20240628 (https://download.01.org/0day-ci/archive/20240628/202406280954.PwlEGWfP-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240628/202406280954.PwlEGWfP-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406280954.PwlEGWfP-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'i2c_safety_check':
>> drivers/platform/x86/dell/dell-lis3lv02d.c:88:15: error: implicit declaration of function 'i2c_smbus_xfer' [-Werror=implicit-function-declaration]
      88 |         err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
         |               ^~~~~~~~~~~~~~
   drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'find_i801':
   drivers/platform/x86/dell/dell-lis3lv02d.c:197:21: error: implicit declaration of function 'i2c_get_adapter'; did you mean 'i2c_get_adapdata'? [-Werror=implicit-function-declaration]
     197 |         *adap_ret = i2c_get_adapter(adap->nr);
         |                     ^~~~~~~~~~~~~~~
         |                     i2c_get_adapdata
   drivers/platform/x86/dell/dell-lis3lv02d.c:197:19: warning: assignment to 'struct i2c_adapter *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     197 |         *adap_ret = i2c_get_adapter(adap->nr);
         |                   ^
   drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'instantiate_i2c_client':
   drivers/platform/x86/dell/dell-lis3lv02d.c:226:19: error: implicit declaration of function 'i2c_new_client_device' [-Werror=implicit-function-declaration]
     226 |         i2c_dev = i2c_new_client_device(adap, &info);
         |                   ^~~~~~~~~~~~~~~~~~~~~
   drivers/platform/x86/dell/dell-lis3lv02d.c:226:17: warning: assignment to 'struct i2c_client *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     226 |         i2c_dev = i2c_new_client_device(adap, &info);
         |                 ^
   drivers/platform/x86/dell/dell-lis3lv02d.c:235:9: error: implicit declaration of function 'i2c_put_adapter' [-Werror=implicit-function-declaration]
     235 |         i2c_put_adapter(adap);
         |         ^~~~~~~~~~~~~~~
   drivers/platform/x86/dell/dell-lis3lv02d.c: In function 'dell_lis3lv02d_module_exit':
   drivers/platform/x86/dell/dell-lis3lv02d.c:325:9: error: implicit declaration of function 'i2c_unregister_device' [-Werror=implicit-function-declaration]
     325 |         i2c_unregister_device(i2c_dev);
         |         ^~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +/i2c_smbus_xfer +88 drivers/platform/x86/dell/dell-lis3lv02d.c

    65	
    66	/*
    67	 * This is the kernel version of the single register device sanity checks from
    68	 * the i2c_safety_check function from lm_sensors sensor-detect script:
    69	 * This is meant to prevent access to 1-register-only devices,
    70	 * which are designed to be accessed with SMBus receive byte and SMBus send
    71	 * byte transactions (i.e. short reads and short writes) and treat SMBus
    72	 * read byte as a real write followed by a read. The device detection
    73	 * routines would write random values to the chip with possibly very nasty
    74	 * results for the hardware. Note that this function won't catch all such
    75	 * chips, as it assumes that reads and writes relate to the same register,
    76	 * but that's the best we can do.
    77	 */
    78	static int i2c_safety_check(struct i2c_adapter *adap, u8 addr)
    79	{
    80		union i2c_smbus_data smbus_data;
    81		int err;
    82		u8 data;
    83	
    84		/*
    85		 * First receive a byte from the chip, and remember it. This
    86		 * also checks if there is a device at the address at all.
    87		 */
  > 88		err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
    89				     I2C_SMBUS_BYTE, &smbus_data);
    90		if (err < 0)
    91			return err;
    92	
    93		data = smbus_data.byte;
    94	
    95		/*
    96		 * Receive a byte again; very likely to be the same for
    97		 * 1-register-only devices.
    98		 */
    99		err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
   100				     I2C_SMBUS_BYTE, &smbus_data);
   101		if (err < 0)
   102			return err;
   103	
   104		if (smbus_data.byte != data)
   105			return 0; /* Not a 1-register-only device. */
   106	
   107		/*
   108		 * Then try a standard byte read, with a register offset equal to
   109		 * the read byte; for 1-register-only device this should read
   110		 * the same byte value in return.
   111		 */
   112		err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
   113				     I2C_SMBUS_BYTE_DATA, &smbus_data);
   114		if (err < 0)
   115			return err;
   116	
   117		if (smbus_data.byte != data)
   118			return 0; /* Not a 1-register-only device. */
   119	
   120		/*
   121		 * Then try a standard byte read, with a slightly different register
   122		 * offset; this should again read the register offset in return.
   123		 */
   124		err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
   125				     I2C_SMBUS_BYTE_DATA, &smbus_data);
   126		if (err < 0)
   127			return err;
   128	
   129		if (smbus_data.byte != (data ^ 0x01))
   130			return 0; /* Not a 1-register-only device. */
   131	
   132		/*
   133		 * Apparently this is a 1-register-only device, restore the original
   134		 * register value and leave it alone.
   135		 */
   136		i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
   137			       I2C_SMBUS_BYTE, NULL);
   138		pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr);
   139		return -ENODEV;
   140	}
   141	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Pali Rohár]

On Monday 24 June 2024 13:15:12 Hans de Goede wrote:
> Hans de Goede (6):
>   i2c: core: Setup i2c_adapter runtime-pm before calling device_add()
>   i2c: i801: Use a different adapter-name for IDF adapters
>   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

Patches 1-5 looks good. There are just a few minor things, but you can add
Reviewed-by: Pali Rohár <pali@kernel.org>

For patch 6 as I mentioned previously I'm strictly against this change
until somebody goes and politely ask Dell about the current situation of
the discovering of accelerometer's i2c address. And if there is no other
option than start discussion if Dell can include this information into
DMI / ACPI / WMI or other part of firmware data which they can send from
BIOS/UEFI to operating system.

>  drivers/i2c/busses/i2c-i801.c                | 133 +-------
>  drivers/i2c/i2c-core-base.c                  |  18 +-
>  drivers/platform/x86/dell/Makefile           |   1 +
>  drivers/platform/x86/dell/dell-lis3lv02d.c   | 331 +++++++++++++++++++
>  drivers/platform/x86/dell/dell-smo8800-ids.h |  26 ++
>  drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
>  6 files changed, 379 insertions(+), 146 deletions(-)
>  create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
>  create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h
> 
> -- 
> 2.45.1
> 

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

[Hans de Goede]

Hi,

On 6/24/24 8:28 PM, Pali Rohár wrote:
> On Monday 24 June 2024 13:15:12 Hans de Goede wrote:
>> Hans de Goede (6):
>>   i2c: core: Setup i2c_adapter runtime-pm before calling device_add()
>>   i2c: i801: Use a different adapter-name for IDF adapters
>>   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
> 
> Patches 1-5 looks good. There are just a few minor things, but you can add
> Reviewed-by: Pali Rohár <pali@kernel.org>

Thank you.

> For patch 6 as I mentioned previously I'm strictly against this change
> until somebody goes and politely ask Dell about the current situation of
> the discovering of accelerometer's i2c address.

Dell is on the Cc and not responding...

> And if there is no other
> option than start discussion if Dell can include this information into
> DMI / ACPI / WMI or other part of firmware data which they can send from
> BIOS/UEFI to operating system.

AFAIK newer Dell laptops don't have a freefall sensor anymore since
everything has moved to nvme. Even the bigger laptops seems to simply
have multiple nvme slots rather then room for a 2.5" HDD. Note I did not
research this, this is is my observation from 3 newer Dell laptops which
I have access to.

Regards,

Hans




>>  drivers/i2c/busses/i2c-i801.c                | 133 +-------
>>  drivers/i2c/i2c-core-base.c                  |  18 +-
>>  drivers/platform/x86/dell/Makefile           |   1 +
>>  drivers/platform/x86/dell/dell-lis3lv02d.c   | 331 +++++++++++++++++++
>>  drivers/platform/x86/dell/dell-smo8800-ids.h |  26 ++
>>  drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
>>  6 files changed, 379 insertions(+), 146 deletions(-)
>>  create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
>>  create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h
>>
>> -- 
>> 2.45.1
>>
> 

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

[Pali Rohár]

On Wednesday 03 July 2024 12:58:01 Hans de Goede wrote:
> Hi,
> 
> On 6/24/24 8:28 PM, Pali Rohár wrote:
> > On Monday 24 June 2024 13:15:12 Hans de Goede wrote:
> >> Hans de Goede (6):
> >>   i2c: core: Setup i2c_adapter runtime-pm before calling device_add()
> >>   i2c: i801: Use a different adapter-name for IDF adapters
> >>   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
> > 
> > Patches 1-5 looks good. There are just a few minor things, but you can add
> > Reviewed-by: Pali Rohár <pali@kernel.org>
> 
> Thank you.
> 
> > For patch 6 as I mentioned previously I'm strictly against this change
> > until somebody goes and politely ask Dell about the current situation of
> > the discovering of accelerometer's i2c address.
> 
> Dell is on the Cc and not responding...

And what do you expecting here? That somebody on the group address
specified in CC list would react to all your tons of messages? Not
mentioning the fact that you did not even ask anything.

This is not how things works.

If you do not change your attitude here then I highly doubt that
somebody will respond to you.

I have feeling that you are doing it on purpose just because you do not
want to do anything, and trying to find some kind of proof that nobody
is responding to you, to convince others for merge your last hack change.

> > And if there is no other
> > option than start discussion if Dell can include this information into
> > DMI / ACPI / WMI or other part of firmware data which they can send from
> > BIOS/UEFI to operating system.
> 
> AFAIK newer Dell laptops don't have a freefall sensor anymore since
> everything has moved to nvme. Even the bigger laptops seems to simply
> have multiple nvme slots rather then room for a 2.5" HDD. Note I did not
> research this, this is is my observation from 3 newer Dell laptops which
> I have access to.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> >>  drivers/i2c/busses/i2c-i801.c                | 133 +-------
> >>  drivers/i2c/i2c-core-base.c                  |  18 +-
> >>  drivers/platform/x86/dell/Makefile           |   1 +
> >>  drivers/platform/x86/dell/dell-lis3lv02d.c   | 331 +++++++++++++++++++
> >>  drivers/platform/x86/dell/dell-smo8800-ids.h |  26 ++
> >>  drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
> >>  6 files changed, 379 insertions(+), 146 deletions(-)
> >>  create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
> >>  create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h
> >>
> >> -- 
> >> 2.45.1
> >>
> > 
> 

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

[Hans de Goede]

Hi Pali,

On 7/3/24 8:41 PM, Pali Rohár wrote:
> On Wednesday 03 July 2024 12:58:01 Hans de Goede wrote:
>> Hi,
>>
>> On 6/24/24 8:28 PM, Pali Rohár wrote:
>>> On Monday 24 June 2024 13:15:12 Hans de Goede wrote:
>>>> Hans de Goede (6):
>>>>   i2c: core: Setup i2c_adapter runtime-pm before calling device_add()
>>>>   i2c: i801: Use a different adapter-name for IDF adapters
>>>>   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
>>>
>>> Patches 1-5 looks good. There are just a few minor things, but you can add
>>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>
>> Thank you.
>>
>>> For patch 6 as I mentioned previously I'm strictly against this change
>>> until somebody goes and politely ask Dell about the current situation of
>>> the discovering of accelerometer's i2c address.
>>
>> Dell is on the Cc and not responding...
> 
> And what do you expecting here? That somebody on the group address
> specified in CC list would react to all your tons of messages? Not
> mentioning the fact that you did not even ask anything.

You keep on repeating this since I first posted this patch
in December last year, but as I already wrote back then:

https://lore.kernel.org/platform-driver-x86/8b3946e0-7eb5-4e1f-9708-1f6cfda95e1a@redhat.com/

"Unfortunately I no longer have any contacts inside Dell"

And Paul Menzel reached out back to gkh back then asking
if Greg had any contacts in he did not have any contacts
either.

Dell.Client.Kernel@dell.com is the official address listed
for Dell drivers under drivers/platform/x86 .

> This is not how things works.

The email address which I'm using is *THE* one which Dell has
provided for contacting about Dell pdx86 drivers. I really
don't know what else you expect me to do here.

You just keep repeating that Dell should be contacted about
this and multiple people (me and Andy) have already pointed
out that Dell does not have any other contact info. Repeating
the same remark over and over does not change things.

As I mentioned in my other email too, if you think you can do
better feel free to try and contact Dell your self, something
which you could already have done the first time you mentioned
this in December 2023, back when I already said I don't have
any other contact info for Dell.

> If you do not change your attitude here then I highly doubt that
> somebody will respond to you.
> 
> I have feeling that you are doing it on purpose just because you do not
> want to do anything, and trying to find some kind of proof that nobody
> is responding to you, to convince others for merge your last hack change.

This is just plain hurtful I do not believe I have ever done
anything to earn this level of distrust from you.

I am hurt that you cannot at least show the common decency to
assume good intentions from my side.

Regards,

Hans

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

[Pali Rohár]

On Thursday 04 July 2024 12:17:27 Hans de Goede wrote:
> Hi Pali,
> 
> On 7/3/24 8:41 PM, Pali Rohár wrote:
> > On Wednesday 03 July 2024 12:58:01 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 6/24/24 8:28 PM, Pali Rohár wrote:
> >>> On Monday 24 June 2024 13:15:12 Hans de Goede wrote:
> >>>> Hans de Goede (6):
> >>>>   i2c: core: Setup i2c_adapter runtime-pm before calling device_add()
> >>>>   i2c: i801: Use a different adapter-name for IDF adapters
> >>>>   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
> >>>
> >>> Patches 1-5 looks good. There are just a few minor things, but you can add
> >>> Reviewed-by: Pali Rohár <pali@kernel.org>
> >>
> >> Thank you.
> >>
> >>> For patch 6 as I mentioned previously I'm strictly against this change
> >>> until somebody goes and politely ask Dell about the current situation of
> >>> the discovering of accelerometer's i2c address.
> >>
> >> Dell is on the Cc and not responding...
> > 
> > And what do you expecting here? That somebody on the group address
> > specified in CC list would react to all your tons of messages? Not
> > mentioning the fact that you did not even ask anything.
> 
> You keep on repeating this since I first posted this patch
> in December last year, but as I already wrote back then:

Yes, because you have not done anything and you are just repeating those
nonsenses. What are you expecting? You are either doing all this on
purpose or you are just lazy and think that somebody (e.g. me) would do
this stuff.

> https://lore.kernel.org/platform-driver-x86/8b3946e0-7eb5-4e1f-9708-1f6cfda95e1a@redhat.com/
> 
> "Unfortunately I no longer have any contacts inside Dell"
> 
> And Paul Menzel reached out back to gkh back then asking
> if Greg had any contacts in he did not have any contacts
> either.
> 
> Dell.Client.Kernel@dell.com is the official address listed
> for Dell drivers under drivers/platform/x86 .

Perfect. And may you explain why you have not tried to contact them with
addressed requests of exact information of what you need and ask for
help? You wrote tons of emails with zero value.

> > This is not how things works.
> 
> The email address which I'm using is *THE* one which Dell has
> provided for contacting about Dell pdx86 drivers. I really
> don't know what else you expect me to do here.
> 
> You just keep repeating that Dell should be contacted about
> this and multiple people (me and Andy) have already pointed
> out that Dell does not have any other contact info. Repeating
> the same remark over and over does not change things.
> 
> As I mentioned in my other email too, if you think you can do
> better feel free to try and contact Dell your self, something

I'm not your servant and I'm not going to play role of your secretary.

> which you could already have done the first time you mentioned
> this in December 2023, back when I already said I don't have
> any other contact info for Dell.

Could you stop complaining? I'm really not interested in your stories
why you are not wanted to do anything.

> > If you do not change your attitude here then I highly doubt that
> > somebody will respond to you.
> > 
> > I have feeling that you are doing it on purpose just because you do not
> > want to do anything, and trying to find some kind of proof that nobody
> > is responding to you, to convince others for merge your last hack change.
> 
> This is just plain hurtful I do not believe I have ever done
> anything to earn this level of distrust from you.

Then read your message again. Now it is more clear that you are doing
it on purpose.

> I am hurt that you cannot at least show the common decency to
> assume good intentions from my side.

I hope that I do not have to explain you how to find contact address in
MAINTAINERS file, how to write an addressed email for target audience,
how to formulate questions and how to ask for information. And how to do
it in _one_ message.

> Regards,
> 
> Hans
> 
> 

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

[Hans de Goede]

Hi,

On 7/4/24 5:54 PM, Pali Rohár wrote:
> On Thursday 04 July 2024 12:17:27 Hans de Goede wrote:
>> Hi Pali,
>>
>> On 7/3/24 8:41 PM, Pali Rohár wrote:
>>> On Wednesday 03 July 2024 12:58:01 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 6/24/24 8:28 PM, Pali Rohár wrote:
>>>>> On Monday 24 June 2024 13:15:12 Hans de Goede wrote:
>>>>>> Hans de Goede (6):
>>>>>>   i2c: core: Setup i2c_adapter runtime-pm before calling device_add()
>>>>>>   i2c: i801: Use a different adapter-name for IDF adapters
>>>>>>   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
>>>>>
>>>>> Patches 1-5 looks good. There are just a few minor things, but you can add
>>>>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>>>
>>>> Thank you.
>>>>
>>>>> For patch 6 as I mentioned previously I'm strictly against this change
>>>>> until somebody goes and politely ask Dell about the current situation of
>>>>> the discovering of accelerometer's i2c address.
>>>>
>>>> Dell is on the Cc and not responding...
>>>
>>> And what do you expecting here? That somebody on the group address
>>> specified in CC list would react to all your tons of messages? Not
>>> mentioning the fact that you did not even ask anything.
>>
>> You keep on repeating this since I first posted this patch
>> in December last year, but as I already wrote back then:
> 
> Yes, because you have not done anything and you are just repeating those
> nonsenses. What are you expecting? You are either doing all this on
> purpose or you are just lazy and think that somebody (e.g. me) would do
> this stuff.
> 
>> https://lore.kernel.org/platform-driver-x86/8b3946e0-7eb5-4e1f-9708-1f6cfda95e1a@redhat.com/
>>
>> "Unfortunately I no longer have any contacts inside Dell"
>>
>> And Paul Menzel reached out back to gkh back then asking
>> if Greg had any contacts in he did not have any contacts
>> either.
>>
>> Dell.Client.Kernel@dell.com is the official address listed
>> for Dell drivers under drivers/platform/x86 .
> 
> Perfect. And may you explain why you have not tried to contact them with
> addressed requests of exact information of what you need and ask for
> help? You wrote tons of emails with zero value.
> 
>>> This is not how things works.
>>
>> The email address which I'm using is *THE* one which Dell has
>> provided for contacting about Dell pdx86 drivers. I really
>> don't know what else you expect me to do here.
>>
>> You just keep repeating that Dell should be contacted about
>> this and multiple people (me and Andy) have already pointed
>> out that Dell does not have any other contact info. Repeating
>> the same remark over and over does not change things.
>>
>> As I mentioned in my other email too, if you think you can do
>> better feel free to try and contact Dell your self, something
> 
> I'm not your servant and I'm not going to play role of your secretary.
> 
>> which you could already have done the first time you mentioned
>> this in December 2023, back when I already said I don't have
>> any other contact info for Dell.
> 
> Could you stop complaining? I'm really not interested in your stories
> why you are not wanted to do anything.
> 
>>> If you do not change your attitude here then I highly doubt that
>>> somebody will respond to you.
>>>
>>> I have feeling that you are doing it on purpose just because you do not
>>> want to do anything, and trying to find some kind of proof that nobody
>>> is responding to you, to convince others for merge your last hack change.
>>
>> This is just plain hurtful I do not believe I have ever done
>> anything to earn this level of distrust from you.
> 
> Then read your message again. Now it is more clear that you are doing
> it on purpose.
> 
>> I am hurt that you cannot at least show the common decency to
>> assume good intentions from my side.
> 
> I hope that I do not have to explain you how to find contact address in
> MAINTAINERS file, how to write an addressed email for target audience,
> how to formulate questions and how to ask for information. And how to do
> it in _one_ message.

Really? Doubling down after insulting me (calling my integrity into
question) ?

So be it, I will simply not be responding to you in this thread anymore.

Regards,

Hans

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

[Hans de Goede]

On 7/3/24 8:41 PM, Pali Rohár wrote:
> On Wednesday 03 July 2024 12:58:01 Hans de Goede wrote:
>> Hi,
>>
>> On 6/24/24 8:28 PM, Pali Rohár wrote:
>>> On Monday 24 June 2024 13:15:12 Hans de Goede wrote:
>>>> Hans de Goede (6):
>>>>   i2c: core: Setup i2c_adapter runtime-pm before calling device_add()
>>>>   i2c: i801: Use a different adapter-name for IDF adapters
>>>>   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
>>>
>>> Patches 1-5 looks good. There are just a few minor things, but you can add
>>> Reviewed-by: Pali Rohár <pali@kernel.org>
>>
>> Thank you.
>>
>>> For patch 6 as I mentioned previously I'm strictly against this change
>>> until somebody goes and politely ask Dell about the current situation of
>>> the discovering of accelerometer's i2c address.
>>
>> Dell is on the Cc and not responding...
> 
> And what do you expecting here? That somebody on the group address
> specified in CC list would react to all your tons of messages? Not
> mentioning the fact that you did not even ask anything.
> 
> This is not how things works.
> 
> If you do not change your attitude here then I highly doubt that
> somebody will respond to you.
> 
> I have feeling that you are doing it on purpose just because you do not
> want to do anything, and trying to find some kind of proof that nobody
> is responding to you, to convince others for merge your last hack change.

p.s.

This is not a hack, please stop with this nonsense about how this
is a hack and super dangerous. It is neither.

Probing for i2c-addresses is something which the kernel has done since
the 199x years. The whole i2c-core has infrastructure for drivers to
indicate which addresses they want the core to probe for them.

This is used by a lot of hwmon i2c drivers since hwmon chips typically
are not described in the ACPI tables.

So userspace manually modprobes these (after a sensors-detect run
setting up the config)  and then when loaded the core will call
these drivers detect() callback for the addresses listed in their
i2c_driver struct.

People using these driver are having the i2c-addresses listed in these
drivers probed (on adapters which indicate they support probing like
i2c-i801) every single boot!

Regards,

Hans









>>> And if there is no other
>>> option than start discussion if Dell can include this information into
>>> DMI / ACPI / WMI or other part of firmware data which they can send from
>>> BIOS/UEFI to operating system.
>>
>> AFAIK newer Dell laptops don't have a freefall sensor anymore since
>> everything has moved to nvme. Even the bigger laptops seems to simply
>> have multiple nvme slots rather then room for a 2.5" HDD. Note I did not
>> research this, this is is my observation from 3 newer Dell laptops which
>> I have access to.
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>
>>>>  drivers/i2c/busses/i2c-i801.c                | 133 +-------
>>>>  drivers/i2c/i2c-core-base.c                  |  18 +-
>>>>  drivers/platform/x86/dell/Makefile           |   1 +
>>>>  drivers/platform/x86/dell/dell-lis3lv02d.c   | 331 +++++++++++++++++++
>>>>  drivers/platform/x86/dell/dell-smo8800-ids.h |  26 ++
>>>>  drivers/platform/x86/dell/dell-smo8800.c     |  16 +-
>>>>  6 files changed, 379 insertions(+), 146 deletions(-)
>>>>  create mode 100644 drivers/platform/x86/dell/dell-lis3lv02d.c
>>>>  create mode 100644 drivers/platform/x86/dell/dell-smo8800-ids.h
>>>>
>>>> -- 
>>>> 2.45.1
>>>>
>>>
>>
>