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

[PATCH v2 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 a patch series which implements my suggestions from:
https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
to improve the lis3lv02d accel support on Dell laptops.

Jean, Andi the actual move is in patch 2/6 after 1 small prep patch on
the dell-smo8800 side. My plan for merging this is to create an immutable
branch based on 6.8-rc1 (once it is out) + these 6 patches and then send
a pull-request for this. Can I have your Ack for the i2c-i801 changes in
patch 2/6?

I think you'll like the changes there since they only remove code
on the i2c-i801.c side :)

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[]

Regards,

Hans


Hans de Goede (6):
  platform/x86: dell-smo8800: Change probe() ordering a bit
  platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client
    from i2c-i801 to dell-smo8800
  platform/x86: dell-smo8800: Pass the IRQ to the lis3lv02d i2c_client
  platform/x86: dell-smo8800: Allow using the IIO st_accel driver
  platform/x86: dell-smo8800: Add support for probing for the
    accelerometer i2c address
  platform/x86: dell-smo8800: Add a couple more models to
    dell_lis3lv02d_devices[]

 drivers/i2c/busses/i2c-i801.c            | 122 --------
 drivers/platform/x86/dell/dell-smo8800.c | 341 +++++++++++++++++++++--
 2 files changed, 319 insertions(+), 144 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] platform/x86: dell-smo8800: Change probe() ordering a bit

[Hans de Goede]

Retrieve the IRQ number from the platform_device a bit earlier
and only call platform_set_drvdata() on successful probe
(the drvdata is only used from the remove() callback).

This is a preparation patch for moving the lis3lv02d i2c_client
instantiation from the i2c-i801 driver to dell-smo8800.

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

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index f7ec17c56833..87339cc78880 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -121,19 +121,17 @@ static int smo8800_probe(struct platform_device *device)
 
 	init_waitqueue_head(&smo8800->misc_wait);
 
+	err = platform_get_irq(device, 0);
+	if (err < 0)
+		return err;
+	smo8800->irq = err;
+
 	err = misc_register(&smo8800->miscdev);
 	if (err) {
 		dev_err(&device->dev, "failed to register misc dev: %d\n", err);
 		return err;
 	}
 
-	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,
@@ -147,6 +145,7 @@ static int smo8800_probe(struct platform_device *device)
 
 	dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
 		 smo8800->irq);
+	platform_set_drvdata(device, smo8800);
 	return 0;
 
 error:
-- 
2.43.0

[PATCH v2 2/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 SMO8xxx 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 Dell specific dell-smo8800 driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
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            | 126 +----------------------
 drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++-
 2 files changed, 123 insertions(+), 124 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 070999139c6d..595e263ba623 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -975,6 +975,10 @@ static const struct i2c_algorithm smbus_algorithm = {
 #define FEATURES_ICH4	(FEATURE_SMBUS_PEC | FEATURE_BLOCK_BUFFER | \
 			 FEATURE_HOST_NOTIFY)
 
+/*
+ * NOTE: If new models with FEATURE_IDF are added please also update
+ * i801_idf_ids[] in drivers/platform/x86/dell-smo8800.c
+ */
 static const struct pci_device_id i801_ids[] = {
 	{ PCI_DEVICE_DATA(INTEL, 82801AA_3,			0)				 },
 	{ PCI_DEVICE_DATA(INTEL, 82801AB_3,			0)				 },
@@ -1141,125 +1145,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 },
-	{ "Vostro V131",        0x1d },
-	{ "Vostro 5568",        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)
 {
@@ -1279,9 +1164,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 */
 #if IS_ENABLED(CONFIG_I2C_MUX_GPIO)
 	if (!priv->mux_pdev)
diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 87339cc78880..c674e3392270 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -4,18 +4,23 @@
  *
  *  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/pci.h>
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
 
@@ -26,6 +31,7 @@ struct smo8800_device {
 	unsigned long misc_opened;   /* whether the device is open */
 	wait_queue_head_t misc_wait; /* Wait queue for the misc dev */
 	struct device *dev;          /* acpi device */
+	struct i2c_client *i2c_dev;  /* i2c_client for lis3lv02d */
 };
 
 static irqreturn_t smo8800_interrupt_quick(int irq, void *data)
@@ -103,6 +109,111 @@ static const struct file_operations smo8800_misc_fops = {
 	.release = smo8800_misc_release,
 };
 
+/*
+ * On 2 older PCH generations, Patsburg (for Sandy Bridge and Ivybridge) and
+ * Wellsburg (for Haswell and Broadwell), the PCH has 3 extra i2c-i801
+ * compatible SMBusses called 'Integrated Device Function' busses. These have
+ * FEATURE_IDF set in the i801_ids[] table in i2c-i801.c.
+ * The ST microelectronics accelerometer is connected to the main SMBus
+ * so the IDF controllers should be skipped.
+ */
+static const struct pci_device_id i801_idf_ids[] = {
+	{ PCI_VDEVICE(INTEL, 0x1d70) }, /* Patsburg IFD0 */
+	{ PCI_VDEVICE(INTEL, 0x1d71) }, /* Patsburg IFD1 */
+	{ PCI_VDEVICE(INTEL, 0x1d72) }, /* Patsburg IFD2 */
+	{ PCI_VDEVICE(INTEL, 0x8d7d) }, /* Wellsburg MS0 */
+	{ PCI_VDEVICE(INTEL, 0x8d7e) }, /* Wellsburg MS1 */
+	{ PCI_VDEVICE(INTEL, 0x8d7f) }, /* Wellsburg MS2 */
+	{}
+};
+
+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;
+
+	if (pci_match_id(i801_idf_ids, to_pci_dev(adap->dev.parent)))
+		return 0; /* Only register client on main SMBus channel */
+
+	*adap_ret = i2c_get_adapter(adap->nr);
+	return 1;
+}
+
+/*
+ * 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 },
+	{ "Vostro V131",        0x1d },
+	{ "Vostro 5568",        0x29 },
+};
+
+static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
+{
+	struct i2c_board_info info = { };
+	struct i2c_adapter *adap = NULL;
+	const char *dmi_product_name;
+	u8 addr = 0;
+	int i;
+
+	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
+	if (!adap)
+		return;
+
+	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) {
+			addr = dell_lis3lv02d_devices[i].i2c_addr;
+			break;
+		}
+	}
+
+	if (!addr) {
+		dev_warn(smo8800->dev,
+			 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
+		goto put_adapter;
+	}
+
+	info.addr = addr;
+	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+
+	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
+	if (IS_ERR(smo8800->i2c_dev)) {
+		dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev),
+			      "registering accel i2c_client\n");
+		smo8800->i2c_dev = NULL;
+	} else {
+		dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n",
+			 info.type, info.addr);
+	}
+put_adapter:
+	i2c_put_adapter(adap);
+}
+
 static int smo8800_probe(struct platform_device *device)
 {
 	int err;
@@ -126,10 +237,12 @@ static int smo8800_probe(struct platform_device *device)
 		return err;
 	smo8800->irq = err;
 
+	smo8800_instantiate_i2c_client(smo8800);
+
 	err = misc_register(&smo8800->miscdev);
 	if (err) {
 		dev_err(&device->dev, "failed to register misc dev: %d\n", err);
-		return err;
+		goto error_unregister_i2c_client;
 	}
 
 	err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
@@ -150,6 +263,8 @@ static int smo8800_probe(struct platform_device *device)
 
 error:
 	misc_deregister(&smo8800->miscdev);
+error_unregister_i2c_client:
+	i2c_unregister_device(smo8800->i2c_dev);
 	return err;
 }
 
@@ -160,9 +275,9 @@ static void smo8800_remove(struct platform_device *device)
 	free_irq(smo8800->irq, smo8800);
 	misc_deregister(&smo8800->miscdev);
 	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
+	i2c_unregister_device(smo8800->i2c_dev);
 }
 
-/* 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 },
@@ -189,3 +304,5 @@ module_platform_driver(smo8800_driver);
 MODULE_DESCRIPTION("Dell Latitude freefall driver (ACPI SMO88XX)");
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Sonal Santan, Pali Rohár");
+/* Ensure the i2c-801 driver is loaded for i2c_client instantiation */
+MODULE_SOFTDEP("pre: i2c-i801");
-- 
2.43.0

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

[Andy Shevchenko]

+ Cc people from tags of 72921427d46b ("string.h: Add str_has_prefix() helper
function"). See below why.

On Sat, Jan 06, 2024 at 05:09:29PM +0100, Hans de Goede wrote:
> It is not necessary to handle the Dell specific instantiation of
> i2c_client-s for SMO8xxx 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 Dell specific dell-smo8800 driver.

...

> +/*
> + * NOTE: If new models with FEATURE_IDF are added please also update
> + * i801_idf_ids[] in drivers/platform/x86/dell-smo8800.c
> + */
>  static const struct pci_device_id i801_ids[] = {

I believe this comment can be enforced with some defines / static_assert().
But I haven't given any thought about it, just an idea.

>  };

...

> + *  Copyright (C) 2023 Hans de Goede <hansg@kernel.org>

2024?

...

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

Bah, we have str_has_prefix() and this, much older one...
Steven? Others? I mean we can do something about this duplication, right?

-- 
With Best Regards,
Andy Shevchenko

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

[Joe Perches]

On Sat, 2024-01-06 at 18:24 +0200, Andy Shevchenko wrote:
> + Cc people from tags of 72921427d46b ("string.h: Add str_has_prefix() helper
> function"). See below why.
> > +	if (!strstarts(adap->name, "SMBus I801 adapter"))
> > +		return 0;
> 
> Bah, we have str_has_prefix() and this, much older one...
> Steven? Others? I mean we can do something about this duplication, right?

coccinelle?

@@
expression a, b;
@@

-	strstarts(a, b)
+	str_has_prefix(a, b)

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

[Steven Rostedt]

On Sat, 6 Jan 2024 18:24:34 +0200
Andy Shevchenko <andy@kernel.org> wrote:

> > +	if (!strstarts(adap->name, "SMBus I801 adapter"))
> > +		return 0;  
> 
> Bah, we have str_has_prefix() and this, much older one...
> Steven? Others? I mean we can do something about this duplication, right?

They are not really duplicate functions.

Note that strstarts() is just a boolean (does this start with something)
where as str_has_prefix() returns the length of the prefix.

Yes, strstarts() can be swapped to str_has_prefix() but it can't go the
other way around. One use case of the str_has_prefix() feature is in the
histogram parsing:

	for (i = 0; i < hist_data->attrs->n_actions; i++) {
		str = hist_data->attrs->action_str[i];

		if ((len = str_has_prefix(str, "onmatch("))) {
			char *action_str = str + len;

			data = onmatch_parse(tr, action_str);
			if (IS_ERR(data)) {
				ret = PTR_ERR(data);
				break;
			}
		} else if ((len = str_has_prefix(str, "onmax("))) {
			char *action_str = str + len;

			data = track_data_parse(hist_data, action_str,
						HANDLER_ONMAX);
			if (IS_ERR(data)) {
				ret = PTR_ERR(data);
				break;
			}
		} else if ((len = str_has_prefix(str, "onchange("))) {
			char *action_str = str + len;

			data = track_data_parse(hist_data, action_str,
						HANDLER_ONCHANGE);
			if (IS_ERR(data)) {
				ret = PTR_ERR(data);
				break;
			}

Where we get the length of the prefix if there's a match, and use that to
skip over the prefix.

If you just need to know if something starts with a string, then
"strstarts()" is perfectly fine to use.

-- Steve

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

[Steven Rostedt]

On Sun, 7 Jan 2024 11:09:17 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> 	for (i = 0; i < hist_data->attrs->n_actions; i++) {
> 		str = hist_data->attrs->action_str[i];
> 
> 		if ((len = str_has_prefix(str, "onmatch("))) {
> 			char *action_str = str + len;
> 
> 			data = onmatch_parse(tr, action_str);
> 			if (IS_ERR(data)) {
> 				ret = PTR_ERR(data);
> 				break;
> 			}
> 		} else if ((len = str_has_prefix(str, "onmax("))) {
> 			char *action_str = str + len;
> 
> 			data = track_data_parse(hist_data, action_str,
> 						HANDLER_ONMAX);
> 			if (IS_ERR(data)) {
> 				ret = PTR_ERR(data);
> 				break;
> 			}
> 		} else if ((len = str_has_prefix(str, "onchange("))) {
> 			char *action_str = str + len;
> 
> 			data = track_data_parse(hist_data, action_str,
> 						HANDLER_ONCHANGE);
> 			if (IS_ERR(data)) {
> 				ret = PTR_ERR(data);
> 				break;
> 			}

And this could possibly be consolidated to:

	for (i = 0; i < hist_data->attrs->n_actions; i++) {
		char *action_str;
		enum handler_id hid;

		ret = -EINVAL;

		str = hist_data->attrs->action_str[i];

		if ((len = str_has_prefix(str, "onmatch(")))
			hid = HANDLER_ONMATCH;
		else if ((len = str_has_prefix(str, "onmax(")))
			hid = HANDLER_ONMAX;
		else if ((len = str_has_prefix(str, "onchange(")))
			hid = HANDLER_ONCHANGE;
		else
			break;

		action_str = str + len;

		switch (hid) {
		case HANDLER_ONMATCH:
			data = onmatch_parse(tr, action_str);
			break;
		default:
			data = track_data_parse(hist_data, action_str, hid);
		}
			
		if (IS_ERR(data)) {
			ret = PTR_ERR(data);
			break;
		}

		hist_data->actions[hist_data->n_actions++] = data;
		ret = 0;
	}

I think I'll go make that change!

-- Steve

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

[Pali Rohár]

On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
> +/*
> + * 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 },
> +	{ "Vostro V131",        0x1d },
> +	{ "Vostro 5568",        0x29 },
> +};
> +
> +static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
> +{
> +	struct i2c_board_info info = { };
> +	struct i2c_adapter *adap = NULL;
> +	const char *dmi_product_name;
> +	u8 addr = 0;
> +	int i;
> +
> +	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
> +	if (!adap)
> +		return;
> +
> +	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> +	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {

Before doing this array iteration it is needed to check for Dell vendor
like it was before:

       if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
               return false;

Or put vendor name into the devices list and check for it (in case you
want to extend list also for non-Dell machines).

> +		if (strcmp(dmi_product_name,
> +			   dell_lis3lv02d_devices[i].dmi_product_name) == 0) {
> +			addr = dell_lis3lv02d_devices[i].i2c_addr;
> +			break;
> +		}
> +	}
> +
> +	if (!addr) {
> +		dev_warn(smo8800->dev,
> +			 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
> +		goto put_adapter;
> +	}
> +
> +	info.addr = addr;
> +	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> +
> +	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
> +	if (IS_ERR(smo8800->i2c_dev)) {
> +		dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev),
> +			      "registering accel i2c_client\n");
> +		smo8800->i2c_dev = NULL;
> +	} else {
> +		dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n",
> +			 info.type, info.addr);
> +	}
> +put_adapter:
> +	i2c_put_adapter(adap);
> +}
> +
>  static int smo8800_probe(struct platform_device *device)
>  {
>  	int err;
> @@ -126,10 +237,12 @@ static int smo8800_probe(struct platform_device *device)
>  		return err;
>  	smo8800->irq = err;
>  
> +	smo8800_instantiate_i2c_client(smo8800);

Now after looking at this change again I see there a problem. If i2c-801
driver initialize i2c-801 device after this smo8800 is called then
accelerometer i2c device would not happen.

Also it has same problem if PCI i801 device is reloaded or reset.

With the current approach this was not an issue as during i801
initialization was smo i2c device automatically created and lis driver
was able to bind and initialize it at any time.

Before parent driver created its own direct children devices. After this
change, the child driver is trying to find who is the parent of its
device and injects its device to the parent in the device tree
hierarchy.

> +
>  	err = misc_register(&smo8800->miscdev);
>  	if (err) {
>  		dev_err(&device->dev, "failed to register misc dev: %d\n", err);
> -		return err;
> +		goto error_unregister_i2c_client;
>  	}
>  
>  	err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
> @@ -150,6 +263,8 @@ static int smo8800_probe(struct platform_device *device)
>  
>  error:
>  	misc_deregister(&smo8800->miscdev);
> +error_unregister_i2c_client:
> +	i2c_unregister_device(smo8800->i2c_dev);
>  	return err;
>  }
>  
> @@ -160,9 +275,9 @@ static void smo8800_remove(struct platform_device *device)
>  	free_irq(smo8800->irq, smo8800);
>  	misc_deregister(&smo8800->miscdev);
>  	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
> +	i2c_unregister_device(smo8800->i2c_dev);
>  }
>  
> -/* 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 },
> @@ -189,3 +304,5 @@ module_platform_driver(smo8800_driver);
>  MODULE_DESCRIPTION("Dell Latitude freefall driver (ACPI SMO88XX)");
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("Sonal Santan, Pali Rohár");
> +/* Ensure the i2c-801 driver is loaded for i2c_client instantiation */
> +MODULE_SOFTDEP("pre: i2c-i801");
> -- 
> 2.43.0
> 

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

[Pali Rohár]

On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
> It is not necessary to handle the Dell specific instantiation of
> i2c_client-s for SMO8xxx 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 Dell specific dell-smo8800 driver.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> 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            | 126 +----------------------
>  drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++-
>  2 files changed, 123 insertions(+), 124 deletions(-)

I'm looking at this change again and I'm not not sure if it is a good
direction to do this movement. Some of the issues which this change
introduces I described in the previous email. I somehow have not caught
up why to do it.

ACPI smo8800 device and i2c lis3lv02d are from the OS resource point of
view totally different devices, not connected together in any way. In
ACPI tables there is no link information that smo8800 belongs to
lis3lv02d, and neither that it is on i2c. System tree of the devices
structures also handle it in this way.

If I'm looking at the current device design, it is a bus who instantiate
devices (as children of the bus). In this case, this i2c has no
information what is there connected (no Device Tree, no ACPI), so only
static data hardcoded in kernel are available. And therefore it should
be the bus who create or delete devices.

If the whole idea of this patch series was to merge smo8800 device and
lis3lv02d device into one device, the question is why to do it and why
it is a good idea in this case? (Specially when firmware provide to use
very little information). I just do not see this motivation. If it is
going to fix some bug or required for some new feature or something
else. I would like to know this one. Maybe I'm completely something
missing and hence I'm wrong...

I know that it is just a one device which provides interrupt and
accelerometer axes, but these two things are from OS persepctive totally
separated and there can be all combinations which of them are available
and which not (BIOS has select option to disable ACPI device=interrupt,
can be ON/OFF and kernel has or does not have DMI information of i2c bus
for acelerometer axes).

I can understand motivation to replace one i2c driver by another, if
there is a new style of it. In this it is just needed to teach new iio
lis driver to support some joystick emulation layer (can be generic, or
only for lis, or only available for HP and Dell machines) and switch can
be done without issue.

I can also understand motivation that freefall code is in two drivers
(old i2c lis driver and acpi smo8800). In this case it can be extracted
somwhere into helper function, or maybe completely moves into
platform/x86 as it is IIRC only for HP and Dell machines, and can ripped
out from the old i2c lis driver (unless there is some other usage for
it).

I also know that it is not a clean to have some Dell DMI data list in
the i801 bus driver and helper code not related to i801. So why not to
move this code from i2c-i801.c source file to some other helper library
and call just the helper function from i801.


I would rather let i2c lis device and ACPI smo8800 device separated,
this concept is less prone to errors, matches linux device model and is
already in use for many years and verified that works fine.

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

[Jean Delvare]

Hi Pali, Hans,

On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
> > It is not necessary to handle the Dell specific instantiation of
> > i2c_client-s for SMO8xxx 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 Dell specific dell-smo8800 driver.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > 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            | 126 +----------------------
> >  drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++-
> >  2 files changed, 123 insertions(+), 124 deletions(-)  
> 
> I'm looking at this change again and I'm not not sure if it is a good
> direction to do this movement. (...)

Same feeling here. Having to lookup the parent i2c bus, which may or
may not be present yet, doesn't feel good.

I wouldn't object if everybody was happy with the move and moving the
code was solving an actual issue, but that doesn't seem to be the case.

-- 
Jean Delvare
SUSE L3 Support

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

[Hans de Goede]

Hi Jean,

On 2/13/24 17:30, Jean Delvare wrote:
> Hi Pali, Hans,
> 
> On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
>> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
>>> It is not necessary to handle the Dell specific instantiation of
>>> i2c_client-s for SMO8xxx 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 Dell specific dell-smo8800 driver.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> 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            | 126 +----------------------
>>>  drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++-
>>>  2 files changed, 123 insertions(+), 124 deletions(-)  
>>
>> I'm looking at this change again and I'm not not sure if it is a good
>> direction to do this movement. (...)
> 
> Same feeling here. Having to lookup the parent i2c bus, which may or
> may not be present yet, doesn't feel good.
> 
> I wouldn't object if everybody was happy with the move and moving the
> code was solving an actual issue, but that doesn't seem to be the case.

I thought you would actually like getting this somewhat clunky code
which basically works around the hw not being properly described in
the ACPI tables out of the generic i2c-i801 code.

I didn't get around to answer's Pali's concerns yet, so let me
start by addressing those since you indicate that you share Pali's
concerns:

Pali wrote:
> Now after looking at this change again I see there a problem. If i2c-801
> driver initialize i2c-801 device after this smo8800 is called then
> accelerometer i2c device would not happen.

That is a good point (which Jean also points out). But this can simply
be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER
if the i2c-i801 i2c-bus is not present yet (all designs using the
dell-smo8800 driver will have an i2c-bus so waiting for this to show
up should not cause regressions).

If we can agree to move forward this series I'll fix this.

Pali wrote:
> Also it has same problem if PCI i801 device is reloaded or reset.

The i801 device is not hotplugable, so normally this will never
happen. If the user manually unbinds + rebinds the i2c-i801 driver
them the i2c_client for the smo88xx device will indeed get removed
and not re-added. But this will normally never happen and if
a user is manually poking things then the user can also unbind +
rebind the dell-mso8800 driver after the i2c-i801 rebind.
So I don't really see this as an issue.

With those remarks addressed let me try to explain why I think
that moving this to the dell-smo8800 code is a good idea:

1. It is a SMO88xx ACPI device specific kludge and as such IMHO
thus belongs in the driver for the SMO88xx ACPI platform_device.

The i2c-i801 driver gets loaded on every x86 system and it is
undesirable to have this extra code and the DMI table in RAM
on all those other systems.

2. Further changes in this series, like adding support for
probing for the i2c address of the lis3lv02d device on models
not yet in the DMI table, will add a bunch of more code specific
to SMO88xx ACPI devices. Making the problem of having SMO88xx
specific code in the generic i2c-i801 driver even bigger.
The current amount of SMO88xx specific code in the
generic i2c-i801 driver might be considered acceptable but I'm
afraid that the amount of code after this series will not be
acceptable.

3. Some of the changes in this series are harder to implement inside
the i2c-i801 code, like optionally instantiating an i2c_client for
the IIO st_accel driver (*) so that the accelerometer gets presented
to userspace as a standard IIO device like all modern accelerometer
drivers do.

This requires setting i2c_client.irq and that IRQ comes from
the SMO88xx ACPI device. So this would require the i2c-i801 code
to lookup the ACPI device and get the IRQ from there. Where as
in the SMO88xx ACPI platform_device driver the IRQ is readily
available.

TL;DR: IMHO all this SMO88xx quirk/glue handling belongs in
the SMO88xx specific dell-smo8800 driver rather then in
the generic i2c-i801 code.

Regards,

Hans


*) Instead of an i2c_client for the somewhat weird (but still
default for backward compat) drivers/misc/lis3lv02d/lis3lv02d.c
driver

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

[Andy Shevchenko]

On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote:
> On 2/13/24 17:30, Jean Delvare wrote:
> > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
> >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:

FWIW, I agree with Hans on the location of the HW quirks.
If there is a possible way to make the actual driver cleaner
and collect quirks somewhere else, I'm full support for that.

> >> I'm looking at this change again and I'm not not sure if it is a good
> >> direction to do this movement. (...)
> > 
> > Same feeling here. Having to lookup the parent i2c bus, which may or
> > may not be present yet, doesn't feel good.
> > 
> > I wouldn't object if everybody was happy with the move and moving the
> > code was solving an actual issue, but that doesn't seem to be the case.
> 
> I thought you would actually like getting this somewhat clunky code
> which basically works around the hw not being properly described in
> the ACPI tables out of the generic i2c-i801 code.
> 
> I didn't get around to answer's Pali's concerns yet, so let me
> start by addressing those since you indicate that you share Pali's
> concerns:
> 
> Pali wrote:
> > Now after looking at this change again I see there a problem. If i2c-801
> > driver initialize i2c-801 device after this smo8800 is called then
> > accelerometer i2c device would not happen.
> 
> That is a good point (which Jean also points out). But this can simply
> be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER
> if the i2c-i801 i2c-bus is not present yet (all designs using the
> dell-smo8800 driver will have an i2c-bus so waiting for this to show
> up should not cause regressions).
> 
> If we can agree to move forward this series I'll fix this.
> 
> Pali wrote:
> > Also it has same problem if PCI i801 device is reloaded or reset.
> 
> The i801 device is not hotplugable, so normally this will never
> happen. If the user manually unbinds + rebinds the i2c-i801 driver
> them the i2c_client for the smo88xx device will indeed get removed
> and not re-added. But this will normally never happen and if
> a user is manually poking things then the user can also unbind +
> rebind the dell-mso8800 driver after the i2c-i801 rebind.
> So I don't really see this as an issue.
> 
> With those remarks addressed let me try to explain why I think
> that moving this to the dell-smo8800 code is a good idea:
> 
> 1. It is a SMO88xx ACPI device specific kludge and as such IMHO
> thus belongs in the driver for the SMO88xx ACPI platform_device.
> 
> The i2c-i801 driver gets loaded on every x86 system and it is
> undesirable to have this extra code and the DMI table in RAM
> on all those other systems.
> 
> 2. Further changes in this series, like adding support for
> probing for the i2c address of the lis3lv02d device on models
> not yet in the DMI table, will add a bunch of more code specific
> to SMO88xx ACPI devices. Making the problem of having SMO88xx
> specific code in the generic i2c-i801 driver even bigger.
> The current amount of SMO88xx specific code in the
> generic i2c-i801 driver might be considered acceptable but I'm
> afraid that the amount of code after this series will not be
> acceptable.
> 
> 3. Some of the changes in this series are harder to implement inside
> the i2c-i801 code, like optionally instantiating an i2c_client for
> the IIO st_accel driver (*) so that the accelerometer gets presented
> to userspace as a standard IIO device like all modern accelerometer
> drivers do.
> 
> This requires setting i2c_client.irq and that IRQ comes from
> the SMO88xx ACPI device. So this would require the i2c-i801 code
> to lookup the ACPI device and get the IRQ from there. Where as
> in the SMO88xx ACPI platform_device driver the IRQ is readily
> available.
> 
> TL;DR: IMHO all this SMO88xx quirk/glue handling belongs in
> the SMO88xx specific dell-smo8800 driver rather then in
> the generic i2c-i801 code.
> 
> Regards,
> 
> Hans
> 
> 
> *) Instead of an i2c_client for the somewhat weird (but still
> default for backward compat) drivers/misc/lis3lv02d/lis3lv02d.c
> driver

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote:
> On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote:
> > On 2/13/24 17:30, Jean Delvare wrote:
> > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
> > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
> 
> FWIW, I agree with Hans on the location of the HW quirks.
> If there is a possible way to make the actual driver cleaner
> and collect quirks somewhere else, I'm full support for that.

Location of the quirks can be moved outside of the i2c-i801.c source
code relatively easily without need to change the way how parent--child
relationship currently works.

Relevant functions is_dell_system_with_lis3lv02d() and
register_dell_lis3lv02d_i2c_device() does not use internals of
i2c-i801 and could be moved into new file, lets say
drivers/platform/x86/dell/dell-smo8800-plat.c
Put this file under a new hidden "bool" config option which is auto
enabled when CONFIG_DELL_SMO8800 is used.

i2c-i801.c currently has code:

	if (is_dell_system_with_lis3lv02d())
		register_dell_lis3lv02d_i2c_device(priv);

This can be put into a new exported function, e.g.
void dell_smo8800_scan_i2c(struct i2c_adapter *adapter);
And i2c-i801.c would call it instead.

register_dell_lis3lv02d_i2c_device just needs "adapter", it does not
need whole i801 priv struct.

With this simple change all dell smo8800 code would be in its subdir
drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code.

This approach does not change any functionality, so should be absolutely
safe.

Future changes will be done only in drivers/platform/x86/dell/ subdir,
touching i801 would not be needed at all.

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

[Andy Shevchenko]

On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote:
> On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote:
> > On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote:
> > > On 2/13/24 17:30, Jean Delvare wrote:
> > > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
> > > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
> >
> > FWIW, I agree with Hans on the location of the HW quirks.
> > If there is a possible way to make the actual driver cleaner
> > and collect quirks somewhere else, I'm full support for that.
>
> Location of the quirks can be moved outside of the i2c-i801.c source
> code relatively easily without need to change the way how parent--child
> relationship currently works.
>
> Relevant functions is_dell_system_with_lis3lv02d() and
> register_dell_lis3lv02d_i2c_device() does not use internals of
> i2c-i801 and could be moved into new file, lets say
> drivers/platform/x86/dell/dell-smo8800-plat.c
> Put this file under a new hidden "bool" config option which is auto
> enabled when CONFIG_DELL_SMO8800 is used.
>
> i2c-i801.c currently has code:
>
>         if (is_dell_system_with_lis3lv02d())
>                 register_dell_lis3lv02d_i2c_device(priv);
>
> This can be put into a new exported function, e.g.
> void dell_smo8800_scan_i2c(struct i2c_adapter *adapter);
> And i2c-i801.c would call it instead.
>
> register_dell_lis3lv02d_i2c_device just needs "adapter", it does not
> need whole i801 priv struct.

I'm wondering why we need all this. We have notifiers when a device is
added / removed. We can provide a board_info for the device and attach
it to the proper adapter, no?

> With this simple change all dell smo8800 code would be in its subdir
> drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code.
>
> This approach does not change any functionality, so should be absolutely
> safe.
>
> Future changes will be done only in drivers/platform/x86/dell/ subdir,
> touching i801 would not be needed at all.

Still these exported functions are not the best solution we can do,
right? We should be able to decouple them without need for the custom
APIs.

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote:
> > On Monday 19 February 2024 13:52:57 Andy Shevchenko wrote:
> > > On Sat, Feb 17, 2024 at 11:33:21AM +0100, Hans de Goede wrote:
> > > > On 2/13/24 17:30, Jean Delvare wrote:
> > > > > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
> > > > >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
> > >
> > > FWIW, I agree with Hans on the location of the HW quirks.
> > > If there is a possible way to make the actual driver cleaner
> > > and collect quirks somewhere else, I'm full support for that.
> >
> > Location of the quirks can be moved outside of the i2c-i801.c source
> > code relatively easily without need to change the way how parent--child
> > relationship currently works.
> >
> > Relevant functions is_dell_system_with_lis3lv02d() and
> > register_dell_lis3lv02d_i2c_device() does not use internals of
> > i2c-i801 and could be moved into new file, lets say
> > drivers/platform/x86/dell/dell-smo8800-plat.c
> > Put this file under a new hidden "bool" config option which is auto
> > enabled when CONFIG_DELL_SMO8800 is used.
> >
> > i2c-i801.c currently has code:
> >
> >         if (is_dell_system_with_lis3lv02d())
> >                 register_dell_lis3lv02d_i2c_device(priv);
> >
> > This can be put into a new exported function, e.g.
> > void dell_smo8800_scan_i2c(struct i2c_adapter *adapter);
> > And i2c-i801.c would call it instead.
> >
> > register_dell_lis3lv02d_i2c_device just needs "adapter", it does not
> > need whole i801 priv struct.
> 
> I'm wondering why we need all this. We have notifiers when a device is
> added / removed. We can provide a board_info for the device and attach
> it to the proper adapter, no?

I do not know how flexible are notifiers. Can notifier call our callback
when new "struct i2c_adapter *adapter" was instanced?

> > With this simple change all dell smo8800 code would be in its subdir
> > drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code.
> >
> > This approach does not change any functionality, so should be absolutely
> > safe.
> >
> > Future changes will be done only in drivers/platform/x86/dell/ subdir,
> > touching i801 would not be needed at all.
> 
> Still these exported functions are not the best solution we can do,
> right? We should be able to decouple them without need for the custom
> APIs.

Well, what I described here is a simple change which get rid of the one
problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx
logic (like adding a new device id) requires touching unrelated
i2c-i801.c source file.

I like small changes which can be easily reviewed and address one
problem. Step by step. That is why I proposed it here.


For decoupling it is needed to get newly instanced adapter (if the
mentioned notifier is able to tell this information) and also it is
needed to check if the adapter is the i801.

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

[Andy Shevchenko]

On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote:
> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote:
> > On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote:

...

> > I'm wondering why we need all this. We have notifiers when a device is
> > added / removed. We can provide a board_info for the device and attach
> > it to the proper adapter, no?
>
> I do not know how flexible are notifiers. Can notifier call our callback
> when new "struct i2c_adapter *adapter" was instanced?

You can follow notifications of *an* I2C adapter being added /
removed. With that, you can filter which one is that. Based on that
you may attach a saved (at __init as you talked about in the reply to
Hans) board_info with all necessary information.

Something like this (combined)
https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515
https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194

> > > With this simple change all dell smo8800 code would be in its subdir
> > > drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code.
> > >
> > > This approach does not change any functionality, so should be absolutely
> > > safe.
> > >
> > > Future changes will be done only in drivers/platform/x86/dell/ subdir,
> > > touching i801 would not be needed at all.
> >
> > Still these exported functions are not the best solution we can do,
> > right? We should be able to decouple them without need for the custom
> > APIs.
>
> Well, what I described here is a simple change which get rid of the one
> problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx
> logic (like adding a new device id) requires touching unrelated
> i2c-i801.c source file.

`get rid of one problem` --> `replace one by another (but maybe less
critical, dunno) problem`. The new one is the spread of custom APIs
for a single user, which also requires an additional, shared header
file and all hell with the Kconfig dependencies.

> I like small changes which can be easily reviewed and address one
> problem. Step by step. That is why I proposed it here.
>
> For decoupling it is needed to get newly instanced adapter (if the
> mentioned notifier is able to tell this information) and also it is
> needed to check if the adapter is the i801.

Yes.


-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 2/27/24 23:37, Andy Shevchenko wrote:
> On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote:
>> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote:
>>> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote:
> 
> ...
> 
>>> I'm wondering why we need all this. We have notifiers when a device is
>>> added / removed. We can provide a board_info for the device and attach
>>> it to the proper adapter, no?
>>
>> I do not know how flexible are notifiers. Can notifier call our callback
>> when new "struct i2c_adapter *adapter" was instanced?
> 
> You can follow notifications of *an* I2C adapter being added /
> removed. With that, you can filter which one is that. Based on that
> you may attach a saved (at __init as you talked about in the reply to
> Hans) board_info with all necessary information.
> 
> Something like this (combined)
> https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515
> https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194

drivers/platform/x86/touchscreen_dmi.c actually already does something
like this for i2c-clients. The problem is that this brings probe-ordering
problems with it. If the i801 driver is loaded before the dell-smo8800
driver then the notifiers will not trigger since the i2c-adapter has
already been created (1).

So we would still need a "cold-plug" manual scan in smo8800_probe()
anyways at which point we might as well just return -EPROBE_DEFER
when the adapter is not there.

As for Pali's suggestion of having the i2c-i801 code call a symbol
exported by dell-smo8800 that will cause the dell-smo8800 driver
to load on all x86 devices with an i2c-i801 controller (pretty
much all of them). Slowing the boot and eating memory.

So IMHO just doing the scan for the i2c-i801 adapter as already
done in this version of the patch-set, extended with returning
-EPROBE_DEFER if it is not found is the best solution.

Yes this means losing the i2c_client for the lis3lv02d device
if the i2c-i801 driver is manually unbound or rmmod-ed. But that
requires explicit root user action and putting just the i801
driver back in place again also requires implicit root action.

IMHO it is acceptable that in this exceptional case, which
normal users will never hit, a rmmod + modprobe of dell-smo8800
is required to re-gain the lis3lv02d i2c_client.

Regards,

Hans


1) touchscreen_dmi is builtin only because of this and we really
want to avoid adding more of that. Actually thinking more about this
it would be nice to modify touchscreen_dmi to use a mix of registering
the notifier + doing a scan after registering it for matching devices
which are already present, so that touchscreen_dmi can become a module
auto-loaded using the DMI info which it already contains.











> 
>>>> With this simple change all dell smo8800 code would be in its subdir
>>>> drivers/platform/x86/dell/ and i2c-i801.c would get rid of smo code.
>>>>
>>>> This approach does not change any functionality, so should be absolutely
>>>> safe.
>>>>
>>>> Future changes will be done only in drivers/platform/x86/dell/ subdir,
>>>> touching i801 would not be needed at all.
>>>
>>> Still these exported functions are not the best solution we can do,
>>> right? We should be able to decouple them without need for the custom
>>> APIs.
>>
>> Well, what I described here is a simple change which get rid of the one
>> problem: i2c-i801.c contains SMO88xx related code and changing SMO88xx
>> logic (like adding a new device id) requires touching unrelated
>> i2c-i801.c source file.
> 
> `get rid of one problem` --> `replace one by another (but maybe less
> critical, dunno) problem`. The new one is the spread of custom APIs
> for a single user, which also requires an additional, shared header
> file and all hell with the Kconfig dependencies.
> 
>> I like small changes which can be easily reviewed and address one
>> problem. Step by step. That is why I proposed it here.
>>
>> For decoupling it is needed to get newly instanced adapter (if the
>> mentioned notifier is able to tell this information) and also it is
>> needed to check if the adapter is the i801.
> 
> Yes.
> 
> 

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

[Pali Rohár]

On Wednesday 28 February 2024 13:50:27 Hans de Goede wrote:
> Hi,
> 
> On 2/27/24 23:37, Andy Shevchenko wrote:
> > On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote:
> >> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote:
> >>> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote:
> > 
> > ...
> > 
> >>> I'm wondering why we need all this. We have notifiers when a device is
> >>> added / removed. We can provide a board_info for the device and attach
> >>> it to the proper adapter, no?
> >>
> >> I do not know how flexible are notifiers. Can notifier call our callback
> >> when new "struct i2c_adapter *adapter" was instanced?
> > 
> > You can follow notifications of *an* I2C adapter being added /
> > removed. With that, you can filter which one is that. Based on that
> > you may attach a saved (at __init as you talked about in the reply to
> > Hans) board_info with all necessary information.
> > 
> > Something like this (combined)
> > https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515
> > https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194
> 
> drivers/platform/x86/touchscreen_dmi.c actually already does something
> like this for i2c-clients. The problem is that this brings probe-ordering
> problems with it. If the i801 driver is loaded before the dell-smo8800
> driver then the notifiers will not trigger since the i2c-adapter has
> already been created (1).
> 
> So we would still need a "cold-plug" manual scan in smo8800_probe()
> anyways at which point we might as well just return -EPROBE_DEFER
> when the adapter is not there.

And that it example why the current existing solution is better, it does
not have such problems like the proposed.

> As for Pali's suggestion of having the i2c-i801 code call a symbol
> exported by dell-smo8800

I did not suggest that! Please do not make false statements about me.

> that will cause the dell-smo8800 driver
> to load on all x86 devices with an i2c-i801 controller (pretty
> much all of them). Slowing the boot and eating memory.

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

[Hans de Goede]

Hi,

On 2/29/24 21:46, Pali Rohár wrote:
> On Wednesday 28 February 2024 13:50:27 Hans de Goede wrote:
>> Hi,
>>
>> On 2/27/24 23:37, Andy Shevchenko wrote:
>>> On Tue, Feb 27, 2024 at 11:50 PM Pali Rohár <pali@kernel.org> wrote:
>>>> On Tuesday 27 February 2024 23:19:19 Andy Shevchenko wrote:
>>>>> On Tue, Feb 27, 2024 at 11:04 PM Pali Rohár <pali@kernel.org> wrote:
>>>
>>> ...
>>>
>>>>> I'm wondering why we need all this. We have notifiers when a device is
>>>>> added / removed. We can provide a board_info for the device and attach
>>>>> it to the proper adapter, no?
>>>>
>>>> I do not know how flexible are notifiers. Can notifier call our callback
>>>> when new "struct i2c_adapter *adapter" was instanced?
>>>
>>> You can follow notifications of *an* I2C adapter being added /
>>> removed. With that, you can filter which one is that. Based on that
>>> you may attach a saved (at __init as you talked about in the reply to
>>> Hans) board_info with all necessary information.
>>>
>>> Something like this (combined)
>>> https://elixir.bootlin.com/linux/latest/source/drivers/ptp/ptp_ocp.c#L4515
>>> https://elixir.bootlin.com/linux/latest/source/drivers/input/mouse/psmouse-smbus.c#L194
>>
>> drivers/platform/x86/touchscreen_dmi.c actually already does something
>> like this for i2c-clients. The problem is that this brings probe-ordering
>> problems with it. If the i801 driver is loaded before the dell-smo8800
>> driver then the notifiers will not trigger since the i2c-adapter has
>> already been created (1).
>>
>> So we would still need a "cold-plug" manual scan in smo8800_probe()
>> anyways at which point we might as well just return -EPROBE_DEFER
>> when the adapter is not there.
> 
> And that it example why the current existing solution is better, it does
> not have such problems like the proposed.
> 
>> As for Pali's suggestion of having the i2c-i801 code call a symbol
>> exported by dell-smo8800
> 
> I did not suggest that! Please do not make false statements about me.

In: https://lore.kernel.org/platform-driver-x86/20240227210429.l5o52wuexqqmrpol@pali/

You write:

"Location of the quirks can be moved outside of the i2c-i801.c source
code relatively easily without need to change the way how parent--child
relationship currently works.

Relevant functions is_dell_system_with_lis3lv02d() and
register_dell_lis3lv02d_i2c_device() does not use internals of
i2c-i801 and could be moved into new file, lets say
drivers/platform/x86/dell/dell-smo8800-plat.c"

Ok I see now you suggest moving the code to a new file,
sorry for misreading that.

But the point is that moving the code does not help because
since there is a symbol used from the new code it will still
get loaded on all machines were the i2c-i801.c driver gets
loaded. So it will still be taking up RAM and increases
boot time (loading an extra module consumes time) on machines
which do not have any SMO88xx devices.

And that point is still valid even independent of the code
is moved to the existing dell-smo8800 module or to a new
dell-smo8800-plat module.

Regards,

Hans

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

[Pali Rohár]

On Saturday 02 March 2024 12:02:39 Hans de Goede wrote:
> But the point is that moving the code does not help because
> since there is a symbol used from the new code it will still
> get loaded on all machines were the i2c-i801.c driver gets
> loaded. So it will still be taking up RAM and increases
> boot time (loading an extra module consumes time) on machines
> which do not have any SMO88xx devices.
> 
> And that point is still valid even independent of the code
> is moved to the existing dell-smo8800 module or to a new
> dell-smo8800-plat module.

This is silly argument if you are opposing to adding trivial exported
function with input argument struct i2c_adapter *adapter and with body

    if (smo88xx_detected)
        i2c_new_client_device(adapter, &info);

with two static global variables:

    struct i2c_board_info info;
    bool smo88xx_detected;

will be compiled and loaded on all x86 machines and taking too much RAM.
Because that design with notifiers and other things would eat much more
RAM and would be also slower.

As I said in previous emails, detection (and so filling those two above
static global variables) can be filled in the __init section and so
would not take after bootup. For detection it is is needed to just call
dmi_match(), acpi_get_devices() and dmi_get_system_info() which can be
done in __init section. I do not see reason why not.

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

[Pali Rohár]

On Saturday 17 February 2024 11:33:21 Hans de Goede wrote:
> Hi Jean,
> 
> On 2/13/24 17:30, Jean Delvare wrote:
> > Hi Pali, Hans,
> > 
> > On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
> >> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
> >>> It is not necessary to handle the Dell specific instantiation of
> >>> i2c_client-s for SMO8xxx 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 Dell specific dell-smo8800 driver.
> >>>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>> 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            | 126 +----------------------
> >>>  drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++-
> >>>  2 files changed, 123 insertions(+), 124 deletions(-)  
> >>
> >> I'm looking at this change again and I'm not not sure if it is a good
> >> direction to do this movement. (...)
> > 
> > Same feeling here. Having to lookup the parent i2c bus, which may or
> > may not be present yet, doesn't feel good.
> > 
> > I wouldn't object if everybody was happy with the move and moving the
> > code was solving an actual issue, but that doesn't seem to be the case.
> 
> I thought you would actually like getting this somewhat clunky code
> which basically works around the hw not being properly described in
> the ACPI tables out of the generic i2c-i801 code.
> 
> I didn't get around to answer's Pali's concerns yet, so let me
> start by addressing those since you indicate that you share Pali's
> concerns:
> 
> Pali wrote:
> > Now after looking at this change again I see there a problem. If i2c-801
> > driver initialize i2c-801 device after this smo8800 is called then
> > accelerometer i2c device would not happen.
> 
> That is a good point (which Jean also points out). But this can simply
> be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER
> if the i2c-i801 i2c-bus is not present yet (all designs using the
> dell-smo8800 driver will have an i2c-bus so waiting for this to show
> up should not cause regressions).

Adding EPROBE_DEFER just complicates the dependency and state model.
I would really suggest to come up with a simpler solution, not too
complicated where it is required to think a lot if is is correct and if
all edge-cases are handled.

> If we can agree to move forward this series I'll fix this.
> 
> Pali wrote:
> > Also it has same problem if PCI i801 device is reloaded or reset.
> 
> The i801 device is not hotplugable, so normally this will never
> happen. If the user manually unbinds + rebinds the i2c-i801 driver
> them the i2c_client for the smo88xx device will indeed get removed
> and not re-added. But this will normally never happen and if
> a user is manually poking things then the user can also unbind +
> rebind the dell-mso8800 driver after the i2c-i801 rebind.
> So I don't really see this as an issue.

Well, rmmod & modprobe is not the rare cases. Whatever developers say
about rmmod (or modprobe -r or whatever is the way for unloading
modules), this is something which is used by a lot of users and would be
used. 

> With those remarks addressed let me try to explain why I think
> that moving this to the dell-smo8800 code is a good idea:
> 
> 1. It is a SMO88xx ACPI device specific kludge and as such IMHO
> thus belongs in the driver for the SMO88xx ACPI platform_device.

I'm not sure if it belongs to "SMO88xx ACPI platform_device" but for
sure I agree with you that it does not belong to i801 code. I would say
that it belongs to some SMO8800 glue code -- because it is not the
classic ACPI driver too. But I'm not against to have SMO glue code and
SMO ACPI driver in one file (maybe it is even better to have it).

> The i2c-i801 driver gets loaded on every x86 system and it is
> undesirable to have this extra code and the DMI table in RAM
> on all those other systems.

I think we can take an assumption that ACPI SMO device does not change
it existence or ACPI enabled/disabled state during runtime. So we can
scan for ACPI SMO device just once in function stored in __init section
called during the kernel/module initialization and cache the result
(bool if device was found + its i2c address). After function marked as
__init finish its job then together with DMI tables can be discarded
from RAM. With this way it does take extra memory on every x86 system.
Also we can combine this with an SMO config option, so the whole code
"glue" code would not be compiled when SMO driver is not enabled via
Kconfig.

> 2. Further changes in this series, like adding support for
> probing for the i2c address of the lis3lv02d device on models
> not yet in the DMI table, will add a bunch of more code specific
> to SMO88xx ACPI devices. Making the problem of having SMO88xx
> specific code in the generic i2c-i801 driver even bigger.
> The current amount of SMO88xx specific code in the
> generic i2c-i801 driver might be considered acceptable but I'm
> afraid that the amount of code after this series will not be
> acceptable.

I think alternative approach which I described in the other email in
this thread could be useful for this issue too (to move SMO code from
i2c-i801.c source file). Together with above __init section approach it
can also decrease memory usage.

> 3. Some of the changes in this series are harder to implement inside
> the i2c-i801 code, like optionally instantiating an i2c_client for
> the IIO st_accel driver (*) so that the accelerometer gets presented
> to userspace as a standard IIO device like all modern accelerometer
> drivers do.

This is something about which I'm not very convinced. IIO st_accel
driver does not support freefall interface (or any other for signalling
hard disk falls, which is used by userspace) and in dell systems, this
hard disk protection is the primary usage of the accelerometer.

In last two months I talked with two people, users of the accelerometers
axis on dell and thinkpad machines. They were using it in games which
were joystick-based (one game was tuxracer, second I do not remember
name).

So I'm not sure that replacing joystick driver by some new API would be
really useful for users of accelerometer axis.

Before such change I would propose to teach IIO st_accel driver (or what
would be the replacement) to support joystick API for userspace.

> This requires setting i2c_client.irq and that IRQ comes from
> the SMO88xx ACPI device. So this would require the i2c-i801 code
> to lookup the ACPI device and get the IRQ from there. Where as
> in the SMO88xx ACPI platform_device driver the IRQ is readily
> available.

I understand this problem.

But I would like to ask a question: WHY it is needed at all?
The IRQ represents the free fall / hard disk fall event, which is
slightly different thing than reporting accelerometer axis.
Why IIO st_accel (or lis3lv02d) needs free fall IRQ?

Hard disk fall interrupt on Dell machines can be handled by separate
driver, not related to ACPI SMO8800 device.

It would be much more easier to split these two different
functionalities (reporting axes; and reporting hard disk fall event)
into two separate drivers. And it would simplify whole logic related to
instantiating free fall hard disk driver and accelerometer axes driver
(either IIO st_accel or lis3lv02d or some other...).

So from my side, I do not see a reason to "inject" IRQ number into
driver which reads accelerometer axes.

> TL;DR: IMHO all this SMO88xx quirk/glue handling belongs in
> the SMO88xx specific dell-smo8800 driver rather then in
> the generic i2c-i801 code.

I agree, that it does not belong to the i2c-i801.c source file. And I
also would like to see movement. That is why I proposed alternative and
simpler solution.

> Regards,
> 
> Hans
> 
> 
> *) Instead of an i2c_client for the somewhat weird (but still
> default for backward compat) drivers/misc/lis3lv02d/lis3lv02d.c
> driver
> 
> 
> 
> 
> 
> 
> 

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

[Hans de Goede]

Hi Pali,

On 2/27/24 22:40, Pali Rohár wrote:
> On Saturday 17 February 2024 11:33:21 Hans de Goede wrote:
>> Hi Jean,
>>
>> On 2/13/24 17:30, Jean Delvare wrote:
>>> Hi Pali, Hans,
>>>
>>> On Sun, 7 Jan 2024 18:10:55 +0100, Pali Rohár wrote:
>>>> On Saturday 06 January 2024 17:09:29 Hans de Goede wrote:
>>>>> It is not necessary to handle the Dell specific instantiation of
>>>>> i2c_client-s for SMO8xxx 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 Dell specific dell-smo8800 driver.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>> 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            | 126 +----------------------
>>>>>  drivers/platform/x86/dell/dell-smo8800.c | 121 +++++++++++++++++++++-
>>>>>  2 files changed, 123 insertions(+), 124 deletions(-)  
>>>>
>>>> I'm looking at this change again and I'm not not sure if it is a good
>>>> direction to do this movement. (...)
>>>
>>> Same feeling here. Having to lookup the parent i2c bus, which may or
>>> may not be present yet, doesn't feel good.
>>>
>>> I wouldn't object if everybody was happy with the move and moving the
>>> code was solving an actual issue, but that doesn't seem to be the case.
>>
>> I thought you would actually like getting this somewhat clunky code
>> which basically works around the hw not being properly described in
>> the ACPI tables out of the generic i2c-i801 code.
>>
>> I didn't get around to answer's Pali's concerns yet, so let me
>> start by addressing those since you indicate that you share Pali's
>> concerns:
>>
>> Pali wrote:
>>> Now after looking at this change again I see there a problem. If i2c-801
>>> driver initialize i2c-801 device after this smo8800 is called then
>>> accelerometer i2c device would not happen.
>>
>> That is a good point (which Jean also points out). But this can simply
>> be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER
>> if the i2c-i801 i2c-bus is not present yet (all designs using the
>> dell-smo8800 driver will have an i2c-bus so waiting for this to show
>> up should not cause regressions).
> 
> Adding EPROBE_DEFER just complicates the dependency and state model.
> I would really suggest to come up with a simpler solution, not too
> complicated where it is required to think a lot if is is correct and if
> all edge-cases are handled.

I'm not sure what you are worried about here. dell-smo8800 is
a leave driver, nothing inside the kernel depends on it being 
loaded or not. So there are no -EPROBE_DEFER complexities here,
yes -EPROBE_DEFER can become a bit hairy with complex dependency
graphs, but this is a very KISS case.

Are there any specific scenarios you are actually worried about
in this specific case?

>> If we can agree to move forward this series I'll fix this.
>>
>> Pali wrote:
>>> Also it has same problem if PCI i801 device is reloaded or reset.
>>
>> The i801 device is not hotplugable, so normally this will never
>> happen. If the user manually unbinds + rebinds the i2c-i801 driver
>> them the i2c_client for the smo88xx device will indeed get removed
>> and not re-added. But this will normally never happen and if
>> a user is manually poking things then the user can also unbind +
>> rebind the dell-mso8800 driver after the i2c-i801 rebind.
>> So I don't really see this as an issue.
> 
> Well, rmmod & modprobe is not the rare cases. Whatever developers say
> about rmmod (or modprobe -r or whatever is the way for unloading
> modules), this is something which is used by a lot of users and would be
> used. 

Many modules actually have bugs in there remove paths and crash,
this is really not a common case. Sometimes users use rmmod + modprobe
surrounding suspend/resume for e.g. a wifi driver to work around
suspend/resume problems but I have never heard of this being used
for a driver like i2c-i801.

And again if users are manually meddling with this, the they can
also rmmod + modprobe dell-smo8800 after re-modprobing i2c-i801.

>> With those remarks addressed let me try to explain why I think
>> that moving this to the dell-smo8800 code is a good idea:
>>
>> 1. It is a SMO88xx ACPI device specific kludge and as such IMHO
>> thus belongs in the driver for the SMO88xx ACPI platform_device.
> 
> I'm not sure if it belongs to "SMO88xx ACPI platform_device" but for
> sure I agree with you that it does not belong to i801 code. I would say
> that it belongs to some SMO8800 glue code -- because it is not the
> classic ACPI driver too. But I'm not against to have SMO glue code and
> SMO ACPI driver in one file (maybe it is even better to have it).

We are trying to get rid of acpi_driver drivers using only
platform_driver drivers for the platform_devices created for
ACPI devices / fwnodes which do not have another physical device.

Also we only want this workaround when the SMO88xx ACPI fwnode
is missing the I2cSerialBusV2 resource for the i2c_client and
conveniently the platform_device will only be created for
ACPI fwnodes without the I2cSerialBusV2 resource for proper
ACPI fwnodes which have the I2C resource an i2c_client will
be created instead. So putting the workaround in
the platform_driver automatically ensures that it only runs
when the ACPI fwnode is incomplete.


> 
>> The i2c-i801 driver gets loaded on every x86 system and it is
>> undesirable to have this extra code and the DMI table in RAM
>> on all those other systems.
> 
> I think we can take an assumption that ACPI SMO device does not change
> it existence or ACPI enabled/disabled state during runtime. So we can
> scan for ACPI SMO device just once in function stored in __init section
> called during the kernel/module initialization and cache the result
> (bool if device was found + its i2c address). After function marked as
> __init finish its job then together with DMI tables can be discarded
> from RAM. With this way it does take extra memory on every x86 system.
> Also we can combine this with an SMO config option, so the whole code
> "glue" code would not be compiled when SMO driver is not enabled via
> Kconfig.

This approach does not work because i2c-i801.c registers a PCI driver,
there is no guarantee that the adapter has already been probed and
an i2c_adapter has been created before it. A PCI driver's probe()
function must not be __init and thus any code which it calls also
must not be __init.

So the majority of the smo88xx handling can not be __init.

Regards,

Hans

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

[Andy Shevchenko]

On Wed, Feb 28, 2024 at 3:10 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 2/27/24 22:40, Pali Rohár wrote:
> > On Saturday 17 February 2024 11:33:21 Hans de Goede wrote:
> >> On 2/13/24 17:30, Jean Delvare wrote:

...

> >> The i801 device is not hotplugable, so normally this will never
> >> happen. If the user manually unbinds + rebinds the i2c-i801 driver
> >> them the i2c_client for the smo88xx device will indeed get removed
> >> and not re-added. But this will normally never happen and if
> >> a user is manually poking things then the user can also unbind +
> >> rebind the dell-mso8800 driver after the i2c-i801 rebind.
> >> So I don't really see this as an issue.
> >
> > Well, rmmod & modprobe is not the rare cases. Whatever developers say
> > about rmmod (or modprobe -r or whatever is the way for unloading
> > modules), this is something which is used by a lot of users and would be
> > used.
>
> Many modules actually have bugs in there remove paths and crash,
> this is really not a common case. Sometimes users use rmmod + modprobe
> surrounding suspend/resume for e.g. a wifi driver to work around
> suspend/resume problems but I have never heard of this being used
> for a driver like i2c-i801.

Hmm... The whole thing of reworking the p2sb was done due to
rebounding the i2c-i801 IIUC.

-- 
With Best Regards,
Andy Shevchenko

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

[Pali Rohár]

On Wednesday 28 February 2024 14:10:14 Hans de Goede wrote:
> >>> Now after looking at this change again I see there a problem. If i2c-801
> >>> driver initialize i2c-801 device after this smo8800 is called then
> >>> accelerometer i2c device would not happen.
> >>
> >> That is a good point (which Jean also points out). But this can simply
> >> be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER
> >> if the i2c-i801 i2c-bus is not present yet (all designs using the
> >> dell-smo8800 driver will have an i2c-bus so waiting for this to show
> >> up should not cause regressions).
> > 
> > Adding EPROBE_DEFER just complicates the dependency and state model.
> > I would really suggest to come up with a simpler solution, not too
> > complicated where it is required to think a lot if is is correct and if
> > all edge-cases are handled.
> 
> I'm not sure what you are worried about here. dell-smo8800 is
> a leave driver, nothing inside the kernel depends on it being 
> loaded or not. So there are no -EPROBE_DEFER complexities here,
> yes -EPROBE_DEFER can become a bit hairy with complex dependency
> graphs, but this is a very KISS case.
> 
> Are there any specific scenarios you are actually worried about
> in this specific case?

-EPROBE_DEFER restarts and schedule probing of the device later. It does
not inform device manager when it can try do it. So it can try probing
it many more times until it decide to not try it again. This
asynchronous design is broken and I do not see reason why to use it in
another driver, in case we have a cleaner solution how to initialize and
probe device without -EPROBE_DEFER. This is for sure not a KISS case
but a case with lot of corner cases... and your proposed patch is just
an example of it as you forgot about at least one corner case. Current
solution does not have edge cases... this can be marked as KISS design.

> >> If we can agree to move forward this series I'll fix this.
> >>
> >> Pali wrote:
> >>> Also it has same problem if PCI i801 device is reloaded or reset.
> >>
> >> The i801 device is not hotplugable, so normally this will never
> >> happen. If the user manually unbinds + rebinds the i2c-i801 driver
> >> them the i2c_client for the smo88xx device will indeed get removed
> >> and not re-added. But this will normally never happen and if
> >> a user is manually poking things then the user can also unbind +
> >> rebind the dell-mso8800 driver after the i2c-i801 rebind.
> >> So I don't really see this as an issue.
> > 
> > Well, rmmod & modprobe is not the rare cases. Whatever developers say
> > about rmmod (or modprobe -r or whatever is the way for unloading
> > modules), this is something which is used by a lot of users and would be
> > used. 
> 
> Many modules actually have bugs in there remove paths and crash,
> this is really not a common case. Sometimes users use rmmod + modprobe
> surrounding suspend/resume for e.g. a wifi driver to work around
> suspend/resume problems but I have never heard of this being used
> for a driver like i2c-i801.
> 
> And again if users are manually meddling with this, the they can
> also rmmod + modprobe dell-smo8800 after re-modprobing i2c-i801.

Argument that other modules have bugs in some code path does not mean to
introduce bugs also into other modules. I do not take it.

> >> The i2c-i801 driver gets loaded on every x86 system and it is
> >> undesirable to have this extra code and the DMI table in RAM
> >> on all those other systems.
> > 
> > I think we can take an assumption that ACPI SMO device does not change
> > it existence or ACPI enabled/disabled state during runtime. So we can
> > scan for ACPI SMO device just once in function stored in __init section
> > called during the kernel/module initialization and cache the result
> > (bool if device was found + its i2c address). After function marked as
> > __init finish its job then together with DMI tables can be discarded
> > from RAM. With this way it does take extra memory on every x86 system.
> > Also we can combine this with an SMO config option, so the whole code
> > "glue" code would not be compiled when SMO driver is not enabled via
> > Kconfig.
> 
> This approach does not work because i2c-i801.c registers a PCI driver,
> there is no guarantee that the adapter has already been probed and
> an i2c_adapter has been created before it. A PCI driver's probe()
> function must not be __init and thus any code which it calls also
> must not be __init.
> 
> So the majority of the smo88xx handling can not be __init.

This argument is wrong. smo88xx has nothing with PCI, has even nothing
with i2c. The detection is purely ACPI based and this can be called at
any time after ACPI initialization. Detection does not need PCI. There
is no reason why it cannot be called in __init section after ACPI is
done.

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

[Hans de Goede]

Hi,

On 2/29/24 21:57, Pali Rohár wrote:
> On Wednesday 28 February 2024 14:10:14 Hans de Goede wrote:
>>>>> Now after looking at this change again I see there a problem. If i2c-801
>>>>> driver initialize i2c-801 device after this smo8800 is called then
>>>>> accelerometer i2c device would not happen.
>>>>
>>>> That is a good point (which Jean also points out). But this can simply
>>>> be fixed by making the dell-smo8800's probe() method return -EPROBE_DEFER
>>>> if the i2c-i801 i2c-bus is not present yet (all designs using the
>>>> dell-smo8800 driver will have an i2c-bus so waiting for this to show
>>>> up should not cause regressions).
>>>
>>> Adding EPROBE_DEFER just complicates the dependency and state model.
>>> I would really suggest to come up with a simpler solution, not too
>>> complicated where it is required to think a lot if is is correct and if
>>> all edge-cases are handled.
>>
>> I'm not sure what you are worried about here. dell-smo8800 is
>> a leave driver, nothing inside the kernel depends on it being 
>> loaded or not. So there are no -EPROBE_DEFER complexities here,
>> yes -EPROBE_DEFER can become a bit hairy with complex dependency
>> graphs, but this is a very KISS case.
>>
>> Are there any specific scenarios you are actually worried about
>> in this specific case?
> 
> -EPROBE_DEFER restarts and schedule probing of the device later. It does
> not inform device manager when it can try do it. So it can try probing
> it many more times until it decide to not try it again.

"until it decide to not try it again" is not how the kernel's EPROBE_DEFER
mechanism works. It will queue a new re-probe of all devices on the
deferred probe list whenever another driver's probe() method succeeds.

So once i801_probe() returns success, the dell-smo8800 driver's probe()
will be tried again and at that point the i2c-i801 i2c_adapter exists
and it will succeed.

Yes the dell-smo8800 driver's probe() may be called multiple times
before i801_probe(), but that is not an issue.

It is guaranteed that the dell-smo8800 driver's probe() will be called
at least once after i801_probe() succeeds.

> This
> asynchronous design is broken and I do not see reason why to use it in
> another driver

EPROBE_DEFER is used in other cases on x86 platforms too and it is
used a whole lot on ARM platforms. If you consider EPROBE_DEFER
fundamentally broken then that is a whole other discussion and
frankly that is out of scope for this discussion. EPROBE_DEFER
is a widely used and proven mechanism. Arguing that this patch
cannot move forward because EPROBE_DEFER has generic issues really
is out of scope.

>>>> If we can agree to move forward this series I'll fix this.
>>>>
>>>> Pali wrote:
>>>>> Also it has same problem if PCI i801 device is reloaded or reset.
>>>>
>>>> The i801 device is not hotplugable, so normally this will never
>>>> happen. If the user manually unbinds + rebinds the i2c-i801 driver
>>>> them the i2c_client for the smo88xx device will indeed get removed
>>>> and not re-added. But this will normally never happen and if
>>>> a user is manually poking things then the user can also unbind +
>>>> rebind the dell-mso8800 driver after the i2c-i801 rebind.
>>>> So I don't really see this as an issue.
>>>
>>> Well, rmmod & modprobe is not the rare cases. Whatever developers say
>>> about rmmod (or modprobe -r or whatever is the way for unloading
>>> modules), this is something which is used by a lot of users and would be
>>> used. 
>>
>> Many modules actually have bugs in there remove paths and crash,
>> this is really not a common case. Sometimes users use rmmod + modprobe
>> surrounding suspend/resume for e.g. a wifi driver to work around
>> suspend/resume problems but I have never heard of this being used
>> for a driver like i2c-i801.
>>
>> And again if users are manually meddling with this, the they can
>> also rmmod + modprobe dell-smo8800 after re-modprobing i2c-i801.
> 
> Argument that other modules have bugs in some code path does not mean to
> introduce bugs also into other modules. I do not take it.

My remark about many modules having bugs in their remove() path
was to counter your argument that people do manual rmmod-s all
the time.

But how many people do or do not do manual rmmods is not
the fundamental point here.

The fundamental point is that if users make manual rmmod calls then
they already need to also manually undo the results of the rmmod call.
So now they will also need to reload dell-smo8800 driver as part of
the manual undoing. I really don't see a problem with that. Users
should not be unloading (and 99% is not unloading) the i2c-i801 driver
in the first place.


>>>> The i2c-i801 driver gets loaded on every x86 system and it is
>>>> undesirable to have this extra code and the DMI table in RAM
>>>> on all those other systems.
>>>
>>> I think we can take an assumption that ACPI SMO device does not change
>>> it existence or ACPI enabled/disabled state during runtime. So we can
>>> scan for ACPI SMO device just once in function stored in __init section
>>> called during the kernel/module initialization and cache the result
>>> (bool if device was found + its i2c address). After function marked as
>>> __init finish its job then together with DMI tables can be discarded
>>> from RAM. With this way it does take extra memory on every x86 system.
>>> Also we can combine this with an SMO config option, so the whole code
>>> "glue" code would not be compiled when SMO driver is not enabled via
>>> Kconfig.
>>
>> This approach does not work because i2c-i801.c registers a PCI driver,
>> there is no guarantee that the adapter has already been probed and
>> an i2c_adapter has been created before it. A PCI driver's probe()
>> function must not be __init and thus any code which it calls also
>> must not be __init.
>>
>> So the majority of the smo88xx handling can not be __init.
> 
> This argument is wrong. smo88xx has nothing with PCI, has even nothing
> with i2c. The detection is purely ACPI based and this can be called at
> any time after ACPI initialization. Detection does not need PCI. There
> is no reason why it cannot be called in __init section after ACPI is
> done.

My patch series adds support for probing the i2c-address to make it
easier for users to check what the address of the lis3lv02d chip
on their laptop model is.

This probing requires access to the actual i2c_adapter which is
a PCI device. So this can only run after the PCI-driver for the
i2c-i801 bus has bound, which means after the probe() from the
PCI driver so it cannot be __init code.

Pali I'm getting the feeling that you have dug in your heels that:

1. Current approach is good
2. Hans' new approach is bad

And that you are not really given my arguments why moving
the code out of the i2c-i801 driver is a good idea a fair hearing.

I would like you to try and take some distance from this and
look at this with more of a helicopter view.

As I mentioned earlier in the thread and as Andy has agreed
with my main motivation for moving the handling of the i2c_client
instantation is that this is a SMO88xx ACPI device specific kludge
and as such IMHO thus belongs in the driver for the SMO88xx ACPI
platform_device.

Had I been involved in (and have the knowledge of kernel internals
I have now) the original i2c-i801.c SMO88xx ACPI device changes
then I would likely have nacked them.

Putting this sort of highly device specific code into generic
drivers like the i2c-i801 code does not scale. What if tomorrow
we find some other ACPI device with similar issues are we then
going to add yet another kludge to the generic shared i2c-i801 code ?

Also note that the i2c-i801.c code already is triggered by
the presence of certain ACPI hw-ids and we already have a
mechanism to only load code based on ACPI hw-ids (1), that is
have a platform_driver with an acpi_match table for those ids,
which is exactly the mechanism my new approach is using.

From a design perspective the handling of all of this
*very obviously* belongs in a driver actually binding to
these ACPI ids and my suggested changes are actually
following this, what IMHO is the only proper way to handle
this.

Now if there were big problems with my suggested approach
then I could understand your reluctance.

But the only real problem you have pointed out is that
if people *manually* rmmod i2c-i801 that then after *manually*
modprobing i2c-i801 again the i2c_client for the lis3lv02d chip
is not automatically re-instated, instead they will need to
also manually reload the dell-smo8800 driver. Which IMHO
really is not an issue since they are already manually messing
with drivers anyways.

And note that even that problem could be fixed by using
bus-notifiers as Andy suggested. IMHO using notifiers here is
overkill. But if you are ok with moving this code out of i2c-i801
and intel dell-smo8800 if I use notifiers in the next version so
that things will keep working even after a *manual* rmmod of
i2c-i801 then I'll do so for v3 of the patchset.

Regards,

Hans


1) The fact alone that the old approach requires manually
syncing the 2 copies of the ACPI hw-id tables already
indicates that the i2c-i801 code is not the right place
for this functionality.

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

[Andy Shevchenko]

On Sat, Mar 2, 2024 at 1:38 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 2/29/24 21:57, Pali Rohár wrote:

...

> But the only real problem you have pointed out is that
> if people *manually* rmmod i2c-i801 that then after *manually*
> modprobing i2c-i801 again the i2c_client for the lis3lv02d chip
> is not automatically re-instated, instead they will need to
> also manually reload the dell-smo8800 driver. Which IMHO
> really is not an issue since they are already manually messing
> with drivers anyways.

Actually we probably can add some code to the i2c core to list the
clients that were instantiated in a way that can't be done again after
removing the respective bus driver. But this is just an idea on how to
inform users what exactly can go wrong after the rmmod/modprobe cycle,
I haven't investigated this anyhow.

> And note that even that problem could be fixed by using
> bus-notifiers as Andy suggested. IMHO using notifiers here is
> overkill. But if you are ok with moving this code out of i2c-i801
> and intel dell-smo8800 if I use notifiers in the next version so
> that things will keep working even after a *manual* rmmod of
> i2c-i801 then I'll do so for v3 of the patchset.

-- 
With Best Regards,
Andy Shevchenko

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

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build warnings:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/platform-x86-dell-smo8800-Change-probe-ordering-a-bit/20240107-001715
base:   linus/master
patch link:    https://lore.kernel.org/r/20240106160935.45487-3-hdegoede%40redhat.com
patch subject: [PATCH v2 2/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800
config: x86_64-buildonly-randconfig-001-20240113 (https://download.01.org/0day-ci/archive/20240113/202401131227.HL4y41DY-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240113/202401131227.HL4y41DY-lkp@intel.com/reproduce)

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

All warnings (new ones prefixed by >>):

   In function 'smo8800_instantiate_i2c_client',
       inlined from 'smo8800_probe' at drivers/platform/x86/dell/dell-smo8800.c:240:2:
>> drivers/platform/x86/dell/dell-smo8800.c:188:21: warning: argument 1 null where non-null expected [-Wnonnull]
     188 |                 if (strcmp(dmi_product_name,
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~
     189 |                            dell_lis3lv02d_devices[i].dmi_product_name) == 0) {
         |                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   In file included from include/linux/bitmap.h:12,
                    from include/linux/cpumask.h:12,
                    from include/linux/smp.h:13,
                    from include/linux/lockdep.h:14,
                    from include/linux/mutex.h:17,
                    from include/linux/kernfs.h:11,
                    from include/linux/sysfs.h:16,
                    from include/linux/kobject.h:20,
                    from include/linux/device/bus.h:17,
                    from drivers/platform/x86/dell/dell-smo8800.c:14:
   include/linux/string.h: In function 'smo8800_probe':
   include/linux/string.h:89:12: note: in a call to function 'strcmp' declared 'nonnull'
      89 | extern int strcmp(const char *,const char *);
         |            ^~~~~~


vim +188 drivers/platform/x86/dell/dell-smo8800.c

   173	
   174	static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
   175	{
   176		struct i2c_board_info info = { };
   177		struct i2c_adapter *adap = NULL;
   178		const char *dmi_product_name;
   179		u8 addr = 0;
   180		int i;
   181	
   182		bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
   183		if (!adap)
   184			return;
   185	
   186		dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
   187		for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
 > 188			if (strcmp(dmi_product_name,
   189				   dell_lis3lv02d_devices[i].dmi_product_name) == 0) {
   190				addr = dell_lis3lv02d_devices[i].i2c_addr;
   191				break;
   192			}
   193		}
   194	
   195		if (!addr) {
   196			dev_warn(smo8800->dev,
   197				 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
   198			goto put_adapter;
   199		}
   200	
   201		info.addr = addr;
   202		strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
   203	
   204		smo8800->i2c_dev = i2c_new_client_device(adap, &info);
   205		if (IS_ERR(smo8800->i2c_dev)) {
   206			dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev),
   207				      "registering accel i2c_client\n");
   208			smo8800->i2c_dev = NULL;
   209		} else {
   210			dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n",
   211				 info.type, info.addr);
   212		}
   213	put_adapter:
   214		i2c_put_adapter(adap);
   215	}
   216	

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

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

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build errors:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/platform-x86-dell-smo8800-Change-probe-ordering-a-bit/20240107-001715
base:   linus/master
patch link:    https://lore.kernel.org/r/20240106160935.45487-3-hdegoede%40redhat.com
patch subject: [PATCH v2 2/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800
config: i386-buildonly-randconfig-005-20240113 (https://download.01.org/0day-ci/archive/20240113/202401131552.PbjGXHjA-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240113/202401131552.PbjGXHjA-lkp@intel.com/reproduce)

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

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

   drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_find_i801':
>> drivers/platform/x86/dell/dell-smo8800.c:144:21: error: implicit declaration of function 'i2c_get_adapter'; did you mean 'i2c_get_adapdata'? [-Werror=implicit-function-declaration]
     144 |         *adap_ret = i2c_get_adapter(adap->nr);
         |                     ^~~~~~~~~~~~~~~
         |                     i2c_get_adapdata
>> drivers/platform/x86/dell/dell-smo8800.c:144:19: warning: assignment to 'struct i2c_adapter *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     144 |         *adap_ret = i2c_get_adapter(adap->nr);
         |                   ^
   drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_instantiate_i2c_client':
>> drivers/platform/x86/dell/dell-smo8800.c:204:28: error: implicit declaration of function 'i2c_new_client_device' [-Werror=implicit-function-declaration]
     204 |         smo8800->i2c_dev = i2c_new_client_device(adap, &info);
         |                            ^~~~~~~~~~~~~~~~~~~~~
>> drivers/platform/x86/dell/dell-smo8800.c:204:26: warning: assignment to 'struct i2c_client *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     204 |         smo8800->i2c_dev = i2c_new_client_device(adap, &info);
         |                          ^
>> drivers/platform/x86/dell/dell-smo8800.c:214:9: error: implicit declaration of function 'i2c_put_adapter' [-Werror=implicit-function-declaration]
     214 |         i2c_put_adapter(adap);
         |         ^~~~~~~~~~~~~~~
   drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_probe':
>> drivers/platform/x86/dell/dell-smo8800.c:267:9: error: implicit declaration of function 'i2c_unregister_device'; did you mean 'pci_unregister_driver'? [-Werror=implicit-function-declaration]
     267 |         i2c_unregister_device(smo8800->i2c_dev);
         |         ^~~~~~~~~~~~~~~~~~~~~
         |         pci_unregister_driver
   cc1: some warnings being treated as errors


vim +144 drivers/platform/x86/dell/dell-smo8800.c

   129	
   130	static int smo8800_find_i801(struct device *dev, void *data)
   131	{
   132		struct i2c_adapter *adap, **adap_ret = data;
   133	
   134		adap = i2c_verify_adapter(dev);
   135		if (!adap)
   136			return 0;
   137	
   138		if (!strstarts(adap->name, "SMBus I801 adapter"))
   139			return 0;
   140	
   141		if (pci_match_id(i801_idf_ids, to_pci_dev(adap->dev.parent)))
   142			return 0; /* Only register client on main SMBus channel */
   143	
 > 144		*adap_ret = i2c_get_adapter(adap->nr);
   145		return 1;
   146	}
   147	
   148	/*
   149	 * Accelerometer's I2C address is not specified in DMI nor ACPI,
   150	 * so it is needed to define mapping table based on DMI product names.
   151	 */
   152	static const struct {
   153		const char *dmi_product_name;
   154		unsigned short i2c_addr;
   155	} dell_lis3lv02d_devices[] = {
   156		/*
   157		 * Dell platform team told us that these Latitude devices have
   158		 * ST microelectronics accelerometer at I2C address 0x29.
   159		 */
   160		{ "Latitude E5250",     0x29 },
   161		{ "Latitude E5450",     0x29 },
   162		{ "Latitude E5550",     0x29 },
   163		{ "Latitude E6440",     0x29 },
   164		{ "Latitude E6440 ATG", 0x29 },
   165		{ "Latitude E6540",     0x29 },
   166		/*
   167		 * Additional individual entries were added after verification.
   168		 */
   169		{ "Latitude 5480",      0x29 },
   170		{ "Vostro V131",        0x1d },
   171		{ "Vostro 5568",        0x29 },
   172	};
   173	
   174	static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
   175	{
   176		struct i2c_board_info info = { };
   177		struct i2c_adapter *adap = NULL;
   178		const char *dmi_product_name;
   179		u8 addr = 0;
   180		int i;
   181	
   182		bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
   183		if (!adap)
   184			return;
   185	
   186		dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
   187		for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
   188			if (strcmp(dmi_product_name,
   189				   dell_lis3lv02d_devices[i].dmi_product_name) == 0) {
   190				addr = dell_lis3lv02d_devices[i].i2c_addr;
   191				break;
   192			}
   193		}
   194	
   195		if (!addr) {
   196			dev_warn(smo8800->dev,
   197				 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
   198			goto put_adapter;
   199		}
   200	
   201		info.addr = addr;
   202		strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
   203	
 > 204		smo8800->i2c_dev = i2c_new_client_device(adap, &info);
   205		if (IS_ERR(smo8800->i2c_dev)) {
   206			dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev),
   207				      "registering accel i2c_client\n");
   208			smo8800->i2c_dev = NULL;
   209		} else {
   210			dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n",
   211				 info.type, info.addr);
   212		}
   213	put_adapter:
 > 214		i2c_put_adapter(adap);
   215	}
   216	
   217	static int smo8800_probe(struct platform_device *device)
   218	{
   219		int err;
   220		struct smo8800_device *smo8800;
   221	
   222		smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL);
   223		if (!smo8800) {
   224			dev_err(&device->dev, "failed to allocate device data\n");
   225			return -ENOMEM;
   226		}
   227	
   228		smo8800->dev = &device->dev;
   229		smo8800->miscdev.minor = MISC_DYNAMIC_MINOR;
   230		smo8800->miscdev.name = "freefall";
   231		smo8800->miscdev.fops = &smo8800_misc_fops;
   232	
   233		init_waitqueue_head(&smo8800->misc_wait);
   234	
   235		err = platform_get_irq(device, 0);
   236		if (err < 0)
   237			return err;
   238		smo8800->irq = err;
   239	
   240		smo8800_instantiate_i2c_client(smo8800);
   241	
   242		err = misc_register(&smo8800->miscdev);
   243		if (err) {
   244			dev_err(&device->dev, "failed to register misc dev: %d\n", err);
   245			goto error_unregister_i2c_client;
   246		}
   247	
   248		err = request_threaded_irq(smo8800->irq, smo8800_interrupt_quick,
   249					   smo8800_interrupt_thread,
   250					   IRQF_TRIGGER_RISING | IRQF_ONESHOT,
   251					   DRIVER_NAME, smo8800);
   252		if (err) {
   253			dev_err(&device->dev,
   254				"failed to request thread for IRQ %d: %d\n",
   255				smo8800->irq, err);
   256			goto error;
   257		}
   258	
   259		dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
   260			 smo8800->irq);
   261		platform_set_drvdata(device, smo8800);
   262		return 0;
   263	
   264	error:
   265		misc_deregister(&smo8800->miscdev);
   266	error_unregister_i2c_client:
 > 267		i2c_unregister_device(smo8800->i2c_dev);
   268		return err;
   269	}
   270	

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

[PATCH v2 3/6] platform/x86: dell-smo8800: Pass the IRQ to the lis3lv02d i2c_client

[Hans de Goede]

When a lis3lv02d i2c_client has been instantiated pass the IRQ to
the i2c_client and let the lis3lv02d driver take care of registering
/dev/freefall and handling the IRQ.

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

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index c674e3392270..3e64ebcf4b21 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -199,6 +199,7 @@ static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 	}
 
 	info.addr = addr;
+	info.irq = smo8800->irq;
 	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
 
 	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
@@ -239,21 +240,24 @@ static int smo8800_probe(struct platform_device *device)
 
 	smo8800_instantiate_i2c_client(smo8800);
 
-	err = misc_register(&smo8800->miscdev);
-	if (err) {
-		dev_err(&device->dev, "failed to register misc dev: %d\n", err);
-		goto error_unregister_i2c_client;
-	}
+	/* smo8800->irq is passed to the i2c_client and its driver will take care of this */
+	if (!smo8800->i2c_dev) {
+		err = misc_register(&smo8800->miscdev);
+		if (err) {
+			dev_err(&device->dev, "failed to register misc dev: %d\n", err);
+			goto error_unregister_i2c_client;
+		}
 
-	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;
+		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",
@@ -272,9 +276,11 @@ static void smo8800_remove(struct platform_device *device)
 {
 	struct smo8800_device *smo8800 = platform_get_drvdata(device);
 
-	free_irq(smo8800->irq, smo8800);
-	misc_deregister(&smo8800->miscdev);
-	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
+	if (!smo8800->i2c_dev) {
+		free_irq(smo8800->irq, smo8800);
+		misc_deregister(&smo8800->miscdev);
+		dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
+	}
 	i2c_unregister_device(smo8800->i2c_dev);
 }
 
-- 
2.43.0

[PATCH v2 4/6] platform/x86: dell-smo8800: Allow using the IIO st_accel driver

[Hans de Goede]

Allow using the IIO st_accel driver so that the accelerometer gets
presented to userspace as an IIO device like all modern accelerometer
drivers do.

Note the default is still to use the drivers/misc/lis3lv02d/lis3lv02d.c
/dev/freefall + evdev joystick emulation driver, so as to not break
existing userspace consumers.

/dev/freefall is used by the tools/laptop/freefall/freefall.c tool which
is shipped by the kernel as well as by the linux-thinkpad hdapsd daemon.

Using the IIO st_accel driver can be enabled by setting the new
use_iio_driver module parameter.

Link: https://github.com/linux-thinkpad/hdapsd/
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Keep using drivers/misc/lis3lv02d/lis3lv02d.c be default
- Rename the module-parameter to use_iio_driver which can be set to
  use the IIO st_accel driver instead
---
 drivers/platform/x86/dell/dell-smo8800.c | 83 ++++++++++++++++++++++--
 1 file changed, 79 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 3e64ebcf4b21..58bb7275ba5c 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>
@@ -24,6 +25,10 @@
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
 
+static bool use_iio_driver;
+module_param(use_iio_driver, bool, 0444);
+MODULE_PARM_DESC(use_iio_driver, "Use IIO accelerometer driver instead of /dev/freefall chardev + evdev joystick emulation");
+
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
 	atomic_t counter;            /* count after last read */
@@ -145,6 +150,65 @@ static int smo8800_find_i801(struct device *dev, void *data)
 	return 1;
 }
 
+/*
+ * Set label to let iio-sensor-proxy know these freefall sensors are located in
+ * the laptop base (not the display) and are not intended for screen rotation.
+ */
+static const struct property_entry smo8800_accel_props[] = {
+	PROPERTY_ENTRY_STRING("label", "accel-base"),
+	{}
+};
+
+const struct software_node smo8800_accel_node = {
+	.properties = smo8800_accel_props,
+};
+
+static int smo8800_detect_accel(struct smo8800_device *smo8800,
+				struct i2c_adapter *adap, u8 addr,
+				struct i2c_board_info *info)
+{
+	union i2c_smbus_data smbus_data;
+	const char *type;
+	int 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;
+	}
+
+	/*
+	 * These who-am-i register mappings to model strings have been
+	 * taken from the old /dev/freefall chardev and joystick driver:
+	 * 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;
+	}
+
+	strscpy(info->type, type, I2C_NAME_SIZE);
+	info->addr = addr;
+	info->irq = smo8800->irq;
+	info->swnode = &smo8800_accel_node;
+	return 0;
+}
+
 /*
  * Accelerometer's I2C address is not specified in DMI nor ACPI,
  * so it is needed to define mapping table based on DMI product names.
@@ -177,7 +241,7 @@ static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 	struct i2c_adapter *adap = NULL;
 	const char *dmi_product_name;
 	u8 addr = 0;
-	int i;
+	int i, err;
 
 	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
 	if (!adap)
@@ -198,9 +262,20 @@ static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 		goto put_adapter;
 	}
 
-	info.addr = addr;
-	info.irq = smo8800->irq;
-	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+	/* Always detect the accel-type, this also checks the accel is actually there */
+	err = smo8800_detect_accel(smo8800, adap, addr, &info);
+	if (err)
+		goto put_adapter;
+
+	/*
+	 * Unless using the IIO driver is requested, override the detected type
+	 * with "lis3lv02d" so that drivers/misc/lis3lv02d/lis3lv02d.c is used
+	 * and /dev/freefall functionality is provided.
+	 */
+	if (!use_iio_driver) {
+		strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+		info.swnode = NULL;
+	}
 
 	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
 	if (IS_ERR(smo8800->i2c_dev)) {
-- 
2.43.0

Re: [PATCH v2 4/6] platform/x86: dell-smo8800: Allow using the IIO st_accel driver

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build warnings:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/platform-x86-dell-smo8800-Change-probe-ordering-a-bit/20240107-001715
base:   linus/master
patch link:    https://lore.kernel.org/r/20240106160935.45487-5-hdegoede%40redhat.com
patch subject: [PATCH v2 4/6] platform/x86: dell-smo8800: Allow using the IIO st_accel driver
config: i386-randconfig-061-20240113 (https://download.01.org/0day-ci/archive/20240113/202401131735.15ClIkvC-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240113/202401131735.15ClIkvC-lkp@intel.com/reproduce)

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

sparse warnings: (new ones prefixed by >>)
>> drivers/platform/x86/dell/dell-smo8800.c:162:28: sparse: sparse: symbol 'smo8800_accel_node' was not declared. Should it be static?

vim +/smo8800_accel_node +162 drivers/platform/x86/dell/dell-smo8800.c

   161	
 > 162	const struct software_node smo8800_accel_node = {
   163		.properties = smo8800_accel_props,
   164	};
   165	

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

Re: [PATCH v2 4/6] platform/x86: dell-smo8800: Allow using the IIO st_accel driver

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build errors:

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

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/platform-x86-dell-smo8800-Change-probe-ordering-a-bit/20240107-001715
base:   linus/master
patch link:    https://lore.kernel.org/r/20240106160935.45487-5-hdegoede%40redhat.com
patch subject: [PATCH v2 4/6] platform/x86: dell-smo8800: Allow using the IIO st_accel driver
config: i386-buildonly-randconfig-005-20240113 (https://download.01.org/0day-ci/archive/20240113/202401132242.9mmyvnaw-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240113/202401132242.9mmyvnaw-lkp@intel.com/reproduce)

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

All errors (new ones prefixed by >>):

   drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_find_i801':
   drivers/platform/x86/dell/dell-smo8800.c:149:21: error: implicit declaration of function 'i2c_get_adapter'; did you mean 'i2c_get_adapdata'? [-Werror=implicit-function-declaration]
     149 |         *adap_ret = i2c_get_adapter(adap->nr);
         |                     ^~~~~~~~~~~~~~~
         |                     i2c_get_adapdata
   drivers/platform/x86/dell/dell-smo8800.c:149:19: warning: assignment to 'struct i2c_adapter *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     149 |         *adap_ret = i2c_get_adapter(adap->nr);
         |                   ^
   drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_detect_accel':
>> drivers/platform/x86/dell/dell-smo8800.c:174:15: error: implicit declaration of function 'i2c_smbus_xfer' [-Werror=implicit-function-declaration]
     174 |         err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
         |               ^~~~~~~~~~~~~~
   drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_instantiate_i2c_client':
   drivers/platform/x86/dell/dell-smo8800.c:280:28: error: implicit declaration of function 'i2c_new_client_device' [-Werror=implicit-function-declaration]
     280 |         smo8800->i2c_dev = i2c_new_client_device(adap, &info);
         |                            ^~~~~~~~~~~~~~~~~~~~~
   drivers/platform/x86/dell/dell-smo8800.c:280:26: warning: assignment to 'struct i2c_client *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     280 |         smo8800->i2c_dev = i2c_new_client_device(adap, &info);
         |                          ^
   drivers/platform/x86/dell/dell-smo8800.c:290:9: error: implicit declaration of function 'i2c_put_adapter' [-Werror=implicit-function-declaration]
     290 |         i2c_put_adapter(adap);
         |         ^~~~~~~~~~~~~~~
   drivers/platform/x86/dell/dell-smo8800.c: In function 'smo8800_probe':
   drivers/platform/x86/dell/dell-smo8800.c:346:9: error: implicit declaration of function 'i2c_unregister_device'; did you mean 'pci_unregister_driver'? [-Werror=implicit-function-declaration]
     346 |         i2c_unregister_device(smo8800->i2c_dev);
         |         ^~~~~~~~~~~~~~~~~~~~~
         |         pci_unregister_driver
   cc1: some warnings being treated as errors


vim +/i2c_smbus_xfer +174 drivers/platform/x86/dell/dell-smo8800.c

   134	
   135	static int smo8800_find_i801(struct device *dev, void *data)
   136	{
   137		struct i2c_adapter *adap, **adap_ret = data;
   138	
   139		adap = i2c_verify_adapter(dev);
   140		if (!adap)
   141			return 0;
   142	
   143		if (!strstarts(adap->name, "SMBus I801 adapter"))
   144			return 0;
   145	
   146		if (pci_match_id(i801_idf_ids, to_pci_dev(adap->dev.parent)))
   147			return 0; /* Only register client on main SMBus channel */
   148	
 > 149		*adap_ret = i2c_get_adapter(adap->nr);
   150		return 1;
   151	}
   152	
   153	/*
   154	 * Set label to let iio-sensor-proxy know these freefall sensors are located in
   155	 * the laptop base (not the display) and are not intended for screen rotation.
   156	 */
   157	static const struct property_entry smo8800_accel_props[] = {
   158		PROPERTY_ENTRY_STRING("label", "accel-base"),
   159		{}
   160	};
   161	
   162	const struct software_node smo8800_accel_node = {
   163		.properties = smo8800_accel_props,
   164	};
   165	
   166	static int smo8800_detect_accel(struct smo8800_device *smo8800,
   167					struct i2c_adapter *adap, u8 addr,
   168					struct i2c_board_info *info)
   169	{
   170		union i2c_smbus_data smbus_data;
   171		const char *type;
   172		int err;
   173	
 > 174		err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
   175				     I2C_SMBUS_BYTE_DATA, &smbus_data);
   176		if (err < 0) {
   177			dev_warn(smo8800->dev, "Failed to read who-am-i register: %d\n", err);
   178			return err;
   179		}
   180	
   181		/*
   182		 * These who-am-i register mappings to model strings have been
   183		 * taken from the old /dev/freefall chardev and joystick driver:
   184		 * drivers/misc/lis3lv02d/lis3lv02d.c
   185		 */
   186		switch (smbus_data.byte) {
   187		case 0x32:
   188			type = "lis331dlh";
   189			break;
   190		case 0x33:
   191			type = "lis2de12"; /* LIS3DC / HP3DC in drivers/misc/lis3lv02d/lis3lv02d.c */
   192			break;
   193		case 0x3a:
   194			type = "lis3lv02dl_accel";
   195			break;
   196		case 0x3b:
   197			type = "lis302dl";
   198			break;
   199		default:
   200			dev_warn(smo8800->dev, "Unknown who-am-i register value 0x%02x\n",
   201				 smbus_data.byte);
   202			return -ENODEV;
   203		}
   204	
   205		strscpy(info->type, type, I2C_NAME_SIZE);
   206		info->addr = addr;
   207		info->irq = smo8800->irq;
   208		info->swnode = &smo8800_accel_node;
   209		return 0;
   210	}
   211	

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

[PATCH v2 5/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 | 112 +++++++++++++++++++++--
 1 file changed, 105 insertions(+), 7 deletions(-)

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 58bb7275ba5c..021ad31eb84d 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -29,6 +29,10 @@ static bool use_iio_driver;
 module_param(use_iio_driver, bool, 0444);
 MODULE_PARM_DESC(use_iio_driver, "Use IIO accelerometer driver instead of /dev/freefall chardev + evdev joystick emulation");
 
+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 */
@@ -150,6 +154,82 @@ static int smo8800_find_i801(struct device *dev, void *data)
 	return 1;
 }
 
+/*
+ * This is the kernel version of the single register device sanity checks from
+ * the i2c_safety_check function from lm_sensors sensor-detect script:
+ * This is meant to prevent access to 1-register-only devices,
+ * which are designed to be accessed with SMBus receive byte and SMBus send
+ * byte transactions (i.e. short reads and short writes) and treat SMBus
+ * read byte as a real write followed by a read. The device detection
+ * routines would write random values to the chip with possibly very nasty
+ * results for the hardware. Note that this function won't catch all such
+ * chips, as it assumes that reads and writes relate to the same register,
+ * but that's the best we can do.
+ */
+static int i2c_safety_check(struct smo8800_device *smo8800, struct i2c_adapter *adap, u8 addr)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+	u8 data;
+
+	/*
+	 * First receive a byte from the chip, and remember it. This
+	 * also checks if there is a device at the address at all.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	data = smbus_data.byte;
+
+	/*
+	 * Receive a byte again; very likely to be the same for
+	 * 1-register-only devices.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a register offset equal to
+	 * the read byte; for 1-register-only device this should read
+	 * the same byte value in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a slightly different register
+	 * offset; this should again read the register offset in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != (data ^ 0x01))
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Apparently this is a 1-register-only device, restore the original
+	 * register value and leave it alone.
+	 */
+	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
+		       I2C_SMBUS_BYTE, NULL);
+	dev_warn(smo8800->dev, "I2C safety check for address 0x%02x failed, skipping\n", addr);
+	return -ENODEV;
+}
+
 /*
  * Set label to let iio-sensor-proxy know these freefall sensors are located in
  * the laptop base (not the display) and are not intended for screen rotation.
@@ -165,12 +245,19 @@ const struct software_node smo8800_accel_node = {
 
 static int smo8800_detect_accel(struct smo8800_device *smo8800,
 				struct i2c_adapter *adap, u8 addr,
-				struct i2c_board_info *info)
+				struct i2c_board_info *info, bool probe)
 {
 	union i2c_smbus_data smbus_data;
 	const char *type;
 	int err;
 
+	if (probe) {
+		dev_info(smo8800->dev, "Probing for accelerometer on address 0x%02x\n", addr);
+		err = i2c_safety_check(smo8800, adap, addr);
+		if (err < 0)
+			return err;
+	}
+
 	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
 			     I2C_SMBUS_BYTE_DATA, &smbus_data);
 	if (err < 0) {
@@ -256,17 +343,25 @@ static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 		}
 	}
 
-	if (!addr) {
+	if (addr) {
+		/* Always detect the accel-type, this also checks the accel is actually there */
+		err = smo8800_detect_accel(smo8800, adap, addr, &info, false);
+		if (err)
+			goto put_adapter;
+	} else if (probe_i2c_addr) {
+		/* First try address 0x29 (most used) and then try 0x1d */
+		if (smo8800_detect_accel(smo8800, adap, 0x29, &info, true) != 0 &&
+		    smo8800_detect_accel(smo8800, adap, 0x1d, &info, true) != 0) {
+			dev_warn(smo8800->dev, "No accelerometer found\n");
+			goto put_adapter;
+		}
+	} else {
 		dev_warn(smo8800->dev,
 			 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
+		dev_info(smo8800->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
 		goto put_adapter;
 	}
 
-	/* Always detect the accel-type, this also checks the accel is actually there */
-	err = smo8800_detect_accel(smo8800, adap, addr, &info);
-	if (err)
-		goto put_adapter;
-
 	/*
 	 * Unless using the IIO driver is requested, override the detected type
 	 * with "lis3lv02d" so that drivers/misc/lis3lv02d/lis3lv02d.c is used
@@ -285,6 +380,9 @@ static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 	} else {
 		dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n",
 			 info.type, info.addr);
+		if (!addr)
+			dev_info(smo8800->dev,
+				 "Please report this address upstream together with the output of 'cat /sys/class/dmi/id/product_name'\n");
 	}
 put_adapter:
 	i2c_put_adapter(adap);
-- 
2.43.0

[PATCH v2 6/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

Tested-by: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- This is a new patch in v2 of this series
---
 drivers/platform/x86/dell/dell-smo8800.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 021ad31eb84d..49547a077df7 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -318,6 +318,8 @@ static const struct {
 	 * Additional individual entries were added after verification.
 	 */
 	{ "Latitude 5480",      0x29 },
+	{ "Latitude E6330",     0x29 },
+	{ "Latitude E6430",     0x29 },
 	{ "Vostro V131",        0x1d },
 	{ "Vostro 5568",        0x29 },
 };
-- 
2.43.0