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

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

[Hans de Goede]

Hi All,

Here is v3 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
the SMO88xx specific dell-smo8800 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 the SMO88xx
specific dell-smo8800 module where it belongs.

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-smo8800 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 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 instantiation of lis3lv02d i2c_client
    from i2c-i801 to dell-smo8800
  platform/x86: dell-smo8800: Allow lis3lv02d i2c_client instantiation
    without IRQ
  platform/x86: dell-smo8800: Add a couple more models to
    dell_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              |  13 +-
 drivers/platform/x86/dell/dell-smo8800.c | 438 +++++++++++++++++++++--
 3 files changed, 424 insertions(+), 160 deletions(-)

-- 
2.45.1

[PATCH v3 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>
---
 drivers/i2c/i2c-core-base.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index db0d1ac82910..fcb4696f1f93 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1521,7 +1521,13 @@ 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);
+	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 +1539,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 v3 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>
---
 drivers/i2c/busses/i2c-i801.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index d2d2a6dbe29f..5ac5bbd60d45 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
 
 	i801_add_tco(priv);
 
-	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
-		"SMBus I801 adapter at %04lx", priv->smba);
+	if (priv->features & FEATURE_IDF)
+		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
+			"SMBus I801 IDF adapter at %04lx", priv->smba);
+	else
+		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
+			"SMBus I801 adapter at %04lx", priv->smba);
+
 	err = i2c_add_adapter(&priv->adapter);
 	if (err) {
 		platform_device_unregister(priv->tco_pdev);
-- 
2.45.1

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

[Hans de Goede]

It is not necessary to handle the Dell specific instantiation of
i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
inside the generic i801 I2C adapter driver.

The kernel already instantiates platform_device-s for these ACPI devices
and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
platform drivers.

Move the i2c_client instantiation from the generic i2c-i801 driver to
the SMO88xx specific dell-smo8800 driver.

Moving the i2c_client instantiation here 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 the SMO88xx
specific dell-smo8800 module where it belongs.

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-smo8800 driver, without needing to modify
the i2c-i801 code.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note the goto out_put_adapter, which can be avoided by moving the DMI check
up, is there deliberately as preparation for adding support to probe for
the i2c address in case there is no DMI match.
---
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/dell-smo8800.c | 214 ++++++++++++++++++++++-
 2 files changed, 213 insertions(+), 125 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ac5bbd60d45..db8d31411148 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/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index f7ec17c56833..cd2e48405859 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -4,20 +4,26 @@
  *
  *  Copyright (C) 2012 Sonal Santan <sonal.santan@gmail.com>
  *  Copyright (C) 2014 Pali Rohár <pali@kernel.org>
+ *  Copyright (C) 2023 Hans de Goede <hansg@kernel.org>
  *
  *  This is loosely based on lis3lv02d driver.
  */
 
 #define DRIVER_NAME "smo8800"
 
+#include <linux/device/bus.h>
+#include <linux/dmi.h>
 #include <linux/fs.h>
+#include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/notifier.h>
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
+#include <linux/workqueue.h>
 
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
@@ -25,6 +31,9 @@ struct smo8800_device {
 	struct miscdevice miscdev;   /* for /dev/freefall */
 	unsigned long misc_opened;   /* whether the device is open */
 	wait_queue_head_t misc_wait; /* Wait queue for the misc dev */
+	struct notifier_block i2c_nb;/* i2c bus notifier */
+	struct work_struct i2c_work; /* Work for instantiating lis3lv02d i2c_client */
+	struct i2c_client *i2c_dev;  /* i2c_client for lis3lv02d */
 	struct device *dev;          /* acpi device */
 };
 
@@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
 	.release = smo8800_misc_release,
 };
 
+/*
+ * 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 smo8800_lis3lv02d_devices[] = {
+	/*
+	 * Dell platform team told us that these Latitude devices have
+	 * ST microelectronics accelerometer at I2C address 0x29.
+	 */
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	/*
+	 * Additional individual entries were added after verification.
+	 */
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+		},
+		.driver_data = (void *)0x1dL,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{ }
+};
+
+static int smo8800_find_i801(struct device *dev, void *data)
+{
+	struct i2c_adapter *adap, **adap_ret = data;
+
+	adap = i2c_verify_adapter(dev);
+	if (!adap)
+		return 0;
+
+	if (!strstarts(adap->name, "SMBus I801 adapter"))
+		return 0;
+
+	*adap_ret = i2c_get_adapter(adap->nr);
+	return 1;
+}
+
+static void smo8800_instantiate_i2c_client(struct work_struct *work)
+{
+	struct smo8800_device *smo8800 =
+		container_of(work, struct smo8800_device, i2c_work);
+	const struct dmi_system_id *lis3lv02d_dmi_id;
+	struct i2c_board_info info = { };
+	struct i2c_adapter *adap = NULL;
+
+	if (smo8800->i2c_dev)
+		return;
+
+	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
+	if (!adap)
+		return;
+
+	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);
+	if (!lis3lv02d_dmi_id)
+		goto out_put_adapter;
+
+	info.addr = (long)lis3lv02d_dmi_id->driver_data;
+	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+
+	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
+	if (IS_ERR(smo8800->i2c_dev)) {
+		dev_err(smo8800->dev, "error %ld registering %s i2c_client\n",
+			PTR_ERR(smo8800->i2c_dev), info.type);
+		smo8800->i2c_dev = NULL;
+	} else {
+		dev_dbg(smo8800->dev, "registered %s i2c_client on address 0x%02x\n",
+			info.type, info.addr);
+	}
+
+out_put_adapter:
+	i2c_put_adapter(adap);
+}
+
+static int smo8800_i2c_bus_notify(struct notifier_block *nb,
+				  unsigned long action, void *data)
+{
+	struct smo8800_device *smo8800 =
+		container_of(nb, struct smo8800_device, i2c_nb);
+	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 (strstarts(adap->name, "SMBus I801 adapter"))
+			queue_work(system_long_wq, &smo8800->i2c_work);
+		break;
+	case BUS_NOTIFY_REMOVED_DEVICE:
+		client = i2c_verify_client(dev);
+		if (!client)
+			break;
+
+		if (smo8800->i2c_dev == client) {
+			dev_dbg(smo8800->dev, "accelerometer i2c_client removed\n");
+			smo8800->i2c_dev = NULL;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static int smo8800_probe(struct platform_device *device)
 {
 	int err;
@@ -118,8 +305,10 @@ static int smo8800_probe(struct platform_device *device)
 	smo8800->miscdev.minor = MISC_DYNAMIC_MINOR;
 	smo8800->miscdev.name = "freefall";
 	smo8800->miscdev.fops = &smo8800_misc_fops;
+	smo8800->i2c_nb.notifier_call = smo8800_i2c_bus_notify;
 
 	init_waitqueue_head(&smo8800->misc_wait);
+	INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client);
 
 	err = misc_register(&smo8800->miscdev);
 	if (err) {
@@ -147,8 +336,26 @@ static int smo8800_probe(struct platform_device *device)
 
 	dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
 		 smo8800->irq);
+
+	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
+		/*
+		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
+		 * i2c_client instantiation.
+		 */
+		err = bus_register_notifier(&i2c_bus_type, &smo8800->i2c_nb);
+		if (err)
+			goto error_free_irq;
+
+		queue_work(system_long_wq, &smo8800->i2c_work);
+	} else {
+		dev_warn(&device->dev,
+			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
+	}
+
 	return 0;
 
+error_free_irq:
+	free_irq(smo8800->irq, smo8800);
 error:
 	misc_deregister(&smo8800->miscdev);
 	return err;
@@ -158,12 +365,17 @@ static void smo8800_remove(struct platform_device *device)
 {
 	struct smo8800_device *smo8800 = platform_get_drvdata(device);
 
+	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
+		bus_unregister_notifier(&i2c_bus_type, &smo8800->i2c_nb);
+		cancel_work_sync(&smo8800->i2c_work);
+		i2c_unregister_device(smo8800->i2c_dev);
+	}
+
 	free_irq(smo8800->irq, smo8800);
 	misc_deregister(&smo8800->miscdev);
 	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 },
-- 
2.45.1

[PATCH v3 4/6] platform/x86: dell-smo8800: Allow lis3lv02d i2c_client instantiation without IRQ

[Hans de Goede]

The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5"
sata disk; or a 90Wh battery in which case the battery occupies the space
for the optional 2.5" sata disk.

On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS
does not add an IRQ resource to the SMO8810 ACPI device.

Make the misc-device registration and the requesting of the IRQ optional
and instantiate a lis3lv02d i2c_client independent of the IRQ being there,
so that the non freefall lis3lv02d functionality can still be used.

Note that IRQ 0 is not a valid IRQ number for platform IRQs
and this patch relies on that.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell/dell-smo8800.c | 67 +++++++++++++-----------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index cd2e48405859..2e49bbb569c6 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -310,33 +310,32 @@ static int smo8800_probe(struct platform_device *device)
 	init_waitqueue_head(&smo8800->misc_wait);
 	INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client);
 
-	err = misc_register(&smo8800->miscdev);
-	if (err) {
-		dev_err(&device->dev, "failed to register misc dev: %d\n", err);
-		return err;
+	err = platform_get_irq_optional(device, 0);
+	if (err > 0)
+		smo8800->irq = err;
+
+	if (smo8800->irq) {
+		err = misc_register(&smo8800->miscdev);
+		if (err) {
+			dev_err(&device->dev, "failed to register misc dev: %d\n", err);
+			return err;
+		}
+
+		err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
+					   smo8800_interrupt_thread,
+					   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+					   DRIVER_NAME, smo8800);
+		if (err) {
+			dev_err(&device->dev,
+				"failed to request thread for IRQ %d: %d\n",
+				smo8800->irq, err);
+			goto error;
+		}
+
+		dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
+			 smo8800->irq);
 	}
 
-	platform_set_drvdata(device, smo8800);
-
-	err = platform_get_irq(device, 0);
-	if (err < 0)
-		goto error;
-	smo8800->irq = err;
-
-	err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
-				   smo8800_interrupt_thread,
-				   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
-				   DRIVER_NAME, smo8800);
-	if (err) {
-		dev_err(&device->dev,
-			"failed to request thread for IRQ %d: %d\n",
-			smo8800->irq, err);
-		goto error;
-	}
-
-	dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
-		 smo8800->irq);
-
 	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
 		/*
 		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
@@ -350,14 +349,20 @@ static int smo8800_probe(struct platform_device *device)
 	} else {
 		dev_warn(&device->dev,
 			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
+		if (!smo8800->irq)
+			return -ENODEV;
 	}
 
+	platform_set_drvdata(device, smo8800);
 	return 0;
 
 error_free_irq:
-	free_irq(smo8800->irq, smo8800);
+	if (smo8800->irq) {
+		free_irq(smo8800->irq, smo8800);
 error:
-	misc_deregister(&smo8800->miscdev);
+		misc_deregister(&smo8800->miscdev);
+	}
+
 	return err;
 }
 
@@ -371,9 +376,11 @@ static void smo8800_remove(struct platform_device *device)
 		i2c_unregister_device(smo8800->i2c_dev);
 	}
 
-	free_irq(smo8800->irq, smo8800);
-	misc_deregister(&smo8800->miscdev);
-	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
+	if (smo8800->irq) {
+		free_irq(smo8800->irq, smo8800);
+		misc_deregister(&smo8800->miscdev);
+		dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
+	}
 }
 
 static const struct acpi_device_id smo8800_ids[] = {
-- 
2.45.1

[PATCH v3 5/6] platform/x86: dell-smo8800: Add a couple more models to dell_lis3lv02d_devices[]

[Hans de Goede]

Add the accelerometer address for the following laptop models
to dell_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>
---
 drivers/platform/x86/dell/dell-smo8800.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 2e49bbb569c6..4c79b2599d96 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -173,6 +173,20 @@ static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
 		},
 		.driver_data = (void *)0x29L,
 	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6330"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6430"),
+		},
+		.driver_data = (void *)0x29L,
+	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
@@ -194,6 +208,13 @@ static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
 		},
 		.driver_data = (void *)0x29L,
 	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 9550"),
+		},
+		.driver_data = (void *)0x29L,
+	},
 	{
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
-- 
2.45.1

[PATCH v3 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell/dell-smo8800.c | 152 ++++++++++++++++++++++-
 1 file changed, 147 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 4c79b2599d96..d64d200e927a 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -10,6 +10,7 @@
  */
 
 #define DRIVER_NAME "smo8800"
+#define LIS3_WHO_AM_I 0x0f
 
 #include <linux/device/bus.h>
 #include <linux/dmi.h>
@@ -25,6 +26,10 @@
 #include <linux/uaccess.h>
 #include <linux/workqueue.h>
 
+static bool probe_i2c_addr;
+module_param(probe_i2c_addr, bool, 0444);
+MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown");
+
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
 	atomic_t counter;            /* count after last read */
@@ -225,6 +230,130 @@ static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
 	{ }
 };
 
+/*
+ * 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 device *dev, struct i2c_adapter *adap, u8 addr)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+	u8 data;
+
+	/*
+	 * First receive a byte from the chip, and remember it. This
+	 * also checks if there is a device at the address at all.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	data = smbus_data.byte;
+
+	/*
+	 * Receive a byte again; very likely to be the same for
+	 * 1-register-only devices.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a register offset equal to
+	 * the read byte; for 1-register-only device this should read
+	 * the same byte value in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a slightly different register
+	 * offset; this should again read the register offset in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != (data ^ 0x01))
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Apparently this is a 1-register-only device, restore the original
+	 * register value and leave it alone.
+	 */
+	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
+		       I2C_SMBUS_BYTE, NULL);
+	dev_warn(dev, "I2C safety check for address 0x%02x failed, skipping\n", addr);
+	return -ENODEV;
+}
+
+static int smo8800_detect_accel(struct smo8800_device *smo8800,
+				struct i2c_adapter *adap, u8 addr,
+				struct i2c_board_info *info, bool probe)
+{
+	union i2c_smbus_data smbus_data;
+	const char *type;
+	int err;
+
+	if (probe) {
+		dev_info(smo8800->dev, "Probing for accelerometer on address 0x%02x\n", addr);
+		err = i2c_safety_check(smo8800->dev, 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) {
+		dev_warn(smo8800->dev, "Failed to read who-am-i register: %d\n", err);
+		return err;
+	}
+
+	/* who-am-i register mappings from drivers/misc/lis3lv02d/lis3lv02d.c */
+	switch (smbus_data.byte) {
+	case 0x32:
+		type = "lis331dlh";
+		break;
+	case 0x33:
+		type = "lis2de12"; /* LIS3DC / HP3DC in drivers/misc/lis3lv02d/lis3lv02d.c */
+		break;
+	case 0x3a:
+		type = "lis3lv02dl_accel";
+		break;
+	case 0x3b:
+		type = "lis302dl";
+		break;
+	default:
+		dev_warn(smo8800->dev, "Unknown who-am-i register value 0x%02x\n",
+			 smbus_data.byte);
+		return -ENODEV;
+	}
+
+	dev_dbg(smo8800->dev, "Detected %s accelerometer on address 0x%02x\n", type, addr);
+	strscpy(info->type, "lis3lv02d", I2C_NAME_SIZE);
+	info->addr = addr;
+	return 0;
+}
+
 static int smo8800_find_i801(struct device *dev, void *data)
 {
 	struct i2c_adapter *adap, **adap_ret = data;
@@ -247,6 +376,7 @@ static void smo8800_instantiate_i2c_client(struct work_struct *work)
 	const struct dmi_system_id *lis3lv02d_dmi_id;
 	struct i2c_board_info info = { };
 	struct i2c_adapter *adap = NULL;
+	int err;
 
 	if (smo8800->i2c_dev)
 		return;
@@ -256,11 +386,22 @@ static void smo8800_instantiate_i2c_client(struct work_struct *work)
 		return;
 
 	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);
-	if (!lis3lv02d_dmi_id)
+	if (lis3lv02d_dmi_id) {
+		info.addr = (long)lis3lv02d_dmi_id->driver_data;
+		/* Always detect the accel-type, this also checks the accel is actually there */
+		err = smo8800_detect_accel(smo8800, adap, info.addr, &info, false);
+		if (err)
+			goto out_put_adapter;
+	} else if (probe_i2c_addr) {
+		/* First try address 0x29 (most used) and then try 0x1d */
+		if (smo8800_detect_accel(smo8800, adap, 0x29, &info, true) != 0 &&
+		    smo8800_detect_accel(smo8800, adap, 0x1d, &info, true) != 0) {
+			dev_warn(smo8800->dev, "failed to probe for lis3lv02d I2C address\n");
+			goto out_put_adapter;
+		}
+	} else {
 		goto out_put_adapter;
-
-	info.addr = (long)lis3lv02d_dmi_id->driver_data;
-	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+	}
 
 	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
 	if (IS_ERR(smo8800->i2c_dev)) {
@@ -357,7 +498,7 @@ static int smo8800_probe(struct platform_device *device)
 			 smo8800->irq);
 	}
 
-	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
+	if (dmi_check_system(smo8800_lis3lv02d_devices) || probe_i2c_addr) {
 		/*
 		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
 		 * i2c_client instantiation.
@@ -370,6 +511,7 @@ static int smo8800_probe(struct platform_device *device)
 	} else {
 		dev_warn(&device->dev,
 			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
+		dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
 		if (!smo8800->irq)
 			return -ENODEV;
 	}
-- 
2.45.1

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

[Andy Shevchenko]

On Fri, Jun 21, 2024 at 2:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

LGTM, but this is non-trivial (I think) for the usual cases, can you
add a comment on top of the code you modified?

...

...somewhere here?

> +       res = device_add(&adap->dev);
>         if (res) {
>                 pr_err("adapter '%s': can't register device (%d)\n", adap->name, res);
>                 goto out_list;

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jun 21, 2024 at 2:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> 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.

...

> -       snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> -               "SMBus I801 adapter at %04lx", priv->smba);
> +       if (priv->features & FEATURE_IDF)
> +               snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> +                       "SMBus I801 IDF adapter at %04lx", priv->smba);
> +       else
> +               snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> +                       "SMBus I801 adapter at %04lx", priv->smba);

Alternatively this can be

       snprintf(priv->adapter.name, sizeof(priv->adapter.name),
               "SMBus %s adapter at %04lx", priv->features &
FEATURE_IDF ? "I801 IDF" : "I801", priv->smba);

This will save a few bytes here and there.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jun 21, 2024 at 2:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> It is not necessary to handle the Dell specific instantiation of
> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
> inside the generic i801 I2C adapter driver.
>
> The kernel already instantiates platform_device-s for these ACPI devices
> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> platform drivers.
>
> Move the i2c_client instantiation from the generic i2c-i801 driver to
> the SMO88xx specific dell-smo8800 driver.
>
> Moving the i2c_client instantiation here 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 the SMO88xx
> specific dell-smo8800 module where it belongs.
>
> 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-smo8800 driver, without needing to modify
> the i2c-i801 code.

...


> +static int smo8800_find_i801(struct device *dev, void *data)
> +{
> +       struct i2c_adapter *adap, **adap_ret = data;
> +
> +       adap = i2c_verify_adapter(dev);
> +       if (!adap)
> +               return 0;
> +
> +       if (!strstarts(adap->name, "SMBus I801 adapter"))

With the comment on the previous patch I'm wondering if it makes sense
to have this to be as simple as strstr("I801") or strstr("I801 IDF")?

> +               return 0;
> +
> +       *adap_ret = i2c_get_adapter(adap->nr);
> +       return 1;
> +}

...

> +       info.addr = (long)lis3lv02d_dmi_id->driver_data;

Hmm... Usually we use uintptr_t, but okay.

...

> +               if (strstarts(adap->name, "SMBus I801 adapter"))

A dup? Is there a possibility it may go desynchronized?

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/6] platform/x86: dell-smo8800: Allow lis3lv02d i2c_client instantiation without IRQ

[Andy Shevchenko]

On Fri, Jun 21, 2024 at 2:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5"
> sata disk; or a 90Wh battery in which case the battery occupies the space
> for the optional 2.5" sata disk.
>
> On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS
> does not add an IRQ resource to the SMO8810 ACPI device.
>
> Make the misc-device registration and the requesting of the IRQ optional
> and instantiate a lis3lv02d i2c_client independent of the IRQ being there,
> so that the non freefall lis3lv02d functionality can still be used.

non-freefall

> Note that IRQ 0 is not a valid IRQ number for platform IRQs
> and this patch relies on that.

...

> +       err = platform_get_irq_optional(device, 0);
> +       if (err > 0)
> +               smo8800->irq = err;
> +
> +       if (smo8800->irq) {

You can still do it in one branch less. But I think I understand the
motivation of the split.

> +               err = misc_register(&smo8800->miscdev);
> +               if (err) {
> +                       dev_err(&device->dev, "failed to register misc dev: %d\n", err);
> +                       return err;
> +               }
> +
> +               err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
> +                                          smo8800_interrupt_thread,
> +                                          IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +                                          DRIVER_NAME, smo8800);
> +               if (err) {
> +                       dev_err(&device->dev,
> +                               "failed to request thread for IRQ %d: %d\n",
> +                               smo8800->irq, err);
> +                       goto error;
> +               }
> +
> +               dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
> +                        smo8800->irq);
>         }

...

>  error_free_irq:
> -       free_irq(smo8800->irq, smo8800);
> +       if (smo8800->irq) {
> +               free_irq(smo8800->irq, smo8800);
>  error:
> -       misc_deregister(&smo8800->miscdev);
> +               misc_deregister(&smo8800->miscdev);
> +       }

This is quite unusual.
I would rather expect

  label_foo:
    if (...)
      foo()
  label_bar:
    if (...)
      bar()


--
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Jun 21, 2024 at 2:26 PM Hans de Goede <hdegoede@redhat.com> 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

At --> Add ?

models

> which are not on the list.
>
> The new probing code sits behind a new probe_i2c_addr module parameter,
> which is disabled by default because probing might be dangerous.

...

> +/*
> + * This is the kernel version of the single register device sanity checks from
> + * the i2c_safety_check function from lm_sensors sensor-detect script:

i2c_safety_check()

> + * 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.
> + */

...

> +       err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,

Is 0x01 defined already?
Or BIT(0) ?

> +                            I2C_SMBUS_BYTE_DATA, &smbus_data);
> +       if (err < 0)
> +               return err;
> +
> +       if (smbus_data.byte != (data ^ 0x01))

Ditto.

> +               return 0; /* Not a 1-register-only device. */

...

> +               dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");

command line
...or...
cmdline

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
> 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>
> ---
>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index d2d2a6dbe29f..5ac5bbd60d45 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  
>  	i801_add_tco(priv);
>  
> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> -		"SMBus I801 adapter at %04lx", priv->smba);
> +	if (priv->features & FEATURE_IDF)
> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
> +	else
> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> +			"SMBus I801 adapter at %04lx", priv->smba);
> +

User visible name is identifier for user / human.

If somebody is going to read this code in next 10 years then can ask
question why to have different name for IDF FEATURE and not also for
other features? And can come to conclusion to unify all names to be
same (why not? it is user identifier).

Depending on user names between different kernel subsystem is fragile,
specially for future as rename can happen.

If you are depending on FEATURE_IDF flag then check for the flag
directly, and not hiding the flag by serializing it into the user
visible name (char[] variable) and then de-serializing it in different
kernel subsystem. If the flag is not exported yet then export it via
some function or other API.

Using user name via char[] for internal flags is misusing user visible
name structures.

>  	err = i2c_add_adapter(&priv->adapter);
>  	if (err) {
>  		platform_device_unregister(priv->tco_pdev);
> -- 
> 2.45.1
> 

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

[Pali Rohár]

On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> It is not necessary to handle the Dell specific instantiation of
> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
> inside the generic i801 I2C adapter driver.
> 
> The kernel already instantiates platform_device-s for these ACPI devices
> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> platform drivers.
> 
> Move the i2c_client instantiation from the generic i2c-i801 driver to
> the SMO88xx specific dell-smo8800 driver.

Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
and freefall code for smo88xx are basically independent.

lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
detection. The only thing which have these "drivers" common is the ACPI
detection mechanism based on presence of SMO88?? identifiers from
acpi_smo8800_ids[] array.

I think it makes both "drivers" cleaner if they are put into separate
files as they are independent of each one.

What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
instead (or similar name)? And just share list of ACPI ids via some
header file (or something like that).

> Moving the i2c_client instantiation here 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 the SMO88xx
> specific dell-smo8800 module where it belongs.
> 
> 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-smo8800 driver, without needing to modify
> the i2c-i801 code.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note the goto out_put_adapter, which can be avoided by moving the DMI check
> up, is there deliberately as preparation for adding support to probe for
> the i2c address in case there is no DMI match.
> ---
> 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/dell-smo8800.c | 214 ++++++++++++++++++++++-
>  2 files changed, 213 insertions(+), 125 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5ac5bbd60d45..db8d31411148 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/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> index f7ec17c56833..cd2e48405859 100644
> --- a/drivers/platform/x86/dell/dell-smo8800.c
> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> @@ -4,20 +4,26 @@
>   *
>   *  Copyright (C) 2012 Sonal Santan <sonal.santan@gmail.com>
>   *  Copyright (C) 2014 Pali Rohár <pali@kernel.org>
> + *  Copyright (C) 2023 Hans de Goede <hansg@kernel.org>
>   *
>   *  This is loosely based on lis3lv02d driver.
>   */
>  
>  #define DRIVER_NAME "smo8800"
>  
> +#include <linux/device/bus.h>
> +#include <linux/dmi.h>
>  #include <linux/fs.h>
> +#include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/miscdevice.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/notifier.h>
>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
> +#include <linux/workqueue.h>
>  
>  struct smo8800_device {
>  	u32 irq;                     /* acpi device irq */
> @@ -25,6 +31,9 @@ struct smo8800_device {
>  	struct miscdevice miscdev;   /* for /dev/freefall */
>  	unsigned long misc_opened;   /* whether the device is open */
>  	wait_queue_head_t misc_wait; /* Wait queue for the misc dev */
> +	struct notifier_block i2c_nb;/* i2c bus notifier */
> +	struct work_struct i2c_work; /* Work for instantiating lis3lv02d i2c_client */
> +	struct i2c_client *i2c_dev;  /* i2c_client for lis3lv02d */
>  	struct device *dev;          /* acpi device */
>  };
>  
> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
>  	.release = smo8800_misc_release,
>  };
>  
> +/*
> + * 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 smo8800_lis3lv02d_devices[] = {
> +	/*
> +	 * Dell platform team told us that these Latitude devices have
> +	 * ST microelectronics accelerometer at I2C address 0x29.
> +	 */
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	/*
> +	 * Additional individual entries were added after verification.
> +	 */
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> +		},
> +		.driver_data = (void *)0x1dL,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> +		},
> +		.driver_data = (void *)0x29L,

At least for me, casting i2c address to LONG and then to pointer looks
very strange. If I look at this code without knowing what the number
0x29 means I would not figure out that expression "(void *)0x29L" is i2c
address.

Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
instead of ".something = (void *)0x29L" to make it readable?

Also does the whole device table has to be such verbose with lot of
duplicated information (which probably also increase size of every linux
image which includes this driver into it)? Previously there was a very
compact structure on few lines:

| 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 },

> +	},
> +	{ }
> +};
> +
> +static int smo8800_find_i801(struct device *dev, void *data)
> +{
> +	struct i2c_adapter *adap, **adap_ret = data;
> +
> +	adap = i2c_verify_adapter(dev);
> +	if (!adap)
> +		return 0;
> +
> +	if (!strstarts(adap->name, "SMBus I801 adapter"))
> +		return 0;
> +
> +	*adap_ret = i2c_get_adapter(adap->nr);
> +	return 1;
> +}
> +
> +static void smo8800_instantiate_i2c_client(struct work_struct *work)
> +{
> +	struct smo8800_device *smo8800 =
> +		container_of(work, struct smo8800_device, i2c_work);
> +	const struct dmi_system_id *lis3lv02d_dmi_id;
> +	struct i2c_board_info info = { };
> +	struct i2c_adapter *adap = NULL;
> +
> +	if (smo8800->i2c_dev)
> +		return;
> +
> +	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
> +	if (!adap)
> +		return;
> +
> +	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);
> +	if (!lis3lv02d_dmi_id)
> +		goto out_put_adapter;
> +
> +	info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> +
> +	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
> +	if (IS_ERR(smo8800->i2c_dev)) {
> +		dev_err(smo8800->dev, "error %ld registering %s i2c_client\n",
> +			PTR_ERR(smo8800->i2c_dev), info.type);
> +		smo8800->i2c_dev = NULL;
> +	} else {
> +		dev_dbg(smo8800->dev, "registered %s i2c_client on address 0x%02x\n",
> +			info.type, info.addr);
> +	}
> +
> +out_put_adapter:
> +	i2c_put_adapter(adap);
> +}
> +
> +static int smo8800_i2c_bus_notify(struct notifier_block *nb,
> +				  unsigned long action, void *data)
> +{
> +	struct smo8800_device *smo8800 =
> +		container_of(nb, struct smo8800_device, i2c_nb);
> +	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 (strstarts(adap->name, "SMBus I801 adapter"))
> +			queue_work(system_long_wq, &smo8800->i2c_work);
> +		break;
> +	case BUS_NOTIFY_REMOVED_DEVICE:
> +		client = i2c_verify_client(dev);
> +		if (!client)
> +			break;
> +
> +		if (smo8800->i2c_dev == client) {
> +			dev_dbg(smo8800->dev, "accelerometer i2c_client removed\n");
> +			smo8800->i2c_dev = NULL;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>  static int smo8800_probe(struct platform_device *device)
>  {
>  	int err;
> @@ -118,8 +305,10 @@ static int smo8800_probe(struct platform_device *device)
>  	smo8800->miscdev.minor = MISC_DYNAMIC_MINOR;
>  	smo8800->miscdev.name = "freefall";
>  	smo8800->miscdev.fops = &smo8800_misc_fops;
> +	smo8800->i2c_nb.notifier_call = smo8800_i2c_bus_notify;
>  
>  	init_waitqueue_head(&smo8800->misc_wait);
> +	INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client);
>  
>  	err = misc_register(&smo8800->miscdev);
>  	if (err) {
> @@ -147,8 +336,26 @@ static int smo8800_probe(struct platform_device *device)
>  
>  	dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
>  		 smo8800->irq);
> +
> +	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
> +		/*
> +		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
> +		 * i2c_client instantiation.
> +		 */
> +		err = bus_register_notifier(&i2c_bus_type, &smo8800->i2c_nb);
> +		if (err)
> +			goto error_free_irq;
> +
> +		queue_work(system_long_wq, &smo8800->i2c_work);
> +	} else {
> +		dev_warn(&device->dev,
> +			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> +	}
> +
>  	return 0;
>  
> +error_free_irq:
> +	free_irq(smo8800->irq, smo8800);
>  error:
>  	misc_deregister(&smo8800->miscdev);
>  	return err;
> @@ -158,12 +365,17 @@ static void smo8800_remove(struct platform_device *device)
>  {
>  	struct smo8800_device *smo8800 = platform_get_drvdata(device);
>  
> +	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
> +		bus_unregister_notifier(&i2c_bus_type, &smo8800->i2c_nb);
> +		cancel_work_sync(&smo8800->i2c_work);
> +		i2c_unregister_device(smo8800->i2c_dev);
> +	}
> +
>  	free_irq(smo8800->irq, smo8800);
>  	misc_deregister(&smo8800->miscdev);
>  	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 },
> -- 
> 2.45.1
> 

Re: [PATCH v3 4/6] platform/x86: dell-smo8800: Allow lis3lv02d i2c_client instantiation without IRQ

[Pali Rohár]

On Friday 21 June 2024 14:24:59 Hans de Goede wrote:
> The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5"
> sata disk; or a 90Wh battery in which case the battery occupies the space
> for the optional 2.5" sata disk.
> 
> On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS
> does not add an IRQ resource to the SMO8810 ACPI device.

That is a pity, but OK, manufacturer decided that freefall sensor is
enabled by BIOS firmware only if the SATA is present.

> Make the misc-device registration and the requesting of the IRQ optional
> and instantiate a lis3lv02d i2c_client independent of the IRQ being there,
> so that the non freefall lis3lv02d functionality can still be used.
> 
> Note that IRQ 0 is not a valid IRQ number for platform IRQs
> and this patch relies on that.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell/dell-smo8800.c | 67 +++++++++++++-----------
>  1 file changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> index cd2e48405859..2e49bbb569c6 100644
> --- a/drivers/platform/x86/dell/dell-smo8800.c
> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> @@ -310,33 +310,32 @@ static int smo8800_probe(struct platform_device *device)
>  	init_waitqueue_head(&smo8800->misc_wait);
>  	INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client);
>  
> -	err = misc_register(&smo8800->miscdev);
> -	if (err) {
> -		dev_err(&device->dev, "failed to register misc dev: %d\n", err);
> -		return err;
> +	err = platform_get_irq_optional(device, 0);
> +	if (err > 0)
> +		smo8800->irq = err;

This code should be rather change to fail immediately. If the IRQ number
is not provided by the BIOS then the ACPI SMO88xx is not usable for us
at all. So return error from the smo8800_probe function.

> +
> +	if (smo8800->irq) {
> +		err = misc_register(&smo8800->miscdev);
> +		if (err) {
> +			dev_err(&device->dev, "failed to register misc dev: %d\n", err);
> +			return err;
> +		}
> +
> +		err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
> +					   smo8800_interrupt_thread,
> +					   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> +					   DRIVER_NAME, smo8800);
> +		if (err) {
> +			dev_err(&device->dev,
> +				"failed to request thread for IRQ %d: %d\n",
> +				smo8800->irq, err);
> +			goto error;
> +		}
> +
> +		dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
> +			 smo8800->irq);
>  	}
>  
> -	platform_set_drvdata(device, smo8800);
> -
> -	err = platform_get_irq(device, 0);
> -	if (err < 0)
> -		goto error;
> -	smo8800->irq = err;
> -
> -	err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
> -				   smo8800_interrupt_thread,
> -				   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> -				   DRIVER_NAME, smo8800);
> -	if (err) {
> -		dev_err(&device->dev,
> -			"failed to request thread for IRQ %d: %d\n",
> -			smo8800->irq, err);
> -		goto error;
> -	}
> -
> -	dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
> -		 smo8800->irq);
> -
>  	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
>  		/*
>  		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
> @@ -350,14 +349,20 @@ static int smo8800_probe(struct platform_device *device)
>  	} else {
>  		dev_warn(&device->dev,
>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> +		if (!smo8800->irq)
> +			return -ENODEV;
>  	}
>  
> +	platform_set_drvdata(device, smo8800);
>  	return 0;
>  
>  error_free_irq:
> -	free_irq(smo8800->irq, smo8800);
> +	if (smo8800->irq) {
> +		free_irq(smo8800->irq, smo8800);
>  error:
> -	misc_deregister(&smo8800->miscdev);
> +		misc_deregister(&smo8800->miscdev);
> +	}
> +
>  	return err;
>  }
>  
> @@ -371,9 +376,11 @@ static void smo8800_remove(struct platform_device *device)
>  		i2c_unregister_device(smo8800->i2c_dev);
>  	}
>  
> -	free_irq(smo8800->irq, smo8800);
> -	misc_deregister(&smo8800->miscdev);
> -	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
> +	if (smo8800->irq) {
> +		free_irq(smo8800->irq, smo8800);
> +		misc_deregister(&smo8800->miscdev);
> +		dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
> +	}
>  }
>  
>  static const struct acpi_device_id smo8800_ids[] = {
> -- 
> 2.45.1
> 

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

[Pali Rohár]

On Friday 21 June 2024 14:25:01 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.

This is statement which I got from Dell for 10 years old Dell models.

I have already stated that poking of address in kernel is a big risk
specially for all current and any future dell hardware. Hiding it just
under the kernel parameter is still a risk, specially as neither its
name, nor description say that it is dangerous.

But anyway, why this code is being introduced? Have you communicated
with Dell about this problem?

> At support to have the kernel probe for the i2c-address for modesl
> which are not on the list.
> 
> The new probing code sits behind a new probe_i2c_addr module parameter,
> which is disabled by default because probing might be dangerous.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell/dell-smo8800.c | 152 ++++++++++++++++++++++-
>  1 file changed, 147 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> index 4c79b2599d96..d64d200e927a 100644
> --- a/drivers/platform/x86/dell/dell-smo8800.c
> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #define DRIVER_NAME "smo8800"
> +#define LIS3_WHO_AM_I 0x0f
>  
>  #include <linux/device/bus.h>
>  #include <linux/dmi.h>
> @@ -25,6 +26,10 @@
>  #include <linux/uaccess.h>
>  #include <linux/workqueue.h>
>  
> +static bool probe_i2c_addr;
> +module_param(probe_i2c_addr, bool, 0444);
> +MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown");
> +
>  struct smo8800_device {
>  	u32 irq;                     /* acpi device irq */
>  	atomic_t counter;            /* count after last read */
> @@ -225,6 +230,130 @@ static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
>  	{ }
>  };
>  
> +/*
> + * 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 device *dev, struct i2c_adapter *adap, u8 addr)
> +{
> +	union i2c_smbus_data smbus_data;
> +	int err;
> +	u8 data;
> +
> +	/*
> +	 * First receive a byte from the chip, and remember it. This
> +	 * also checks if there is a device at the address at all.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +			     I2C_SMBUS_BYTE, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	data = smbus_data.byte;
> +
> +	/*
> +	 * Receive a byte again; very likely to be the same for
> +	 * 1-register-only devices.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +			     I2C_SMBUS_BYTE, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != data)
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Then try a standard byte read, with a register offset equal to
> +	 * the read byte; for 1-register-only device this should read
> +	 * the same byte value in return.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != data)
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Then try a standard byte read, with a slightly different register
> +	 * offset; this should again read the register offset in return.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != (data ^ 0x01))
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Apparently this is a 1-register-only device, restore the original
> +	 * register value and leave it alone.
> +	 */
> +	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
> +		       I2C_SMBUS_BYTE, NULL);
> +	dev_warn(dev, "I2C safety check for address 0x%02x failed, skipping\n", addr);
> +	return -ENODEV;
> +}
> +
> +static int smo8800_detect_accel(struct smo8800_device *smo8800,
> +				struct i2c_adapter *adap, u8 addr,
> +				struct i2c_board_info *info, bool probe)
> +{
> +	union i2c_smbus_data smbus_data;
> +	const char *type;
> +	int err;
> +
> +	if (probe) {
> +		dev_info(smo8800->dev, "Probing for accelerometer on address 0x%02x\n", addr);
> +		err = i2c_safety_check(smo8800->dev, 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) {
> +		dev_warn(smo8800->dev, "Failed to read who-am-i register: %d\n", err);
> +		return err;
> +	}
> +
> +	/* who-am-i register mappings from drivers/misc/lis3lv02d/lis3lv02d.c */
> +	switch (smbus_data.byte) {
> +	case 0x32:
> +		type = "lis331dlh";
> +		break;
> +	case 0x33:
> +		type = "lis2de12"; /* LIS3DC / HP3DC in drivers/misc/lis3lv02d/lis3lv02d.c */
> +		break;
> +	case 0x3a:
> +		type = "lis3lv02dl_accel";
> +		break;
> +	case 0x3b:
> +		type = "lis302dl";
> +		break;
> +	default:
> +		dev_warn(smo8800->dev, "Unknown who-am-i register value 0x%02x\n",
> +			 smbus_data.byte);
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(smo8800->dev, "Detected %s accelerometer on address 0x%02x\n", type, addr);
> +	strscpy(info->type, "lis3lv02d", I2C_NAME_SIZE);
> +	info->addr = addr;
> +	return 0;
> +}
> +
>  static int smo8800_find_i801(struct device *dev, void *data)
>  {
>  	struct i2c_adapter *adap, **adap_ret = data;
> @@ -247,6 +376,7 @@ static void smo8800_instantiate_i2c_client(struct work_struct *work)
>  	const struct dmi_system_id *lis3lv02d_dmi_id;
>  	struct i2c_board_info info = { };
>  	struct i2c_adapter *adap = NULL;
> +	int err;
>  
>  	if (smo8800->i2c_dev)
>  		return;
> @@ -256,11 +386,22 @@ static void smo8800_instantiate_i2c_client(struct work_struct *work)
>  		return;
>  
>  	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);
> -	if (!lis3lv02d_dmi_id)
> +	if (lis3lv02d_dmi_id) {
> +		info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +		/* Always detect the accel-type, this also checks the accel is actually there */
> +		err = smo8800_detect_accel(smo8800, adap, info.addr, &info, false);
> +		if (err)
> +			goto out_put_adapter;
> +	} else if (probe_i2c_addr) {
> +		/* First try address 0x29 (most used) and then try 0x1d */
> +		if (smo8800_detect_accel(smo8800, adap, 0x29, &info, true) != 0 &&
> +		    smo8800_detect_accel(smo8800, adap, 0x1d, &info, true) != 0) {
> +			dev_warn(smo8800->dev, "failed to probe for lis3lv02d I2C address\n");
> +			goto out_put_adapter;
> +		}
> +	} else {
>  		goto out_put_adapter;
> -
> -	info.addr = (long)lis3lv02d_dmi_id->driver_data;
> -	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> +	}
>  
>  	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
>  	if (IS_ERR(smo8800->i2c_dev)) {
> @@ -357,7 +498,7 @@ static int smo8800_probe(struct platform_device *device)
>  			 smo8800->irq);
>  	}
>  
> -	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
> +	if (dmi_check_system(smo8800_lis3lv02d_devices) || probe_i2c_addr) {
>  		/*
>  		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
>  		 * i2c_client instantiation.
> @@ -370,6 +511,7 @@ static int smo8800_probe(struct platform_device *device)
>  	} else {
>  		dev_warn(&device->dev,
>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> +		dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
>  		if (!smo8800->irq)
>  			return -ENODEV;
>  	}
> -- 
> 2.45.1
> 

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

[Hans de Goede]

Hi,

On 6/22/24 2:46 PM, Pali Rohár wrote:
> On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
>> 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>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index d2d2a6dbe29f..5ac5bbd60d45 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>  
>>  	i801_add_tco(priv);
>>  
>> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>> -		"SMBus I801 adapter at %04lx", priv->smba);
>> +	if (priv->features & FEATURE_IDF)
>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
>> +	else
>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>> +			"SMBus I801 adapter at %04lx", priv->smba);
>> +
> 
> User visible name is identifier for user / human.
> 
> If somebody is going to read this code in next 10 years then can ask
> question why to have different name for IDF FEATURE and not also for
> other features? And can come to conclusion to unify all names to be
> same (why not? it is user identifier).

That is a good point, I'll add a comment about this for the next
version.

> Depending on user names between different kernel subsystem is fragile,
> specially for future as rename can happen.

Relying no devices names to find devices is standard practice. E.g.
this is how 99% of the platform drivers bind to platform devices
by the driver and the device having the same name.

> If you are depending on FEATURE_IDF flag then check for the flag
> directly, and not hiding the flag by serializing it into the user
> visible name (char[] variable) and then de-serializing it in different
> kernel subsystem. If the flag is not exported yet then export it via
> some function or other API.

Exporting this through some new function is non trivial and adds
extra dependencies between modules, causing issues when one is builtin
and the other is build as a module.

The name check OTOH allows the modules to be less tightly coupled
and there is plenty of precedence for using a name check here.

Regards,

Hans



>>  	err = i2c_add_adapter(&priv->adapter);
>>  	if (err) {
>>  		platform_device_unregister(priv->tco_pdev);
>> -- 
>> 2.45.1
>>
> 

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

[Hans de Goede]

Hi Andy,

On 6/21/24 5:24 PM, Andy Shevchenko wrote:
> On Fri, Jun 21, 2024 at 2:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> It is not necessary to handle the Dell specific instantiation of
>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
>> inside the generic i801 I2C adapter driver.
>>
>> The kernel already instantiates platform_device-s for these ACPI devices
>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
>> platform drivers.
>>
>> Move the i2c_client instantiation from the generic i2c-i801 driver to
>> the SMO88xx specific dell-smo8800 driver.
>>
>> Moving the i2c_client instantiation here 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 the SMO88xx
>> specific dell-smo8800 module where it belongs.
>>
>> 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-smo8800 driver, without needing to modify
>> the i2c-i801 code.
> 
> ...
> 
> 
>> +static int smo8800_find_i801(struct device *dev, void *data)
>> +{
>> +       struct i2c_adapter *adap, **adap_ret = data;
>> +
>> +       adap = i2c_verify_adapter(dev);
>> +       if (!adap)
>> +               return 0;
>> +
>> +       if (!strstarts(adap->name, "SMBus I801 adapter"))
> 
> With the comment on the previous patch I'm wondering if it makes sense
> to have this to be as simple as strstr("I801") or strstr("I801 IDF")?

We want the non IDF one, strstr("I801") would match both and
strstr("I801 IDF") would match the one we don't want.

> 
>> +               return 0;
>> +
>> +       *adap_ret = i2c_get_adapter(adap->nr);
>> +       return 1;
>> +}
> 
> ...
> 
>> +       info.addr = (long)lis3lv02d_dmi_id->driver_data;
> 
> Hmm... Usually we use uintptr_t, but okay.
> 
> ...
> 
>> +               if (strstarts(adap->name, "SMBus I801 adapter"))
> 
> A dup? Is there a possibility it may go desynchronized?

That is a good point I'll add a small i2c_adapter_is_main_i801(adap)
helper for this for the next version.

Regards,

Hans

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

[Hans de Goede]

Hi Pali,

On 6/22/24 3:16 PM, Pali Rohár wrote:
> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
>> It is not necessary to handle the Dell specific instantiation of
>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
>> inside the generic i801 I2C adapter driver.
>>
>> The kernel already instantiates platform_device-s for these ACPI devices
>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
>> platform drivers.
>>
>> Move the i2c_client instantiation from the generic i2c-i801 driver to
>> the SMO88xx specific dell-smo8800 driver.
> 
> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
> and freefall code for smo88xx are basically independent.
> 
> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
> detection. The only thing which have these "drivers" common is the ACPI
> detection mechanism based on presence of SMO88?? identifiers from
> acpi_smo8800_ids[] array.
> 
> I think it makes both "drivers" cleaner if they are put into separate
> files as they are independent of each one.
> 
> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
> instead (or similar name)? And just share list of ACPI ids via some
> header file (or something like that).

Interesting idea, but that will not work, only 1 driver can bind to
the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.

>> Moving the i2c_client instantiation here 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 the SMO88xx
>> specific dell-smo8800 module where it belongs.
>>
>> 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-smo8800 driver, without needing to modify
>> the i2c-i801 code.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note the goto out_put_adapter, which can be avoided by moving the DMI check
>> up, is there deliberately as preparation for adding support to probe for
>> the i2c address in case there is no DMI match.
>> ---
>> 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/dell-smo8800.c | 214 ++++++++++++++++++++++-
>>  2 files changed, 213 insertions(+), 125 deletions(-)

<snip>

>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
>> index f7ec17c56833..cd2e48405859 100644
>> --- a/drivers/platform/x86/dell/dell-smo8800.c
>> +++ b/drivers/platform/x86/dell/dell-smo8800.c

...

>> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
>>  	.release = smo8800_misc_release,
>>  };
>>  
>> +/*
>> + * 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 smo8800_lis3lv02d_devices[] = {
>> +	/*
>> +	 * Dell platform team told us that these Latitude devices have
>> +	 * ST microelectronics accelerometer at I2C address 0x29.
>> +	 */
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	/*
>> +	 * Additional individual entries were added after verification.
>> +	 */
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
>> +		},
>> +		.driver_data = (void *)0x1dL,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
>> +		},
>> +		.driver_data = (void *)0x29L,
>> +	},
>> +	{
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
>> +		},
>> +		.driver_data = (void *)0x29L,
> 
> At least for me, casting i2c address to LONG and then to pointer looks
> very strange. If I look at this code without knowing what the number
> 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
> address.
> 
> Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
> instead of ".something = (void *)0x29L" to make it readable?

struct dmi_system_id is an existing structure and we cannot just go adding
fields to it. driver_data is intended to tie driver specific data to
each DMI match, often pointing to some struct, so it is a void *, but
in this case we only need a single integer, so we store that in the
pointer. That is is the address becomes obvious when looking at the code
which consumes the data.

> Also does the whole device table has to be such verbose with lot of
> duplicated information (which probably also increase size of every linux
> image which includes this driver into it)?

struct dmi_system_id is the default way to specify DMI matches in
the kernel. This avoids code duplication in the form of writing
a DYI function to do the matching.

In v2 of the patch-set I only matched on product-name, but you asked
in the review of v2 to also match on sys-vendor and you mentioned
we may want to support other sys-vendors too, since some other brands
have SMO88xx ACPI devices too. This more or less automatically leads
to using the kernel's standard, existing, DMI matching mechanism.

We really want to avoid coming up with something "new" ourselves here
leading to unnecessary code duplication.

Regards,

Hans

Re: [PATCH v3 4/6] platform/x86: dell-smo8800: Allow lis3lv02d i2c_client instantiation without IRQ

[Hans de Goede]

Hi Pali,

On 6/22/24 3:20 PM, Pali Rohár wrote:
> On Friday 21 June 2024 14:24:59 Hans de Goede wrote:
>> The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5"
>> sata disk; or a 90Wh battery in which case the battery occupies the space
>> for the optional 2.5" sata disk.
>>
>> On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS
>> does not add an IRQ resource to the SMO8810 ACPI device.
> 
> That is a pity, but OK, manufacturer decided that freefall sensor is
> enabled by BIOS firmware only if the SATA is present.
> 
>> Make the misc-device registration and the requesting of the IRQ optional
>> and instantiate a lis3lv02d i2c_client independent of the IRQ being there,
>> so that the non freefall lis3lv02d functionality can still be used.
>>
>> Note that IRQ 0 is not a valid IRQ number for platform IRQs
>> and this patch relies on that.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/dell/dell-smo8800.c | 67 +++++++++++++-----------
>>  1 file changed, 37 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
>> index cd2e48405859..2e49bbb569c6 100644
>> --- a/drivers/platform/x86/dell/dell-smo8800.c
>> +++ b/drivers/platform/x86/dell/dell-smo8800.c
>> @@ -310,33 +310,32 @@ static int smo8800_probe(struct platform_device *device)
>>  	init_waitqueue_head(&smo8800->misc_wait);
>>  	INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client);
>>  
>> -	err = misc_register(&smo8800->miscdev);
>> -	if (err) {
>> -		dev_err(&device->dev, "failed to register misc dev: %d\n", err);
>> -		return err;
>> +	err = platform_get_irq_optional(device, 0);
>> +	if (err > 0)
>> +		smo8800->irq = err;
> 
> This code should be rather change to fail immediately. If the IRQ number
> is not provided by the BIOS then the ACPI SMO88xx is not usable for us
> at all. So return error from the smo8800_probe function.

The goal of this patch is to still register the bus-notifier for i2c-client
instantiation for the lis3lv02d driver. Existing immediately here (as was
done before) means we will still not register the bus-notifier.

Regards,

Hans





>> +
>> +	if (smo8800->irq) {
>> +		err = misc_register(&smo8800->miscdev);
>> +		if (err) {
>> +			dev_err(&device->dev, "failed to register misc dev: %d\n", err);
>> +			return err;
>> +		}
>> +
>> +		err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
>> +					   smo8800_interrupt_thread,
>> +					   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> +					   DRIVER_NAME, smo8800);
>> +		if (err) {
>> +			dev_err(&device->dev,
>> +				"failed to request thread for IRQ %d: %d\n",
>> +				smo8800->irq, err);
>> +			goto error;
>> +		}
>> +
>> +		dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
>> +			 smo8800->irq);
>>  	}
>>  
>> -	platform_set_drvdata(device, smo8800);
>> -
>> -	err = platform_get_irq(device, 0);
>> -	if (err < 0)
>> -		goto error;
>> -	smo8800->irq = err;
>> -
>> -	err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
>> -				   smo8800_interrupt_thread,
>> -				   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> -				   DRIVER_NAME, smo8800);
>> -	if (err) {
>> -		dev_err(&device->dev,
>> -			"failed to request thread for IRQ %d: %d\n",
>> -			smo8800->irq, err);
>> -		goto error;
>> -	}
>> -
>> -	dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
>> -		 smo8800->irq);
>> -
>>  	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
>>  		/*
>>  		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
>> @@ -350,14 +349,20 @@ static int smo8800_probe(struct platform_device *device)
>>  	} else {
>>  		dev_warn(&device->dev,
>>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
>> +		if (!smo8800->irq)
>> +			return -ENODEV;
>>  	}
>>  
>> +	platform_set_drvdata(device, smo8800);
>>  	return 0;
>>  
>>  error_free_irq:
>> -	free_irq(smo8800->irq, smo8800);
>> +	if (smo8800->irq) {
>> +		free_irq(smo8800->irq, smo8800);
>>  error:
>> -	misc_deregister(&smo8800->miscdev);
>> +		misc_deregister(&smo8800->miscdev);
>> +	}
>> +
>>  	return err;
>>  }
>>  
>> @@ -371,9 +376,11 @@ static void smo8800_remove(struct platform_device *device)
>>  		i2c_unregister_device(smo8800->i2c_dev);
>>  	}
>>  
>> -	free_irq(smo8800->irq, smo8800);
>> -	misc_deregister(&smo8800->miscdev);
>> -	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
>> +	if (smo8800->irq) {
>> +		free_irq(smo8800->irq, smo8800);
>> +		misc_deregister(&smo8800->miscdev);
>> +		dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
>> +	}
>>  }
>>  
>>  static const struct acpi_device_id smo8800_ids[] = {
>> -- 
>> 2.45.1
>>
> 

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

[Pali Rohár]

On Saturday 22 June 2024 15:56:03 Hans de Goede wrote:
> Hi,
> 
> On 6/22/24 2:46 PM, Pali Rohár wrote:
> > On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
> >> 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>
> >> ---
> >>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >> index d2d2a6dbe29f..5ac5bbd60d45 100644
> >> --- a/drivers/i2c/busses/i2c-i801.c
> >> +++ b/drivers/i2c/busses/i2c-i801.c
> >> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>  
> >>  	i801_add_tco(priv);
> >>  
> >> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >> -		"SMBus I801 adapter at %04lx", priv->smba);
> >> +	if (priv->features & FEATURE_IDF)
> >> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
> >> +	else
> >> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >> +			"SMBus I801 adapter at %04lx", priv->smba);
> >> +
> > 
> > User visible name is identifier for user / human.
> > 
> > If somebody is going to read this code in next 10 years then can ask
> > question why to have different name for IDF FEATURE and not also for
> > other features? And can come to conclusion to unify all names to be
> > same (why not? it is user identifier).
> 
> That is a good point, I'll add a comment about this for the next
> version.
> 
> > Depending on user names between different kernel subsystem is fragile,
> > specially for future as rename can happen.
> 
> Relying no devices names to find devices is standard practice. E.g.
> this is how 99% of the platform drivers bind to platform devices
> by the driver and the device having the same name.

But here it is adapter name which is more likely description, not the
device name which is used for binding.

> > If you are depending on FEATURE_IDF flag then check for the flag
> > directly, and not hiding the flag by serializing it into the user
> > visible name (char[] variable) and then de-serializing it in different
> > kernel subsystem. If the flag is not exported yet then export it via
> > some function or other API.
> 
> Exporting this through some new function is non trivial and adds
> extra dependencies between modules, causing issues when one is builtin
> and the other is build as a module.

Access to "struct i801_priv *" is not possible? For example via
dev_get_drvdata() on "struct device *" which you have in
smo8800_find_i801()?

Because if it is possible then you can create an inline function in some
shared header file which access this flag. Not perfect (as accessing
private data is not the best thing) but can avoid dependences between
modules.

> The name check OTOH allows the modules to be less tightly coupled
> and there is plenty of precedence for using a name check here.
> 
> Regards,
> 
> Hans
> 
> 
> 
> >>  	err = i2c_add_adapter(&priv->adapter);
> >>  	if (err) {
> >>  		platform_device_unregister(priv->tco_pdev);
> >> -- 
> >> 2.45.1
> >>
> > 
> 

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

[Hans de Goede]

Hi Pali,

On 6/22/24 4:08 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 15:56:03 Hans de Goede wrote:
>> Hi,
>>
>> On 6/22/24 2:46 PM, Pali Rohár wrote:
>>> On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
>>>> 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>
>>>> ---
>>>>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>> index d2d2a6dbe29f..5ac5bbd60d45 100644
>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>>  
>>>>  	i801_add_tco(priv);
>>>>  
>>>> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>> -		"SMBus I801 adapter at %04lx", priv->smba);
>>>> +	if (priv->features & FEATURE_IDF)
>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
>>>> +	else
>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>> +			"SMBus I801 adapter at %04lx", priv->smba);
>>>> +
>>>
>>> User visible name is identifier for user / human.
>>>
>>> If somebody is going to read this code in next 10 years then can ask
>>> question why to have different name for IDF FEATURE and not also for
>>> other features? And can come to conclusion to unify all names to be
>>> same (why not? it is user identifier).
>>
>> That is a good point, I'll add a comment about this for the next
>> version.
>>
>>> Depending on user names between different kernel subsystem is fragile,
>>> specially for future as rename can happen.
>>
>> Relying no devices names to find devices is standard practice. E.g.
>> this is how 99% of the platform drivers bind to platform devices
>> by the driver and the device having the same name.
> 
> But here it is adapter name which is more likely description, not the
> device name which is used for binding.

It is still matching on a name.

>>> If you are depending on FEATURE_IDF flag then check for the flag
>>> directly, and not hiding the flag by serializing it into the user
>>> visible name (char[] variable) and then de-serializing it in different
>>> kernel subsystem. If the flag is not exported yet then export it via
>>> some function or other API.
>>
>> Exporting this through some new function is non trivial and adds
>> extra dependencies between modules, causing issues when one is builtin
>> and the other is build as a module.
> 
> Access to "struct i801_priv *" is not possible? For example via
> dev_get_drvdata() on "struct device *" which you have in
> smo8800_find_i801()?
> 
> Because if it is possible then you can create an inline function in some
> shared header file which access this flag. Not perfect (as accessing
> private data is not the best thing) but can avoid dependences between
> modules.

Prodding inside another drivers private driver struct is a big nono
and much much more fragile then the name checking.

Regards,

Hans

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

[Pali Rohár]

On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 3:16 PM, Pali Rohár wrote:
> > On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> >> It is not necessary to handle the Dell specific instantiation of
> >> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
> >> inside the generic i801 I2C adapter driver.
> >>
> >> The kernel already instantiates platform_device-s for these ACPI devices
> >> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> >> platform drivers.
> >>
> >> Move the i2c_client instantiation from the generic i2c-i801 driver to
> >> the SMO88xx specific dell-smo8800 driver.
> > 
> > Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
> > and freefall code for smo88xx are basically independent.
> > 
> > lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
> > detection. The only thing which have these "drivers" common is the ACPI
> > detection mechanism based on presence of SMO88?? identifiers from
> > acpi_smo8800_ids[] array.
> > 
> > I think it makes both "drivers" cleaner if they are put into separate
> > files as they are independent of each one.
> > 
> > What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
> > instead (or similar name)? And just share list of ACPI ids via some
> > header file (or something like that).
> 
> Interesting idea, but that will not work, only 1 driver can bind to
> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.

And it is required to bind lis3 device to ACPI code? What is needed is
just to check if system matches DMI strings and ACPI strings. You are
not binding device to DMI strings, so I think there is no need to bind
it neither to ACPI strings.

> >> Moving the i2c_client instantiation here 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 the SMO88xx
> >> specific dell-smo8800 module where it belongs.
> >>
> >> 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-smo8800 driver, without needing to modify
> >> the i2c-i801 code.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Note the goto out_put_adapter, which can be avoided by moving the DMI check
> >> up, is there deliberately as preparation for adding support to probe for
> >> the i2c address in case there is no DMI match.
> >> ---
> >> 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/dell-smo8800.c | 214 ++++++++++++++++++++++-
> >>  2 files changed, 213 insertions(+), 125 deletions(-)
> 
> <snip>
> 
> >> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> >> index f7ec17c56833..cd2e48405859 100644
> >> --- a/drivers/platform/x86/dell/dell-smo8800.c
> >> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> 
> ...
> 
> >> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
> >>  	.release = smo8800_misc_release,
> >>  };
> >>  
> >> +/*
> >> + * 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 smo8800_lis3lv02d_devices[] = {
> >> +	/*
> >> +	 * Dell platform team told us that these Latitude devices have
> >> +	 * ST microelectronics accelerometer at I2C address 0x29.
> >> +	 */
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	/*
> >> +	 * Additional individual entries were added after verification.
> >> +	 */
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> >> +		},
> >> +		.driver_data = (void *)0x1dL,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> > 
> > At least for me, casting i2c address to LONG and then to pointer looks
> > very strange. If I look at this code without knowing what the number
> > 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
> > address.
> > 
> > Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
> > instead of ".something = (void *)0x29L" to make it readable?
> 
> struct dmi_system_id is an existing structure and we cannot just go adding
> fields to it. driver_data is intended to tie driver specific data to
> each DMI match, often pointing to some struct, so it is a void *, but

Yes, I know it.

> in this case we only need a single integer, so we store that in the
> pointer. That is is the address becomes obvious when looking at the code
> which consumes the data.

Ok, this makes sense. Anyway, is explicit void* cast and L suffix
required?

> > Also does the whole device table has to be such verbose with lot of
> > duplicated information (which probably also increase size of every linux
> > image which includes this driver into it)?
> 
> struct dmi_system_id is the default way to specify DMI matches in
> the kernel. This avoids code duplication in the form of writing
> a DYI function to do the matching.
> 
> In v2 of the patch-set I only matched on product-name, but you asked
> in the review of v2 to also match on sys-vendor and you mentioned
> we may want to support other sys-vendors too, since some other brands
> have SMO88xx ACPI devices too. This more or less automatically leads
> to using the kernel's standard, existing, DMI matching mechanism.
> 
> We really want to avoid coming up with something "new" ourselves here
> leading to unnecessary code duplication.
> 
> Regards,
> 
> Hans

Ok, then let that table as you have it now.

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

[Hans de Goede]

Hi Pali,

On 6/22/24 3:32 PM, Pali Rohár wrote:
> On Friday 21 June 2024 14:25:01 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.
> 
> This is statement which I got from Dell for 10 years old Dell models.
> 
> I have already stated that poking of address in kernel is a big risk
> specially for all current and any future dell hardware. Hiding it just
> under the kernel parameter is still a risk, specially as neither its
> name, nor description say that it is dangerous:

I think you are overstating how dangerous this is. lm-sensors detect
scripts has been poking i2c addresses for years without problems (1) and
still does so till today.

Besides the kernel message telling users about this option does mention that
it is dangerous:

>> @@ -370,6 +511,7 @@ static int smo8800_probe(struct platform_device *device)
>>  	} else {
>>  		dev_warn(&device->dev,
>>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
>> +		dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
>>  		if (!smo8800->irq)
>>  			return -ENODEV;
>>  	}

> But anyway, why this code is being introduced?

Because users have been asking about an easier way to find the address for
not yet supported Dell models:

https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/

This is the whole reason why I started working on this patch-set in
the first place.

> Have you communicated
> with Dell about this problem?

Dell is on the Cc of this thread, as well as the previous v2 posting:

Cc: Dell.Client.Kernel@dell.com

Regards,

Hans


1) There were some problems more then a decade ago, but those were only
at specific addresses on some really old (by now) ThinkPads and for
the other case the i2c_safety_check() function was added.

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

[Pali Rohár]

On Saturday 22 June 2024 16:14:11 Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 4:08 PM, Pali Rohár wrote:
> > On Saturday 22 June 2024 15:56:03 Hans de Goede wrote:
> >> Hi,
> >>
> >> On 6/22/24 2:46 PM, Pali Rohár wrote:
> >>> On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
> >>>> 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>
> >>>> ---
> >>>>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
> >>>>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >>>> index d2d2a6dbe29f..5ac5bbd60d45 100644
> >>>> --- a/drivers/i2c/busses/i2c-i801.c
> >>>> +++ b/drivers/i2c/busses/i2c-i801.c
> >>>> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>>>  
> >>>>  	i801_add_tco(priv);
> >>>>  
> >>>> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >>>> -		"SMBus I801 adapter at %04lx", priv->smba);
> >>>> +	if (priv->features & FEATURE_IDF)
> >>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >>>> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
> >>>> +	else
> >>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >>>> +			"SMBus I801 adapter at %04lx", priv->smba);
> >>>> +
> >>>
> >>> User visible name is identifier for user / human.
> >>>
> >>> If somebody is going to read this code in next 10 years then can ask
> >>> question why to have different name for IDF FEATURE and not also for
> >>> other features? And can come to conclusion to unify all names to be
> >>> same (why not? it is user identifier).
> >>
> >> That is a good point, I'll add a comment about this for the next
> >> version.
> >>
> >>> Depending on user names between different kernel subsystem is fragile,
> >>> specially for future as rename can happen.
> >>
> >> Relying no devices names to find devices is standard practice. E.g.
> >> this is how 99% of the platform drivers bind to platform devices
> >> by the driver and the device having the same name.
> > 
> > But here it is adapter name which is more likely description, not the
> > device name which is used for binding.
> 
> It is still matching on a name.
> 
> >>> If you are depending on FEATURE_IDF flag then check for the flag
> >>> directly, and not hiding the flag by serializing it into the user
> >>> visible name (char[] variable) and then de-serializing it in different
> >>> kernel subsystem. If the flag is not exported yet then export it via
> >>> some function or other API.
> >>
> >> Exporting this through some new function is non trivial and adds
> >> extra dependencies between modules, causing issues when one is builtin
> >> and the other is build as a module.
> > 
> > Access to "struct i801_priv *" is not possible? For example via
> > dev_get_drvdata() on "struct device *" which you have in
> > smo8800_find_i801()?
> > 
> > Because if it is possible then you can create an inline function in some
> > shared header file which access this flag. Not perfect (as accessing
> > private data is not the best thing) but can avoid dependences between
> > modules.
> 
> Prodding inside another drivers private driver struct is a big nono
> and much much more fragile then the name checking.

I know, that is why I wrote to access this structure and flags in
separate function which can be an inline in e.g. i2c-i801.h header file.

> Regards,
> 
> Hans
> 
> 

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

[Hans de Goede]

Hi Pali,

On 6/22/24 4:20 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
>> Hi Pali,
>>
>> On 6/22/24 3:16 PM, Pali Rohár wrote:
>>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
>>>> It is not necessary to handle the Dell specific instantiation of
>>>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
>>>> inside the generic i801 I2C adapter driver.
>>>>
>>>> The kernel already instantiates platform_device-s for these ACPI devices
>>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
>>>> platform drivers.
>>>>
>>>> Move the i2c_client instantiation from the generic i2c-i801 driver to
>>>> the SMO88xx specific dell-smo8800 driver.
>>>
>>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
>>> and freefall code for smo88xx are basically independent.
>>>
>>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
>>> detection. The only thing which have these "drivers" common is the ACPI
>>> detection mechanism based on presence of SMO88?? identifiers from
>>> acpi_smo8800_ids[] array.
>>>
>>> I think it makes both "drivers" cleaner if they are put into separate
>>> files as they are independent of each one.
>>>
>>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
>>> instead (or similar name)? And just share list of ACPI ids via some
>>> header file (or something like that).
>>
>> Interesting idea, but that will not work, only 1 driver can bind to
>> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
> 
> And it is required to bind lis3 device to ACPI code? What is needed is
> just to check if system matches DMI strings and ACPI strings. You are
> not binding device to DMI strings, so I think there is no need to bind
> it neither to ACPI strings.

The driver needs to bind to something ...

This is code for special handling required for SMO88xx ACPI devices,
dell-smo8800 is *the* driver for those ACPI devices. So this code clearly
belongs here.

According to diffstat this adds about 400 lines of code that is really not
that big, so I see no urgent reason to introduce weird tricks instead of
standard driver binding for this.

Regards,

Hans






> 
>>>> Moving the i2c_client instantiation here 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 the SMO88xx
>>>> specific dell-smo8800 module where it belongs.
>>>>
>>>> 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-smo8800 driver, without needing to modify
>>>> the i2c-i801 code.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Note the goto out_put_adapter, which can be avoided by moving the DMI check
>>>> up, is there deliberately as preparation for adding support to probe for
>>>> the i2c address in case there is no DMI match.
>>>> ---
>>>> 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/dell-smo8800.c | 214 ++++++++++++++++++++++-
>>>>  2 files changed, 213 insertions(+), 125 deletions(-)
>>
>> <snip>
>>
>>>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
>>>> index f7ec17c56833..cd2e48405859 100644
>>>> --- a/drivers/platform/x86/dell/dell-smo8800.c
>>>> +++ b/drivers/platform/x86/dell/dell-smo8800.c
>>
>> ...
>>
>>>> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
>>>>  	.release = smo8800_misc_release,
>>>>  };
>>>>  
>>>> +/*
>>>> + * 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 smo8800_lis3lv02d_devices[] = {
>>>> +	/*
>>>> +	 * Dell platform team told us that these Latitude devices have
>>>> +	 * ST microelectronics accelerometer at I2C address 0x29.
>>>> +	 */
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	/*
>>>> +	 * Additional individual entries were added after verification.
>>>> +	 */
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
>>>> +		},
>>>> +		.driver_data = (void *)0x1dL,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>
>>> At least for me, casting i2c address to LONG and then to pointer looks
>>> very strange. If I look at this code without knowing what the number
>>> 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
>>> address.
>>>
>>> Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
>>> instead of ".something = (void *)0x29L" to make it readable?
>>
>> struct dmi_system_id is an existing structure and we cannot just go adding
>> fields to it. driver_data is intended to tie driver specific data to
>> each DMI match, often pointing to some struct, so it is a void *, but
> 
> Yes, I know it.
> 
>> in this case we only need a single integer, so we store that in the
>> pointer. That is is the address becomes obvious when looking at the code
>> which consumes the data.
> 
> Ok, this makes sense. Anyway, is explicit void* cast and L suffix
> required?
> 
>>> Also does the whole device table has to be such verbose with lot of
>>> duplicated information (which probably also increase size of every linux
>>> image which includes this driver into it)?
>>
>> struct dmi_system_id is the default way to specify DMI matches in
>> the kernel. This avoids code duplication in the form of writing
>> a DYI function to do the matching.
>>
>> In v2 of the patch-set I only matched on product-name, but you asked
>> in the review of v2 to also match on sys-vendor and you mentioned
>> we may want to support other sys-vendors too, since some other brands
>> have SMO88xx ACPI devices too. This more or less automatically leads
>> to using the kernel's standard, existing, DMI matching mechanism.
>>
>> We really want to avoid coming up with something "new" ourselves here
>> leading to unnecessary code duplication.
>>
>> Regards,
>>
>> Hans
> 
> Ok, then let that table as you have it now.
> 

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

[Hans de Goede]

Hi,

On 6/22/24 4:23 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 16:14:11 Hans de Goede wrote:
>> Hi Pali,
>>
>> On 6/22/24 4:08 PM, Pali Rohár wrote:
>>> On Saturday 22 June 2024 15:56:03 Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 6/22/24 2:46 PM, Pali Rohár wrote:
>>>>> On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
>>>>>> 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>
>>>>>> ---
>>>>>>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
>>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>>>> index d2d2a6dbe29f..5ac5bbd60d45 100644
>>>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>>>> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>>>>  
>>>>>>  	i801_add_tco(priv);
>>>>>>  
>>>>>> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>> -		"SMBus I801 adapter at %04lx", priv->smba);
>>>>>> +	if (priv->features & FEATURE_IDF)
>>>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
>>>>>> +	else
>>>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>> +			"SMBus I801 adapter at %04lx", priv->smba);
>>>>>> +
>>>>>
>>>>> User visible name is identifier for user / human.
>>>>>
>>>>> If somebody is going to read this code in next 10 years then can ask
>>>>> question why to have different name for IDF FEATURE and not also for
>>>>> other features? And can come to conclusion to unify all names to be
>>>>> same (why not? it is user identifier).
>>>>
>>>> That is a good point, I'll add a comment about this for the next
>>>> version.
>>>>
>>>>> Depending on user names between different kernel subsystem is fragile,
>>>>> specially for future as rename can happen.
>>>>
>>>> Relying no devices names to find devices is standard practice. E.g.
>>>> this is how 99% of the platform drivers bind to platform devices
>>>> by the driver and the device having the same name.
>>>
>>> But here it is adapter name which is more likely description, not the
>>> device name which is used for binding.
>>
>> It is still matching on a name.
>>
>>>>> If you are depending on FEATURE_IDF flag then check for the flag
>>>>> directly, and not hiding the flag by serializing it into the user
>>>>> visible name (char[] variable) and then de-serializing it in different
>>>>> kernel subsystem. If the flag is not exported yet then export it via
>>>>> some function or other API.
>>>>
>>>> Exporting this through some new function is non trivial and adds
>>>> extra dependencies between modules, causing issues when one is builtin
>>>> and the other is build as a module.
>>>
>>> Access to "struct i801_priv *" is not possible? For example via
>>> dev_get_drvdata() on "struct device *" which you have in
>>> smo8800_find_i801()?
>>>
>>> Because if it is possible then you can create an inline function in some
>>> shared header file which access this flag. Not perfect (as accessing
>>> private data is not the best thing) but can avoid dependences between
>>> modules.
>>
>> Prodding inside another drivers private driver struct is a big nono
>> and much much more fragile then the name checking.
> 
> I know, that is why I wrote to access this structure and flags in
> separate function which can be an inline in e.g. i2c-i801.h header file.

We would still need to be very very sure the device we are calling that
function on actually has the i2c-i801.c driver bound to it, so that
e.g. we are not dereferencing a NULL pointer drvdata, or worse
poking at some other drivers private data because we are calling
the helper on the wrong device.

To make sure that is the case we would need to e.g. check that:
a) The device in question is an i2c adapter
b) the adapter name matches, so we would still be doing name matching ...

Really just matching on the adapter name is by far the cleanest
option here.

Regards,

Hans

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

[Pali Rohár]

On Saturday 22 June 2024 16:21:28 Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 3:32 PM, Pali Rohár wrote:
> > On Friday 21 June 2024 14:25:01 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.
> > 
> > This is statement which I got from Dell for 10 years old Dell models.
> > 
> > I have already stated that poking of address in kernel is a big risk
> > specially for all current and any future dell hardware. Hiding it just
> > under the kernel parameter is still a risk, specially as neither its
> > name, nor description say that it is dangerous:
> 
> I think you are overstating how dangerous this is. lm-sensors detect
> scripts has been poking i2c addresses for years without problems (1) and
> still does so till today.
> 
> Besides the kernel message telling users about this option does mention that
> it is dangerous:

But description from modinfo -p and neither parameter name itself does not.
And neither no kernel message when the parameter was enabled.

> >> @@ -370,6 +511,7 @@ static int smo8800_probe(struct platform_device *device)
> >>  	} else {
> >>  		dev_warn(&device->dev,
> >>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> >> +		dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
> >>  		if (!smo8800->irq)
> >>  			return -ENODEV;
> >>  	}
> 
> > But anyway, why this code is being introduced?
> 
> Because users have been asking about an easier way to find the address for
> not yet supported Dell models:
> 
> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
> 
> This is the whole reason why I started working on this patch-set in
> the first place.
> 
> > Have you communicated
> > with Dell about this problem?
> 
> Dell is on the Cc of this thread, as well as the previous v2 posting:
> 
> Cc: Dell.Client.Kernel@dell.com

And what do you think, that if you send tons of long emails with C code
to lot of recipients and add a carbon copy of all those your emails to
some corporate group address, that somebody on that group address will
read every one paragraph of every your email and tries to detect in if
there is not hidden some question to which you want to know answer?
No, with this behavior you are going to be put on the corporate
spam list by automated systems.

This looks like you just wanted to mark your personal checkbox
"I sent email to some dell address" and let it as is.

In past when I sent private email to dell I normally received responses.
Also they said to me that group address is (or at that time was)
monitored.

So it would be nice to start communication with dell and figure out what
is the current state of smbus address detection via ACPI/WMI/DMI/whatever,
instead of adding this hacks via poking of smbus addresses.

And in case the mentioned group address does not work anymore there are
still other linux developers from dell who could be able to figure
something out.

> Regards,
> 
> Hans
> 
> 
> 1) There were some problems more then a decade ago, but those were only
> at specific addresses on some really old (by now) ThinkPads and for
> the other case the i2c_safety_check() function was added.
> 
> 

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

[Pali Rohár]

On Saturday 22 June 2024 16:29:21 Hans de Goede wrote:
> Hi,
> 
> On 6/22/24 4:23 PM, Pali Rohár wrote:
> > On Saturday 22 June 2024 16:14:11 Hans de Goede wrote:
> >> Hi Pali,
> >>
> >> On 6/22/24 4:08 PM, Pali Rohár wrote:
> >>> On Saturday 22 June 2024 15:56:03 Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 6/22/24 2:46 PM, Pali Rohár wrote:
> >>>>> On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
> >>>>>> 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>
> >>>>>> ---
> >>>>>>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
> >>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> >>>>>> index d2d2a6dbe29f..5ac5bbd60d45 100644
> >>>>>> --- a/drivers/i2c/busses/i2c-i801.c
> >>>>>> +++ b/drivers/i2c/busses/i2c-i801.c
> >>>>>> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
> >>>>>>  
> >>>>>>  	i801_add_tco(priv);
> >>>>>>  
> >>>>>> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >>>>>> -		"SMBus I801 adapter at %04lx", priv->smba);
> >>>>>> +	if (priv->features & FEATURE_IDF)
> >>>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >>>>>> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
> >>>>>> +	else
> >>>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
> >>>>>> +			"SMBus I801 adapter at %04lx", priv->smba);
> >>>>>> +
> >>>>>
> >>>>> User visible name is identifier for user / human.
> >>>>>
> >>>>> If somebody is going to read this code in next 10 years then can ask
> >>>>> question why to have different name for IDF FEATURE and not also for
> >>>>> other features? And can come to conclusion to unify all names to be
> >>>>> same (why not? it is user identifier).
> >>>>
> >>>> That is a good point, I'll add a comment about this for the next
> >>>> version.
> >>>>
> >>>>> Depending on user names between different kernel subsystem is fragile,
> >>>>> specially for future as rename can happen.
> >>>>
> >>>> Relying no devices names to find devices is standard practice. E.g.
> >>>> this is how 99% of the platform drivers bind to platform devices
> >>>> by the driver and the device having the same name.
> >>>
> >>> But here it is adapter name which is more likely description, not the
> >>> device name which is used for binding.
> >>
> >> It is still matching on a name.
> >>
> >>>>> If you are depending on FEATURE_IDF flag then check for the flag
> >>>>> directly, and not hiding the flag by serializing it into the user
> >>>>> visible name (char[] variable) and then de-serializing it in different
> >>>>> kernel subsystem. If the flag is not exported yet then export it via
> >>>>> some function or other API.
> >>>>
> >>>> Exporting this through some new function is non trivial and adds
> >>>> extra dependencies between modules, causing issues when one is builtin
> >>>> and the other is build as a module.
> >>>
> >>> Access to "struct i801_priv *" is not possible? For example via
> >>> dev_get_drvdata() on "struct device *" which you have in
> >>> smo8800_find_i801()?
> >>>
> >>> Because if it is possible then you can create an inline function in some
> >>> shared header file which access this flag. Not perfect (as accessing
> >>> private data is not the best thing) but can avoid dependences between
> >>> modules.
> >>
> >> Prodding inside another drivers private driver struct is a big nono
> >> and much much more fragile then the name checking.
> > 
> > I know, that is why I wrote to access this structure and flags in
> > separate function which can be an inline in e.g. i2c-i801.h header file.
> 
> We would still need to be very very sure the device we are calling that
> function on actually has the i2c-i801.c driver bound to it, so that
> e.g. we are not dereferencing a NULL pointer drvdata, or worse
> poking at some other drivers private data because we are calling
> the helper on the wrong device.
> 
> To make sure that is the case we would need to e.g. check that:
> a) The device in question is an i2c adapter
> b) the adapter name matches, so we would still be doing name matching ...
> 
> Really just matching on the adapter name is by far the cleanest
> option here.
> 
> Regards,
> 
> Hans

Ok, I have looked at it now.

For a) you are already using i2c_verify_adapter(). So this part is done.

For b) you do not need to check adapter name, but rather adapter driver
name (in case driver data are begin accessed). And I think you can use
dev->driver.name to get it.

Anyway, I see that pattern "to_<something>_driver" is commonly used.

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

[Pali Rohár]

On Saturday 22 June 2024 16:26:13 Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 4:20 PM, Pali Rohár wrote:
> > On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
> >> Hi Pali,
> >>
> >> On 6/22/24 3:16 PM, Pali Rohár wrote:
> >>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> >>>> It is not necessary to handle the Dell specific instantiation of
> >>>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
> >>>> inside the generic i801 I2C adapter driver.
> >>>>
> >>>> The kernel already instantiates platform_device-s for these ACPI devices
> >>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> >>>> platform drivers.
> >>>>
> >>>> Move the i2c_client instantiation from the generic i2c-i801 driver to
> >>>> the SMO88xx specific dell-smo8800 driver.
> >>>
> >>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
> >>> and freefall code for smo88xx are basically independent.
> >>>
> >>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
> >>> detection. The only thing which have these "drivers" common is the ACPI
> >>> detection mechanism based on presence of SMO88?? identifiers from
> >>> acpi_smo8800_ids[] array.
> >>>
> >>> I think it makes both "drivers" cleaner if they are put into separate
> >>> files as they are independent of each one.
> >>>
> >>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
> >>> instead (or similar name)? And just share list of ACPI ids via some
> >>> header file (or something like that).
> >>
> >> Interesting idea, but that will not work, only 1 driver can bind to
> >> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
> > 
> > And it is required to bind lis3 device to ACPI code? What is needed is
> > just to check if system matches DMI strings and ACPI strings. You are
> > not binding device to DMI strings, so I think there is no need to bind
> > it neither to ACPI strings.
> 
> The driver needs to bind to something ...

Why?

Currently in i2c-i801.c file was called just
register_dell_lis3lv02d_i2c_device() function and there was no binding
to anything, no binding to DMI structure and neither no binding to ACPI
structures.

And if I'm looking correctly at your new function
smo8800_instantiate_i2c_client() it does not bind device neither.
And smo8800_i2c_bus_notify() does not depend on binding.

So I do not see where is that binding requirement.

> This is code for special handling required for SMO88xx ACPI devices,
> dell-smo8800 is *the* driver for those ACPI devices. So this code clearly
> belongs here.
> 
> According to diffstat this adds about 400 lines of code that is really not
> that big, so I see no urgent reason to introduce weird tricks instead of
> standard driver binding for this.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> > 
> >>>> Moving the i2c_client instantiation here 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 the SMO88xx
> >>>> specific dell-smo8800 module where it belongs.
> >>>>
> >>>> 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-smo8800 driver, without needing to modify
> >>>> the i2c-i801 code.
> >>>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>> ---
> >>>> Note the goto out_put_adapter, which can be avoided by moving the DMI check
> >>>> up, is there deliberately as preparation for adding support to probe for
> >>>> the i2c address in case there is no DMI match.
> >>>> ---
> >>>> 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/dell-smo8800.c | 214 ++++++++++++++++++++++-
> >>>>  2 files changed, 213 insertions(+), 125 deletions(-)
> >>
> >> <snip>
> >>
> >>>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> >>>> index f7ec17c56833..cd2e48405859 100644
> >>>> --- a/drivers/platform/x86/dell/dell-smo8800.c
> >>>> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> >>
> >> ...
> >>
> >>>> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
> >>>>  	.release = smo8800_misc_release,
> >>>>  };
> >>>>  
> >>>> +/*
> >>>> + * 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 smo8800_lis3lv02d_devices[] = {
> >>>> +	/*
> >>>> +	 * Dell platform team told us that these Latitude devices have
> >>>> +	 * ST microelectronics accelerometer at I2C address 0x29.
> >>>> +	 */
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	/*
> >>>> +	 * Additional individual entries were added after verification.
> >>>> +	 */
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x1dL,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>
> >>> At least for me, casting i2c address to LONG and then to pointer looks
> >>> very strange. If I look at this code without knowing what the number
> >>> 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
> >>> address.
> >>>
> >>> Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
> >>> instead of ".something = (void *)0x29L" to make it readable?
> >>
> >> struct dmi_system_id is an existing structure and we cannot just go adding
> >> fields to it. driver_data is intended to tie driver specific data to
> >> each DMI match, often pointing to some struct, so it is a void *, but
> > 
> > Yes, I know it.
> > 
> >> in this case we only need a single integer, so we store that in the
> >> pointer. That is is the address becomes obvious when looking at the code
> >> which consumes the data.
> > 
> > Ok, this makes sense. Anyway, is explicit void* cast and L suffix
> > required?
> > 
> >>> Also does the whole device table has to be such verbose with lot of
> >>> duplicated information (which probably also increase size of every linux
> >>> image which includes this driver into it)?
> >>
> >> struct dmi_system_id is the default way to specify DMI matches in
> >> the kernel. This avoids code duplication in the form of writing
> >> a DYI function to do the matching.
> >>
> >> In v2 of the patch-set I only matched on product-name, but you asked
> >> in the review of v2 to also match on sys-vendor and you mentioned
> >> we may want to support other sys-vendors too, since some other brands
> >> have SMO88xx ACPI devices too. This more or less automatically leads
> >> to using the kernel's standard, existing, DMI matching mechanism.
> >>
> >> We really want to avoid coming up with something "new" ourselves here
> >> leading to unnecessary code duplication.
> >>
> >> Regards,
> >>
> >> Hans
> > 
> > Ok, then let that table as you have it now.
> > 
> 

Re: [PATCH v3 4/6] platform/x86: dell-smo8800: Allow lis3lv02d i2c_client instantiation without IRQ

[Pali Rohár]

On Saturday 22 June 2024 16:07:53 Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 3:20 PM, Pali Rohár wrote:
> > On Friday 21 June 2024 14:24:59 Hans de Goede wrote:
> >> The Dell XPS 15 9550 can have a 60Wh battery, leaving space for a 2.5"
> >> sata disk; or a 90Wh battery in which case the battery occupies the space
> >> for the optional 2.5" sata disk.
> >>
> >> On models with the 90Wh battery and thus without a 2.5" sata disk, the BIOS
> >> does not add an IRQ resource to the SMO8810 ACPI device.
> > 
> > That is a pity, but OK, manufacturer decided that freefall sensor is
> > enabled by BIOS firmware only if the SATA is present.
> > 
> >> Make the misc-device registration and the requesting of the IRQ optional
> >> and instantiate a lis3lv02d i2c_client independent of the IRQ being there,
> >> so that the non freefall lis3lv02d functionality can still be used.
> >>
> >> Note that IRQ 0 is not a valid IRQ number for platform IRQs
> >> and this patch relies on that.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/platform/x86/dell/dell-smo8800.c | 67 +++++++++++++-----------
> >>  1 file changed, 37 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> >> index cd2e48405859..2e49bbb569c6 100644
> >> --- a/drivers/platform/x86/dell/dell-smo8800.c
> >> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> >> @@ -310,33 +310,32 @@ static int smo8800_probe(struct platform_device *device)
> >>  	init_waitqueue_head(&smo8800->misc_wait);
> >>  	INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client);
> >>  
> >> -	err = misc_register(&smo8800->miscdev);
> >> -	if (err) {
> >> -		dev_err(&device->dev, "failed to register misc dev: %d\n", err);
> >> -		return err;
> >> +	err = platform_get_irq_optional(device, 0);
> >> +	if (err > 0)
> >> +		smo8800->irq = err;
> > 
> > This code should be rather change to fail immediately. If the IRQ number
> > is not provided by the BIOS then the ACPI SMO88xx is not usable for us
> > at all. So return error from the smo8800_probe function.
> 
> The goal of this patch is to still register the bus-notifier for i2c-client
> instantiation for the lis3lv02d driver. Existing immediately here (as was
> done before) means we will still not register the bus-notifier.

If the lis3 part would be moved into separate module then there is no
need to add these checks. So it clearly shows that lis3 and smo parts
are independent and it could make sense to separate them as pointed
under other patch.

> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> >> +
> >> +	if (smo8800->irq) {
> >> +		err = misc_register(&smo8800->miscdev);
> >> +		if (err) {
> >> +			dev_err(&device->dev, "failed to register misc dev: %d\n", err);
> >> +			return err;
> >> +		}
> >> +
> >> +		err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
> >> +					   smo8800_interrupt_thread,
> >> +					   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> >> +					   DRIVER_NAME, smo8800);
> >> +		if (err) {
> >> +			dev_err(&device->dev,
> >> +				"failed to request thread for IRQ %d: %d\n",
> >> +				smo8800->irq, err);
> >> +			goto error;
> >> +		}
> >> +
> >> +		dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
> >> +			 smo8800->irq);
> >>  	}
> >>  
> >> -	platform_set_drvdata(device, smo8800);
> >> -
> >> -	err = platform_get_irq(device, 0);
> >> -	if (err < 0)
> >> -		goto error;
> >> -	smo8800->irq = err;
> >> -
> >> -	err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
> >> -				   smo8800_interrupt_thread,
> >> -				   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> >> -				   DRIVER_NAME, smo8800);
> >> -	if (err) {
> >> -		dev_err(&device->dev,
> >> -			"failed to request thread for IRQ %d: %d\n",
> >> -			smo8800->irq, err);
> >> -		goto error;
> >> -	}
> >> -
> >> -	dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
> >> -		 smo8800->irq);
> >> -
> >>  	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
> >>  		/*
> >>  		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
> >> @@ -350,14 +349,20 @@ static int smo8800_probe(struct platform_device *device)
> >>  	} else {
> >>  		dev_warn(&device->dev,
> >>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> >> +		if (!smo8800->irq)
> >> +			return -ENODEV;
> >>  	}
> >>  
> >> +	platform_set_drvdata(device, smo8800);
> >>  	return 0;
> >>  
> >>  error_free_irq:
> >> -	free_irq(smo8800->irq, smo8800);
> >> +	if (smo8800->irq) {
> >> +		free_irq(smo8800->irq, smo8800);
> >>  error:
> >> -	misc_deregister(&smo8800->miscdev);
> >> +		misc_deregister(&smo8800->miscdev);
> >> +	}
> >> +
> >>  	return err;
> >>  }
> >>  
> >> @@ -371,9 +376,11 @@ static void smo8800_remove(struct platform_device *device)
> >>  		i2c_unregister_device(smo8800->i2c_dev);
> >>  	}
> >>  
> >> -	free_irq(smo8800->irq, smo8800);
> >> -	misc_deregister(&smo8800->miscdev);
> >> -	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
> >> +	if (smo8800->irq) {
> >> +		free_irq(smo8800->irq, smo8800);
> >> +		misc_deregister(&smo8800->miscdev);
> >> +		dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
> >> +	}
> >>  }
> >>  
> >>  static const struct acpi_device_id smo8800_ids[] = {
> >> -- 
> >> 2.45.1
> >>
> > 
> 

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

[Pali Rohár]

On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> +static void smo8800_instantiate_i2c_client(struct work_struct *work)
> +{
> +	struct smo8800_device *smo8800 =
> +		container_of(work, struct smo8800_device, i2c_work);
> +	const struct dmi_system_id *lis3lv02d_dmi_id;
> +	struct i2c_board_info info = { };
> +	struct i2c_adapter *adap = NULL;
> +
> +	if (smo8800->i2c_dev)
> +		return;
> +
> +	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
> +	if (!adap)
> +		return;
> +
> +	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);

Result of this function call is always same. You can call it just once,
e.g. in module __init section and store cached result.

> +	if (!lis3lv02d_dmi_id)
> +		goto out_put_adapter;
> +
> +	info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> +
> +	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
> +	if (IS_ERR(smo8800->i2c_dev)) {
> +		dev_err(smo8800->dev, "error %ld registering %s i2c_client\n",
> +			PTR_ERR(smo8800->i2c_dev), info.type);
> +		smo8800->i2c_dev = NULL;
> +	} else {
> +		dev_dbg(smo8800->dev, "registered %s i2c_client on address 0x%02x\n",
> +			info.type, info.addr);
> +	}
> +
> +out_put_adapter:
> +	i2c_put_adapter(adap);
> +}

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

[Pali Rohár]

On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:
> > >> +	{
> > >> +		.matches = {
> > >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > >> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> > >> +		},
> > >> +		.driver_data = (void *)0x29L,
> > > 
> > > At least for me, casting i2c address to LONG and then to pointer looks
> > > very strange. If I look at this code without knowing what the number
> > > 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
> > > address.
> > > 
> > > Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
> > > instead of ".something = (void *)0x29L" to make it readable?
> > 
> > struct dmi_system_id is an existing structure and we cannot just go adding
> > fields to it. driver_data is intended to tie driver specific data to
> > each DMI match, often pointing to some struct, so it is a void *, but
> 
> Yes, I know it.
> 
> > in this case we only need a single integer, so we store that in the
> > pointer. That is is the address becomes obvious when looking at the code
> > which consumes the data.
> 
> Ok, this makes sense. Anyway, is explicit void* cast and L suffix
> required?

I have checked compilers and L suffix is not needed. No error or warning
is generated without L.

Explicit cast is needed as without it compiler generates warning.

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

[Pali Rohár]

On Saturday 22 June 2024 17:12:50 Pali Rohár wrote:
> On Saturday 22 June 2024 16:26:13 Hans de Goede wrote:
> > Hi Pali,
> > 
> > On 6/22/24 4:20 PM, Pali Rohár wrote:
> > > On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
> > >> Hi Pali,
> > >>
> > >> On 6/22/24 3:16 PM, Pali Rohár wrote:
> > >>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> > >>>> It is not necessary to handle the Dell specific instantiation of
> > >>>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
> > >>>> inside the generic i801 I2C adapter driver.
> > >>>>
> > >>>> The kernel already instantiates platform_device-s for these ACPI devices
> > >>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> > >>>> platform drivers.
> > >>>>
> > >>>> Move the i2c_client instantiation from the generic i2c-i801 driver to
> > >>>> the SMO88xx specific dell-smo8800 driver.
> > >>>
> > >>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
> > >>> and freefall code for smo88xx are basically independent.
> > >>>
> > >>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
> > >>> detection. The only thing which have these "drivers" common is the ACPI
> > >>> detection mechanism based on presence of SMO88?? identifiers from
> > >>> acpi_smo8800_ids[] array.
> > >>>
> > >>> I think it makes both "drivers" cleaner if they are put into separate
> > >>> files as they are independent of each one.
> > >>>
> > >>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
> > >>> instead (or similar name)? And just share list of ACPI ids via some
> > >>> header file (or something like that).
> > >>
> > >> Interesting idea, but that will not work, only 1 driver can bind to
> > >> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
> > > 
> > > And it is required to bind lis3 device to ACPI code? What is needed is
> > > just to check if system matches DMI strings and ACPI strings. You are
> > > not binding device to DMI strings, so I think there is no need to bind
> > > it neither to ACPI strings.
> > 
> > The driver needs to bind to something ...
> 
> Why?
> 
> Currently in i2c-i801.c file was called just
> register_dell_lis3lv02d_i2c_device() function and there was no binding
> to anything, no binding to DMI structure and neither no binding to ACPI
> structures.
> 
> And if I'm looking correctly at your new function
> smo8800_instantiate_i2c_client() it does not bind device neither.
> And smo8800_i2c_bus_notify() does not depend on binding.
> 
> So I do not see where is that binding requirement.

Now I have tried to do it, to move code into dell-lis3lv02d.c file.

Below is example how it could look like. I reused most of your code.
I have not tested it. It is just an idea.


#include <linux/acpi.h>
#include <linux/dmi.h>
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/notifier.h>
#include <linux/workqueue.h>

static struct work_struct dell_lis3lv02d_i2c_work;
static struct i2c_client *dell_lis3lv02d_i2c_dev;
static unsigned short dell_lis3lv02d_i2c_addr;

static int dell_lis3lv02d_find_i801(struct device *dev, void *data)
{
	struct i2c_adapter *adap, **adap_ret = data;

	adap = i2c_verify_adapter(dev);
	if (!adap)
		return 0;

	if (!strstarts(adap->name, "SMBus I801 adapter"))
		return 0;

	*adap_ret = i2c_get_adapter(adap->nr);
	return 1;
}

static void dell_lis3lv02d_instantiate_i2c_client(struct work_struct *work)
{
	struct i2c_board_info info = { };
	struct i2c_adapter *adap = NULL;
	struct i2c_client *client;

	if (dell_lis3lv02d_i2c_dev)
		return;

	bus_for_each_dev(&i2c_bus_type, NULL, &adap, dell_lis3lv02d_find_i801);
	if (!adap)
		return;

	info.addr = dell_lis3lv02d_i2c_addr;
	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);

	client = i2c_new_client_device(adap, &info);
	if (IS_ERR(client)) {
		pr_err("error %ld registering %s i2c_client\n",
			PTR_ERR(client), info.type);
		return;
	}

	dell_lis3lv02d_i2c_dev = client;

	pr_err("registered %s i2c_client on address 0x%02x\n",
		info.type, info.addr);
}

static int dell_lis3lv02d_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 (strstarts(adap->name, "SMBus I801 adapter"))
			queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
		break;
	case BUS_NOTIFY_REMOVED_DEVICE:
		client = i2c_verify_client(dev);
		if (!client)
			break;

		if (dell_lis3lv02d_i2c_dev == client) {
			pr_debug("accelerometer i2c_client removed\n");
			dell_lis3lv02d_i2c_dev = NULL;
		}
		break;
	default:
		break;
	}

	return 0;
}

// TODO: move this array into dell-smo8800.h header file
static const char *const acpi_smo8800_ids[] __initconst = {
	"SMO8800",
	"SMO8801",
	"SMO8810",
	"SMO8811",
	"SMO8820",
	"SMO8821",
	"SMO8830",
	"SMO8831",
};

static acpi_status __init 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 __init has_acpi_smo88xx_device(void)
{
	void *err = ERR_PTR(-ENOENT);

	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 dmi_system_id dell_lis3lv02d_devices[] __initconst = {
	/*
	 * Dell platform team told us that these Latitude devices have
	 * ST microelectronics accelerometer at I2C address 0x29.
	 */
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
		},
		.driver_data = (void *)0x29,
	},
	/*
	 * Additional individual entries were added after verification.
	 */
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
		},
		.driver_data = (void *)0x1d,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
		},
		.driver_data = (void *)0x29,
	},
	{ }
};
MODULE_DEVICE_TABLE(dmi, dell_lis3lv02d_devices);

static struct notifier_block dell_lis3lv02d_i2c_nb = {
	.notifier_call = dell_lis3lv02d_i2c_bus_notify,
};

static int __init dell_lis3lv02d_init(void)
{
	const struct dmi_system_id *lis3lv02d_dmi_id;
	int err;

	if (!has_acpi_smo88xx_device())
		return -ENODEV;

	lis3lv02d_dmi_id = dmi_first_match(dell_lis3lv02d_devices);
	if (!lis3lv02d_dmi_id)
		return -ENODEV;

	dell_lis3lv02d_i2c_addr = (uintptr_t)lis3lv02d_dmi_id->driver_data;

	err = bus_register_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
	if (err)
		return err;

	INIT_WORK(&dell_lis3lv02d_i2c_work, dell_lis3lv02d_instantiate_i2c_client);
	queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);

	return 0;
}

static void __exit dell_lis3lv02d_exit(void)
{
	bus_unregister_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
	cancel_work_sync(&dell_lis3lv02d_i2c_work);
	if (dell_lis3lv02d_i2c_dev)
		i2c_unregister_device(dell_lis3lv02d_i2c_dev);
}

module_init(dell_lis3lv02d_init);
module_exit(dell_lis3lv02d_exit);

MODULE_LICENSE("GPL");

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

[Pali Rohár]

On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:
> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
> > > Also does the whole device table has to be such verbose with lot of
> > > duplicated information (which probably also increase size of every linux
> > > image which includes this driver into it)?
> > 
> > struct dmi_system_id is the default way to specify DMI matches in
> > the kernel. This avoids code duplication in the form of writing
> > a DYI function to do the matching.
> > 
> > In v2 of the patch-set I only matched on product-name, but you asked
> > in the review of v2 to also match on sys-vendor and you mentioned
> > we may want to support other sys-vendors too, since some other brands
> > have SMO88xx ACPI devices too. This more or less automatically leads
> > to using the kernel's standard, existing, DMI matching mechanism.
> > 
> > We really want to avoid coming up with something "new" ourselves here
> > leading to unnecessary code duplication.
> > 
> > Regards,
> > 
> > Hans
> 
> Ok, then let that table as you have it now.

Definition of the table can be simplified by defining a macro which
expand to those verbose parts which are being repeating, without need to
introduce something "new". E.g.:

#define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \
	{ \
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), \
			DMI_MATCH(DMI_PRODUCT_NAME, product_name), \
		}, \
		.driver_data = (void *)(i2c_addr), \
	}

static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250", 0x29),
	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450", 0x29),
	...
	{ }
};

Any opinion about this?

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

[Andy Shevchenko]

On Sat, Jun 22, 2024 at 4:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 6/22/24 4:20 PM, Pali Rohár wrote:


> This is code for special handling required for SMO88xx ACPI devices,
> dell-smo8800 is *the* driver for those ACPI devices. So this code clearly
> belongs here.
>
> According to diffstat this adds about 400 lines of code that is really not
> that big, so I see no urgent reason to introduce weird tricks instead of
> standard driver binding for this.

This discussion may make me wonder why we don't use
MODULE_DEVICE_TABLE() for this strings, because it's a big advantage
as well of using standard kernel data types and APIs.

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

On Sunday 23 June 2024 00:36:28 Andy Shevchenko wrote:
> On Sat, Jun 22, 2024 at 4:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 6/22/24 4:20 PM, Pali Rohár wrote:
> 
> 
> > This is code for special handling required for SMO88xx ACPI devices,
> > dell-smo8800 is *the* driver for those ACPI devices. So this code clearly
> > belongs here.
> >
> > According to diffstat this adds about 400 lines of code that is really not
> > that big, so I see no urgent reason to introduce weird tricks instead of
> > standard driver binding for this.
> 
> This discussion may make me wonder why we don't use
> MODULE_DEVICE_TABLE() for this strings, because it's a big advantage
> as well of using standard kernel data types and APIs.

In example which I sent in <20240622163518.rfm2wa2kzucy7in4@pali> I used
MODULE_DEVICE_TABLE with those DMI tables.

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

[Andy Shevchenko]

On Sat, Jun 22, 2024 at 6:43 PM Pali Rohár <pali@kernel.org> wrote:
> On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:

...

> Definition of the table can be simplified by defining a macro which
> expand to those verbose parts which are being repeating, without need to
> introduce something "new". E.g.:
>
> #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \
>         { \
>                 .matches = {
>                         DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), \
>                         DMI_MATCH(DMI_PRODUCT_NAME, product_name), \
>                 }, \
>                 .driver_data = (void *)(i2c_addr), \

I'm not against this as we have a lot of different examples similar to
this (with maybe other types of ID tables). But what makes me worry is
the use of (void *) here. Shouldn't it be (const void *) so we exclude
the (potential) cases of dropping const qualifier?

>         }
>
> static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
>         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250", 0x29),
>         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450", 0x29),
>         ...
>         { }
> };
>
> Any opinion about this?



-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Sat, Jun 22, 2024 at 4:51 PM Pali Rohár <pali@kernel.org> wrote:
> On Saturday 22 June 2024 16:21:28 Hans de Goede wrote:
> > On 6/22/24 3:32 PM, Pali Rohár wrote:
> > > On Friday 21 June 2024 14:25:01 Hans de Goede wrote:

...

> > > Have you communicated
> > > with Dell about this problem?
> >
> > Dell is on the Cc of this thread, as well as the previous v2 posting:
> >
> > Cc: Dell.Client.Kernel@dell.com
>
> And what do you think, that if you send tons of long emails with C code
> to lot of recipients and add a carbon copy of all those your emails to
> some corporate group address, that somebody on that group address will
> read every one paragraph of every your email and tries to detect in if
> there is not hidden some question to which you want to know answer?
> No, with this behavior you are going to be put on the corporate
> spam list by automated systems.
>
> This looks like you just wanted to mark your personal checkbox
> "I sent email to some dell address" and let it as is.
>
> In past when I sent private email to dell I normally received responses.
> Also they said to me that group address is (or at that time was)
> monitored.
>
> So it would be nice to start communication with dell and figure out what
> is the current state of smbus address detection via ACPI/WMI/DMI/whatever,
> instead of adding this hacks via poking of smbus addresses.
>
> And in case the mentioned group address does not work anymore there are
> still other linux developers from dell who could be able to figure
> something out.

I would put it as "does Dell care about Linux?". If they do, they
should react to the messages in the Cc list. I was trying, for
example, to communicate with Relatek on some ACPI ID matters and
despite having tons of active developers it was no help. They
evidently don't care, so why would I?

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

On Sunday 23 June 2024 00:43:17 Andy Shevchenko wrote:
> On Sat, Jun 22, 2024 at 6:43 PM Pali Rohár <pali@kernel.org> wrote:
> > On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:
> 
> ...
> 
> > Definition of the table can be simplified by defining a macro which
> > expand to those verbose parts which are being repeating, without need to
> > introduce something "new". E.g.:
> >
> > #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \
> >         { \
> >                 .matches = {
> >                         DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), \
> >                         DMI_MATCH(DMI_PRODUCT_NAME, product_name), \
> >                 }, \
> >                 .driver_data = (void *)(i2c_addr), \
> 
> I'm not against this as we have a lot of different examples similar to
> this (with maybe other types of ID tables). But what makes me worry is
> the use of (void *) here. Shouldn't it be (const void *) so we exclude
> the (potential) cases of dropping const qualifier?

I do not know what is the best way here for casting short int to void*.
For me it looks strange if such casting is needed. Anyway I think that
in any case casting 16-bit short integer to const void* does not produce
different result as casting to plain (non-const) void*. It is not about
const qualifier but about integer-to-pointer cast, where is dropped
everything to that integer type.

> >         }
> >
> > static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> >         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250", 0x29),
> >         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450", 0x29),
> >         ...
> >         { }
> > };
> >
> > Any opinion about this?
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko

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

[Andy Shevchenko]

On Sun, Jun 23, 2024 at 12:50 AM Pali Rohár <pali@kernel.org> wrote:
> On Sunday 23 June 2024 00:43:17 Andy Shevchenko wrote:
> > On Sat, Jun 22, 2024 at 6:43 PM Pali Rohár <pali@kernel.org> wrote:
> > > On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:

...

> > > Definition of the table can be simplified by defining a macro which
> > > expand to those verbose parts which are being repeating, without need to
> > > introduce something "new". E.g.:
> > >
> > > #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \
> > >         { \
> > >                 .matches = {
> > >                         DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), \
> > >                         DMI_MATCH(DMI_PRODUCT_NAME, product_name), \
> > >                 }, \
> > >                 .driver_data = (void *)(i2c_addr), \
> >
> > I'm not against this as we have a lot of different examples similar to
> > this (with maybe other types of ID tables). But what makes me worry is
> > the use of (void *) here. Shouldn't it be (const void *) so we exclude
> > the (potential) cases of dropping const qualifier?
>
> I do not know what is the best way here for casting short int to void*.
> For me it looks strange if such casting is needed. Anyway I think that
> in any case casting 16-bit short integer to const void* does not produce
> different result as casting to plain (non-const) void*. It is not about
> const qualifier but about integer-to-pointer cast, where is dropped
> everything to that integer type.

You missed the long-term issue with macros like this. If we ever go
away from an integer to a real pointer, it will be easier to make such
a mistake. using proper casting will prevent you from doing that.

>
> > >         }
> > >
> > > static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> > >         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250", 0x29),
> > >         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450", 0x29),
> > >         ...
> > >         { }
> > > };
> > >
> > > Any opinion about this?

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 6/22/24 5:35 PM, Pali Rohár wrote:
> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
>> +static void smo8800_instantiate_i2c_client(struct work_struct *work)
>> +{
>> +	struct smo8800_device *smo8800 =
>> +		container_of(work, struct smo8800_device, i2c_work);
>> +	const struct dmi_system_id *lis3lv02d_dmi_id;
>> +	struct i2c_board_info info = { };
>> +	struct i2c_adapter *adap = NULL;
>> +
>> +	if (smo8800->i2c_dev)
>> +		return;
>> +
>> +	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
>> +	if (!adap)
>> +		return;
>> +
>> +	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);
> 
> Result of this function call is always same. You can call it just once,
> e.g. in module __init section and store cached result.

This function will only run when a new main i2c-i801 adapter shows up.
Which normally only happens once per boot, so there is no need to
make things more complex to optimize this.

Regards,

Hans


> 
>> +	if (!lis3lv02d_dmi_id)
>> +		goto out_put_adapter;
>> +
>> +	info.addr = (long)lis3lv02d_dmi_id->driver_data;
>> +	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>> +
>> +	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
>> +	if (IS_ERR(smo8800->i2c_dev)) {
>> +		dev_err(smo8800->dev, "error %ld registering %s i2c_client\n",
>> +			PTR_ERR(smo8800->i2c_dev), info.type);
>> +		smo8800->i2c_dev = NULL;
>> +	} else {
>> +		dev_dbg(smo8800->dev, "registered %s i2c_client on address 0x%02x\n",
>> +			info.type, info.addr);
>> +	}
>> +
>> +out_put_adapter:
>> +	i2c_put_adapter(adap);
>> +}
> 

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

[Hans de Goede]

Hi,

On 6/22/24 6:26 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:
>>>>> +	{
>>>>> +		.matches = {
>>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
>>>>> +		},
>>>>> +		.driver_data = (void *)0x29L,
>>>>
>>>> At least for me, casting i2c address to LONG and then to pointer looks
>>>> very strange. If I look at this code without knowing what the number
>>>> 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
>>>> address.
>>>>
>>>> Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
>>>> instead of ".something = (void *)0x29L" to make it readable?
>>>
>>> struct dmi_system_id is an existing structure and we cannot just go adding
>>> fields to it. driver_data is intended to tie driver specific data to
>>> each DMI match, often pointing to some struct, so it is a void *, but
>>
>> Yes, I know it.
>>
>>> in this case we only need a single integer, so we store that in the
>>> pointer. That is is the address becomes obvious when looking at the code
>>> which consumes the data.
>>
>> Ok, this makes sense. Anyway, is explicit void* cast and L suffix
>> required?
> 
> I have checked compilers and L suffix is not needed. No error or warning
> is generated without L.
> 
> Explicit cast is needed as without it compiler generates warning.

The L definitely is necessary to avoid a warning about casting an
integer to a pointer of different size. Without the L the integer
constant will alway be 32 bits which triggers the different size
warning on architectures with 64 bit pointers.

Regards,

Hans

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

[Hans de Goede]

Hi Pali,

On 6/22/24 6:35 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 17:12:50 Pali Rohár wrote:
>> On Saturday 22 June 2024 16:26:13 Hans de Goede wrote:
>>> Hi Pali,
>>>
>>> On 6/22/24 4:20 PM, Pali Rohár wrote:
>>>> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
>>>>> Hi Pali,
>>>>>
>>>>> On 6/22/24 3:16 PM, Pali Rohár wrote:
>>>>>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
>>>>>>> It is not necessary to handle the Dell specific instantiation of
>>>>>>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
>>>>>>> inside the generic i801 I2C adapter driver.
>>>>>>>
>>>>>>> The kernel already instantiates platform_device-s for these ACPI devices
>>>>>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
>>>>>>> platform drivers.
>>>>>>>
>>>>>>> Move the i2c_client instantiation from the generic i2c-i801 driver to
>>>>>>> the SMO88xx specific dell-smo8800 driver.
>>>>>>
>>>>>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
>>>>>> and freefall code for smo88xx are basically independent.
>>>>>>
>>>>>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
>>>>>> detection. The only thing which have these "drivers" common is the ACPI
>>>>>> detection mechanism based on presence of SMO88?? identifiers from
>>>>>> acpi_smo8800_ids[] array.
>>>>>>
>>>>>> I think it makes both "drivers" cleaner if they are put into separate
>>>>>> files as they are independent of each one.
>>>>>>
>>>>>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
>>>>>> instead (or similar name)? And just share list of ACPI ids via some
>>>>>> header file (or something like that).
>>>>>
>>>>> Interesting idea, but that will not work, only 1 driver can bind to
>>>>> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
>>>>
>>>> And it is required to bind lis3 device to ACPI code? What is needed is
>>>> just to check if system matches DMI strings and ACPI strings. You are
>>>> not binding device to DMI strings, so I think there is no need to bind
>>>> it neither to ACPI strings.
>>>
>>> The driver needs to bind to something ...
>>
>> Why?
>>
>> Currently in i2c-i801.c file was called just
>> register_dell_lis3lv02d_i2c_device() function and there was no binding
>> to anything, no binding to DMI structure and neither no binding to ACPI
>> structures.
>>
>> And if I'm looking correctly at your new function
>> smo8800_instantiate_i2c_client() it does not bind device neither.
>> And smo8800_i2c_bus_notify() does not depend on binding.
>>
>> So I do not see where is that binding requirement.
> 
> Now I have tried to do it, to move code into dell-lis3lv02d.c file.
> 
> Below is example how it could look like. I reused most of your code.
> I have not tested it. It is just an idea.

Thank you for going through the trouble of writing this proof
of concept.

The problem with DMI matching, instead of binding to the ACPI
SMO88xx platform_device is that this will now only load on
laptops for which we already have the DMI ids.

So this looses the warning about the i2c address info missing
which we currently give when there is a SMO88xx ACPI device
on laptops not in the DMI table (the current i2c-i801.c code
has this already). Not giving the warning in turn means we
cannot inform users about trying the new probe option, which is
the whole reason to do this patch-set in the first place.

I still believe that keeping this new code in dell-smo8800.c
is best:

1. This very much is about handling the SMO88xx ACPI devices
which makes putting it in the smo8800.c driver the logical /
the right thing to do.

2. This only adds 400 lines of code. After this all of
dell-smo8800.c is only 600 lines. That is very small so
I really so no pressing need to spread this out over 2 files.

3. You claim the freefall IRQ and the i2c_client instantiation
are 2 different things, but they are both for the same chip
and normally would both be described in the same ACPI device
node. The manual i2c_client instantation is done to address
a shortcoming of the SMO88xx ACPI device node, so handling it
belongs in the smo8800 driver.

Regards,

Hans




> #include <linux/acpi.h>
> #include <linux/dmi.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/notifier.h>
> #include <linux/workqueue.h>
> 
> static struct work_struct dell_lis3lv02d_i2c_work;
> static struct i2c_client *dell_lis3lv02d_i2c_dev;
> static unsigned short dell_lis3lv02d_i2c_addr;
> 
> static int dell_lis3lv02d_find_i801(struct device *dev, void *data)
> {
> 	struct i2c_adapter *adap, **adap_ret = data;
> 
> 	adap = i2c_verify_adapter(dev);
> 	if (!adap)
> 		return 0;
> 
> 	if (!strstarts(adap->name, "SMBus I801 adapter"))
> 		return 0;
> 
> 	*adap_ret = i2c_get_adapter(adap->nr);
> 	return 1;
> }
> 
> static void dell_lis3lv02d_instantiate_i2c_client(struct work_struct *work)
> {
> 	struct i2c_board_info info = { };
> 	struct i2c_adapter *adap = NULL;
> 	struct i2c_client *client;
> 
> 	if (dell_lis3lv02d_i2c_dev)
> 		return;
> 
> 	bus_for_each_dev(&i2c_bus_type, NULL, &adap, dell_lis3lv02d_find_i801);
> 	if (!adap)
> 		return;
> 
> 	info.addr = dell_lis3lv02d_i2c_addr;
> 	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> 
> 	client = i2c_new_client_device(adap, &info);
> 	if (IS_ERR(client)) {
> 		pr_err("error %ld registering %s i2c_client\n",
> 			PTR_ERR(client), info.type);
> 		return;
> 	}
> 
> 	dell_lis3lv02d_i2c_dev = client;
> 
> 	pr_err("registered %s i2c_client on address 0x%02x\n",
> 		info.type, info.addr);
> }
> 
> static int dell_lis3lv02d_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 (strstarts(adap->name, "SMBus I801 adapter"))
> 			queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
> 		break;
> 	case BUS_NOTIFY_REMOVED_DEVICE:
> 		client = i2c_verify_client(dev);
> 		if (!client)
> 			break;
> 
> 		if (dell_lis3lv02d_i2c_dev == client) {
> 			pr_debug("accelerometer i2c_client removed\n");
> 			dell_lis3lv02d_i2c_dev = NULL;
> 		}
> 		break;
> 	default:
> 		break;
> 	}
> 
> 	return 0;
> }
> 
> // TODO: move this array into dell-smo8800.h header file
> static const char *const acpi_smo8800_ids[] __initconst = {
> 	"SMO8800",
> 	"SMO8801",
> 	"SMO8810",
> 	"SMO8811",
> 	"SMO8820",
> 	"SMO8821",
> 	"SMO8830",
> 	"SMO8831",
> };
> 
> static acpi_status __init 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 __init has_acpi_smo88xx_device(void)
> {
> 	void *err = ERR_PTR(-ENOENT);
> 
> 	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 dmi_system_id dell_lis3lv02d_devices[] __initconst = {
> 	/*
> 	 * Dell platform team told us that these Latitude devices have
> 	 * ST microelectronics accelerometer at I2C address 0x29.
> 	 */
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	/*
> 	 * Additional individual entries were added after verification.
> 	 */
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> 		},
> 		.driver_data = (void *)0x1d,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{ }
> };
> MODULE_DEVICE_TABLE(dmi, dell_lis3lv02d_devices);
> 
> static struct notifier_block dell_lis3lv02d_i2c_nb = {
> 	.notifier_call = dell_lis3lv02d_i2c_bus_notify,
> };
> 
> static int __init dell_lis3lv02d_init(void)
> {
> 	const struct dmi_system_id *lis3lv02d_dmi_id;
> 	int err;
> 
> 	if (!has_acpi_smo88xx_device())
> 		return -ENODEV;
> 
> 	lis3lv02d_dmi_id = dmi_first_match(dell_lis3lv02d_devices);
> 	if (!lis3lv02d_dmi_id)
> 		return -ENODEV;
> 
> 	dell_lis3lv02d_i2c_addr = (uintptr_t)lis3lv02d_dmi_id->driver_data;
> 
> 	err = bus_register_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
> 	if (err)
> 		return err;
> 
> 	INIT_WORK(&dell_lis3lv02d_i2c_work, dell_lis3lv02d_instantiate_i2c_client);
> 	queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
> 
> 	return 0;
> }
> 
> static void __exit dell_lis3lv02d_exit(void)
> {
> 	bus_unregister_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
> 	cancel_work_sync(&dell_lis3lv02d_i2c_work);
> 	if (dell_lis3lv02d_i2c_dev)
> 		i2c_unregister_device(dell_lis3lv02d_i2c_dev);
> }
> 
> module_init(dell_lis3lv02d_init);
> module_exit(dell_lis3lv02d_exit);
> 
> MODULE_LICENSE("GPL");
> 

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

[Hans de Goede]

Hi,

On 6/22/24 5:07 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 16:29:21 Hans de Goede wrote:
>> Hi,
>>
>> On 6/22/24 4:23 PM, Pali Rohár wrote:
>>> On Saturday 22 June 2024 16:14:11 Hans de Goede wrote:
>>>> Hi Pali,
>>>>
>>>> On 6/22/24 4:08 PM, Pali Rohár wrote:
>>>>> On Saturday 22 June 2024 15:56:03 Hans de Goede wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 6/22/24 2:46 PM, Pali Rohár wrote:
>>>>>>> On Friday 21 June 2024 14:24:57 Hans de Goede wrote:
>>>>>>>> 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>
>>>>>>>> ---
>>>>>>>>  drivers/i2c/busses/i2c-i801.c | 9 +++++++--
>>>>>>>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>>>>>>>> index d2d2a6dbe29f..5ac5bbd60d45 100644
>>>>>>>> --- a/drivers/i2c/busses/i2c-i801.c
>>>>>>>> +++ b/drivers/i2c/busses/i2c-i801.c
>>>>>>>> @@ -1760,8 +1760,13 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>>>>>>>  
>>>>>>>>  	i801_add_tco(priv);
>>>>>>>>  
>>>>>>>> -	snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>>>> -		"SMBus I801 adapter at %04lx", priv->smba);
>>>>>>>> +	if (priv->features & FEATURE_IDF)
>>>>>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>>>> +			"SMBus I801 IDF adapter at %04lx", priv->smba);
>>>>>>>> +	else
>>>>>>>> +		snprintf(priv->adapter.name, sizeof(priv->adapter.name),
>>>>>>>> +			"SMBus I801 adapter at %04lx", priv->smba);
>>>>>>>> +
>>>>>>>
>>>>>>> User visible name is identifier for user / human.
>>>>>>>
>>>>>>> If somebody is going to read this code in next 10 years then can ask
>>>>>>> question why to have different name for IDF FEATURE and not also for
>>>>>>> other features? And can come to conclusion to unify all names to be
>>>>>>> same (why not? it is user identifier).
>>>>>>
>>>>>> That is a good point, I'll add a comment about this for the next
>>>>>> version.
>>>>>>
>>>>>>> Depending on user names between different kernel subsystem is fragile,
>>>>>>> specially for future as rename can happen.
>>>>>>
>>>>>> Relying no devices names to find devices is standard practice. E.g.
>>>>>> this is how 99% of the platform drivers bind to platform devices
>>>>>> by the driver and the device having the same name.
>>>>>
>>>>> But here it is adapter name which is more likely description, not the
>>>>> device name which is used for binding.
>>>>
>>>> It is still matching on a name.
>>>>
>>>>>>> If you are depending on FEATURE_IDF flag then check for the flag
>>>>>>> directly, and not hiding the flag by serializing it into the user
>>>>>>> visible name (char[] variable) and then de-serializing it in different
>>>>>>> kernel subsystem. If the flag is not exported yet then export it via
>>>>>>> some function or other API.
>>>>>>
>>>>>> Exporting this through some new function is non trivial and adds
>>>>>> extra dependencies between modules, causing issues when one is builtin
>>>>>> and the other is build as a module.
>>>>>
>>>>> Access to "struct i801_priv *" is not possible? For example via
>>>>> dev_get_drvdata() on "struct device *" which you have in
>>>>> smo8800_find_i801()?
>>>>>
>>>>> Because if it is possible then you can create an inline function in some
>>>>> shared header file which access this flag. Not perfect (as accessing
>>>>> private data is not the best thing) but can avoid dependences between
>>>>> modules.
>>>>
>>>> Prodding inside another drivers private driver struct is a big nono
>>>> and much much more fragile then the name checking.
>>>
>>> I know, that is why I wrote to access this structure and flags in
>>> separate function which can be an inline in e.g. i2c-i801.h header file.
>>
>> We would still need to be very very sure the device we are calling that
>> function on actually has the i2c-i801.c driver bound to it, so that
>> e.g. we are not dereferencing a NULL pointer drvdata, or worse
>> poking at some other drivers private data because we are calling
>> the helper on the wrong device.
>>
>> To make sure that is the case we would need to e.g. check that:
>> a) The device in question is an i2c adapter
>> b) the adapter name matches, so we would still be doing name matching ...
>>
>> Really just matching on the adapter name is by far the cleanest
>> option here.
>>
>> Regards,
>>
>> Hans
> 
> Ok, I have looked at it now.
> 
> For a) you are already using i2c_verify_adapter(). So this part is done.
> 
> For b) you do not need to check adapter name, but rather adapter driver
> name (in case driver data are begin accessed). And I think you can use
> dev->driver.name to get it.

This would still be checking a name and on top of that requires adding
a new helper to i2c-i801.c so we are still checking a name and the code
has gotten more complex.

Regards,

Hans

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

[Hans de Goede]

Hi Pali,

On 6/22/24 6:43 PM, Pali Rohár wrote:
> On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:
>> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
>>>> Also does the whole device table has to be such verbose with lot of
>>>> duplicated information (which probably also increase size of every linux
>>>> image which includes this driver into it)?
>>>
>>> struct dmi_system_id is the default way to specify DMI matches in
>>> the kernel. This avoids code duplication in the form of writing
>>> a DYI function to do the matching.
>>>
>>> In v2 of the patch-set I only matched on product-name, but you asked
>>> in the review of v2 to also match on sys-vendor and you mentioned
>>> we may want to support other sys-vendors too, since some other brands
>>> have SMO88xx ACPI devices too. This more or less automatically leads
>>> to using the kernel's standard, existing, DMI matching mechanism.
>>>
>>> We really want to avoid coming up with something "new" ourselves here
>>> leading to unnecessary code duplication.
>>>
>>> Regards,
>>>
>>> Hans
>>
>> Ok, then let that table as you have it now.
> 
> Definition of the table can be simplified by defining a macro which
> expand to those verbose parts which are being repeating, without need to
> introduce something "new". E.g.:
> 
> #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr) \
> 	{ \
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."), \
> 			DMI_MATCH(DMI_PRODUCT_NAME, product_name), \
> 		}, \
> 		.driver_data = (void *)(i2c_addr), \
> 	}
> 
> static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> 	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250", 0x29),
> 	DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450", 0x29),
> 	...
> 	{ }
> };
> 
> Any opinion about this?

Thank you that is a good idea. I'll do as you suggest for v4 with
the addition of Andy's suggestion to use const in the cast.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 6/23/24 3:56 PM, Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 6:35 PM, Pali Rohár wrote:
>> On Saturday 22 June 2024 17:12:50 Pali Rohár wrote:
>>> On Saturday 22 June 2024 16:26:13 Hans de Goede wrote:
>>>> Hi Pali,
>>>>
>>>> On 6/22/24 4:20 PM, Pali Rohár wrote:
>>>>> On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
>>>>>> Hi Pali,
>>>>>>
>>>>>> On 6/22/24 3:16 PM, Pali Rohár wrote:
>>>>>>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
>>>>>>>> It is not necessary to handle the Dell specific instantiation of
>>>>>>>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
>>>>>>>> inside the generic i801 I2C adapter driver.
>>>>>>>>
>>>>>>>> The kernel already instantiates platform_device-s for these ACPI devices
>>>>>>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
>>>>>>>> platform drivers.
>>>>>>>>
>>>>>>>> Move the i2c_client instantiation from the generic i2c-i801 driver to
>>>>>>>> the SMO88xx specific dell-smo8800 driver.
>>>>>>>
>>>>>>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
>>>>>>> and freefall code for smo88xx are basically independent.
>>>>>>>
>>>>>>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
>>>>>>> detection. The only thing which have these "drivers" common is the ACPI
>>>>>>> detection mechanism based on presence of SMO88?? identifiers from
>>>>>>> acpi_smo8800_ids[] array.
>>>>>>>
>>>>>>> I think it makes both "drivers" cleaner if they are put into separate
>>>>>>> files as they are independent of each one.
>>>>>>>
>>>>>>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
>>>>>>> instead (or similar name)? And just share list of ACPI ids via some
>>>>>>> header file (or something like that).
>>>>>>
>>>>>> Interesting idea, but that will not work, only 1 driver can bind to
>>>>>> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
>>>>>
>>>>> And it is required to bind lis3 device to ACPI code? What is needed is
>>>>> just to check if system matches DMI strings and ACPI strings. You are
>>>>> not binding device to DMI strings, so I think there is no need to bind
>>>>> it neither to ACPI strings.
>>>>
>>>> The driver needs to bind to something ...
>>>
>>> Why?
>>>
>>> Currently in i2c-i801.c file was called just
>>> register_dell_lis3lv02d_i2c_device() function and there was no binding
>>> to anything, no binding to DMI structure and neither no binding to ACPI
>>> structures.
>>>
>>> And if I'm looking correctly at your new function
>>> smo8800_instantiate_i2c_client() it does not bind device neither.
>>> And smo8800_i2c_bus_notify() does not depend on binding.
>>>
>>> So I do not see where is that binding requirement.
>>
>> Now I have tried to do it, to move code into dell-lis3lv02d.c file.
>>
>> Below is example how it could look like. I reused most of your code.
>> I have not tested it. It is just an idea.
> 
> Thank you for going through the trouble of writing this proof
> of concept.
> 
> The problem with DMI matching, instead of binding to the ACPI
> SMO88xx platform_device is that this will now only load on
> laptops for which we already have the DMI ids.

I guess we could put all of this from the current dell-smo8800.c
code in a .h :

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

Andy, I guess this is what you ment with your MODULE_DEVICE_TABLE()
comment?

And then include that .h from both modules. Then both modules
will auto-load and in the dell-lis3lv02d module we can use
module_init() / module_exit() to register the bus-notifier.

Pali, since you have expressed a clear preference for splitting
things and since this solution should not impact functionality
in anyway, I'll do this split for v4 of this series.

I do plan to re-use the existing CONFIG_DELL_SMO8800 Kconfig
value to decide whether or not to build the new dell-lis3lv02d
module. Having 2 separate Kconfig options for this seems unnecessary.

Regards,

Hans




> 
> So this looses the warning about the i2c address info missing
> which we currently give when there is a SMO88xx ACPI device
> on laptops not in the DMI table (the current i2c-i801.c code
> has this already). Not giving the warning in turn means we
> cannot inform users about trying the new probe option, which is
> the whole reason to do this patch-set in the first place.
> 
> I still believe that keeping this new code in dell-smo8800.c
> is best:
> 
> 1. This very much is about handling the SMO88xx ACPI devices
> which makes putting it in the smo8800.c driver the logical /
> the right thing to do.
> 
> 2. This only adds 400 lines of code. After this all of
> dell-smo8800.c is only 600 lines. That is very small so
> I really so no pressing need to spread this out over 2 files.
> 
> 3. You claim the freefall IRQ and the i2c_client instantiation
> are 2 different things, but they are both for the same chip
> and normally would both be described in the same ACPI device
> node. The manual i2c_client instantation is done to address
> a shortcoming of the SMO88xx ACPI device node, so handling it
> belongs in the smo8800 driver.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
>> #include <linux/acpi.h>
>> #include <linux/dmi.h>
>> #include <linux/i2c.h>
>> #include <linux/module.h>
>> #include <linux/notifier.h>
>> #include <linux/workqueue.h>
>>
>> static struct work_struct dell_lis3lv02d_i2c_work;
>> static struct i2c_client *dell_lis3lv02d_i2c_dev;
>> static unsigned short dell_lis3lv02d_i2c_addr;
>>
>> static int dell_lis3lv02d_find_i801(struct device *dev, void *data)
>> {
>> 	struct i2c_adapter *adap, **adap_ret = data;
>>
>> 	adap = i2c_verify_adapter(dev);
>> 	if (!adap)
>> 		return 0;
>>
>> 	if (!strstarts(adap->name, "SMBus I801 adapter"))
>> 		return 0;
>>
>> 	*adap_ret = i2c_get_adapter(adap->nr);
>> 	return 1;
>> }
>>
>> static void dell_lis3lv02d_instantiate_i2c_client(struct work_struct *work)
>> {
>> 	struct i2c_board_info info = { };
>> 	struct i2c_adapter *adap = NULL;
>> 	struct i2c_client *client;
>>
>> 	if (dell_lis3lv02d_i2c_dev)
>> 		return;
>>
>> 	bus_for_each_dev(&i2c_bus_type, NULL, &adap, dell_lis3lv02d_find_i801);
>> 	if (!adap)
>> 		return;
>>
>> 	info.addr = dell_lis3lv02d_i2c_addr;
>> 	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>>
>> 	client = i2c_new_client_device(adap, &info);
>> 	if (IS_ERR(client)) {
>> 		pr_err("error %ld registering %s i2c_client\n",
>> 			PTR_ERR(client), info.type);
>> 		return;
>> 	}
>>
>> 	dell_lis3lv02d_i2c_dev = client;
>>
>> 	pr_err("registered %s i2c_client on address 0x%02x\n",
>> 		info.type, info.addr);
>> }
>>
>> static int dell_lis3lv02d_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 (strstarts(adap->name, "SMBus I801 adapter"))
>> 			queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
>> 		break;
>> 	case BUS_NOTIFY_REMOVED_DEVICE:
>> 		client = i2c_verify_client(dev);
>> 		if (!client)
>> 			break;
>>
>> 		if (dell_lis3lv02d_i2c_dev == client) {
>> 			pr_debug("accelerometer i2c_client removed\n");
>> 			dell_lis3lv02d_i2c_dev = NULL;
>> 		}
>> 		break;
>> 	default:
>> 		break;
>> 	}
>>
>> 	return 0;
>> }
>>
>> // TODO: move this array into dell-smo8800.h header file
>> static const char *const acpi_smo8800_ids[] __initconst = {
>> 	"SMO8800",
>> 	"SMO8801",
>> 	"SMO8810",
>> 	"SMO8811",
>> 	"SMO8820",
>> 	"SMO8821",
>> 	"SMO8830",
>> 	"SMO8831",
>> };
>>
>> static acpi_status __init 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 __init has_acpi_smo88xx_device(void)
>> {
>> 	void *err = ERR_PTR(-ENOENT);
>>
>> 	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 dmi_system_id dell_lis3lv02d_devices[] __initconst = {
>> 	/*
>> 	 * Dell platform team told us that these Latitude devices have
>> 	 * ST microelectronics accelerometer at I2C address 0x29.
>> 	 */
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	/*
>> 	 * Additional individual entries were added after verification.
>> 	 */
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
>> 		},
>> 		.driver_data = (void *)0x1d,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{
>> 		.matches = {
>> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>> 			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
>> 		},
>> 		.driver_data = (void *)0x29,
>> 	},
>> 	{ }
>> };
>> MODULE_DEVICE_TABLE(dmi, dell_lis3lv02d_devices);
>>
>> static struct notifier_block dell_lis3lv02d_i2c_nb = {
>> 	.notifier_call = dell_lis3lv02d_i2c_bus_notify,
>> };
>>
>> static int __init dell_lis3lv02d_init(void)
>> {
>> 	const struct dmi_system_id *lis3lv02d_dmi_id;
>> 	int err;
>>
>> 	if (!has_acpi_smo88xx_device())
>> 		return -ENODEV;
>>
>> 	lis3lv02d_dmi_id = dmi_first_match(dell_lis3lv02d_devices);
>> 	if (!lis3lv02d_dmi_id)
>> 		return -ENODEV;
>>
>> 	dell_lis3lv02d_i2c_addr = (uintptr_t)lis3lv02d_dmi_id->driver_data;
>>
>> 	err = bus_register_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
>> 	if (err)
>> 		return err;
>>
>> 	INIT_WORK(&dell_lis3lv02d_i2c_work, dell_lis3lv02d_instantiate_i2c_client);
>> 	queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
>>
>> 	return 0;
>> }
>>
>> static void __exit dell_lis3lv02d_exit(void)
>> {
>> 	bus_unregister_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
>> 	cancel_work_sync(&dell_lis3lv02d_i2c_work);
>> 	if (dell_lis3lv02d_i2c_dev)
>> 		i2c_unregister_device(dell_lis3lv02d_i2c_dev);
>> }
>>
>> module_init(dell_lis3lv02d_init);
>> module_exit(dell_lis3lv02d_exit);
>>
>> MODULE_LICENSE("GPL");
>>
> 

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

[Pali Rohár]

On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},

This is an example why DMI_MATCH is not a good idea for usage and
DMI_EXACT_MATCH should be there instead. First entry is substring of
second entry, so kernel running on second model will use first entry.

Both have same driver_data, so it is not an issue now. But if the list
will be extended in future then DMI_MATCH conflicts can appear invisibly.

Anyway, in i2c-i801.c is exact match on the both vendor and product name.