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

[PATCH 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 3/6 after some small prep patches
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 3/6? I think you'll like the changes there since they only
remove code :)

Regards,

Hans


Hans de Goede (6):
  platform/x86: dell-smo8800: Only load on Dell laptops
  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: Instantiate an i2c_client for the IIO
    st_accel driver
  platform/x86: dell-smo8800: Add support for probing for the
    accelerometer i2c address

 drivers/i2c/busses/i2c-i801.c            | 122 --------
 drivers/platform/x86/dell/dell-smo8800.c | 337 +++++++++++++++++++++--
 2 files changed, 316 insertions(+), 143 deletions(-)

-- 
2.43.0

[PATCH 1/6] platform/x86: dell-smo8800: Only load on Dell laptops

[Hans de Goede]

The SMO8xxx ACPI HIDs are generic accelerometer ids which are also used
by other vendors. Add a sys_vendor check to ensure that the dell-smo8800
driver only loads on Dell laptops.

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

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index f7ec17c56833..4d5f778fb599 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"
 
+#include <linux/dmi.h>
 #include <linux/fs.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
@@ -108,6 +109,9 @@ static int smo8800_probe(struct platform_device *device)
 	int err;
 	struct smo8800_device *smo8800;
 
+	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
+		return false;
+
 	smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL);
 	if (!smo8800) {
 		dev_err(&device->dev, "failed to allocate device data\n");
-- 
2.43.0

Re: [PATCH 1/6] platform/x86: dell-smo8800: Only load on Dell laptops

[Pali Rohár]

On Sunday 24 December 2023 22:36:17 Hans de Goede wrote:
> The SMO8xxx ACPI HIDs are generic accelerometer ids which are also used
> by other vendors. Add a sys_vendor check to ensure that the dell-smo8800
> driver only loads on Dell laptops.

I saw that this driver was used also on some Acer laptops. As you wrote
above, and now after years I have also feeling that these ACPI HIDs are
generic, not Dell specific.

So I would propose to not restrict smo8800 driver just for Dell laptops
like this patch is introducing.

Maybe better option would be to move this driver out of the dell
namespace.

Are we able to collect dmesg outputs and verify this information on how
many non-dell laptops is this driver loaded?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell/dell-smo8800.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> index f7ec17c56833..4d5f778fb599 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"
>  
> +#include <linux/dmi.h>
>  #include <linux/fs.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
> @@ -108,6 +109,9 @@ static int smo8800_probe(struct platform_device *device)
>  	int err;
>  	struct smo8800_device *smo8800;
>  
> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
> +		return false;
> +
>  	smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL);
>  	if (!smo8800) {
>  		dev_err(&device->dev, "failed to allocate device data\n");
> -- 
> 2.43.0
> 

Re: [PATCH 1/6] platform/x86: dell-smo8800: Only load on Dell laptops

[Hans de Goede]

Hi Pali,

Thank you for your feedback on this series.

On 12/24/23 22:48, Pali Rohár wrote:
> On Sunday 24 December 2023 22:36:17 Hans de Goede wrote:
>> The SMO8xxx ACPI HIDs are generic accelerometer ids which are also used
>> by other vendors. Add a sys_vendor check to ensure that the dell-smo8800
>> driver only loads on Dell laptops.
> 
> I saw that this driver was used also on some Acer laptops. As you wrote
> above, and now after years I have also feeling that these ACPI HIDs are
> generic, not Dell specific.
> 
> So I would propose to not restrict smo8800 driver just for Dell laptops
> like this patch is introducing.

Ok, I was not aware that some Acer laptops were also using this.

I was just trying to honor the vendor check done in
the i2c-i801 driver after the move of the i2c-client instantiation
to dell-smo8800.

Given that we are keeping the existing DMI matching I don't 
think keeping the sys-vendor check is necessary. So I'll
drop the patch from v2 of the series.

Regards,

Hans





> Maybe better option would be to move this driver out of the dell
> namespace.
> 
> Are we able to collect dmesg outputs and verify this information on how
> many non-dell laptops is this driver loaded?
> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/dell/dell-smo8800.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
>> index f7ec17c56833..4d5f778fb599 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"
>>  
>> +#include <linux/dmi.h>
>>  #include <linux/fs.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/kernel.h>
>> @@ -108,6 +109,9 @@ static int smo8800_probe(struct platform_device *device)
>>  	int err;
>>  	struct smo8800_device *smo8800;
>>  
>> +	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
>> +		return false;
>> +
>>  	smo8800 = devm_kzalloc(&device->dev, sizeof(*smo8800), GFP_KERNEL);
>>  	if (!smo8800) {
>>  		dev_err(&device->dev, "failed to allocate device data\n");
>> -- 
>> 2.43.0
>>
> 

[PATCH 2/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 4d5f778fb599..86f898a857e1 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -125,19 +125,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,
@@ -151,6 +149,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 3/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800

[Hans de Goede]

It is not necessary to handle the Dell specific instantiation of
i2c_client-s for 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>
---
 drivers/i2c/busses/i2c-i801.c            | 122 -----------------------
 drivers/platform/x86/dell/dell-smo8800.c | 117 +++++++++++++++++++++-
 2 files changed, 115 insertions(+), 124 deletions(-)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 070999139c6d..552bad99f04d 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1141,125 +1141,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 +1160,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 86f898a857e1..416b399cb9a1 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -4,19 +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>
 
@@ -27,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)
@@ -104,6 +109,108 @@ static const struct file_operations smo8800_misc_fops = {
 	.release = smo8800_misc_release,
 };
 
+static int smo8800_find_i801(struct device *dev, void *data)
+{
+	static const u16 i801_idf_pci_device_ids[] = {
+		0x1d70, /* Patsburg (PCH) */
+		0x1d71, /* Patsburg (PCH) */
+		0x1d72, /* Patsburg (PCH) */
+		0x8d7d, /* Wellsburg (PCH) */
+		0x8d7e, /* Wellsburg (PCH) */
+		0x8d7f, /* Wellsburg (PCH) */
+	};
+	struct i2c_adapter *adap, **adap_ret = data;
+	struct pci_dev *pdev;
+	int i;
+
+	adap = i2c_verify_adapter(dev);
+	if (!adap)
+		return 0;
+
+	if (!strstarts(adap->name, "SMBus I801 adapter"))
+		return 0;
+
+	/* The parent of an I801 adapter is always a PCI device */
+	pdev = to_pci_dev(adap->dev.parent);
+	for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) {
+		if (pdev->device == i801_idf_pci_device_ids[i])
+			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 },
+	{ "Latitude E6330",     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;
@@ -130,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,
@@ -154,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;
 }
 
@@ -164,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 },
@@ -193,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 3/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800

[Pali Rohár]

On Sunday 24 December 2023 22:36:19 Hans de Goede wrote:
> +static int smo8800_find_i801(struct device *dev, void *data)
> +{
> +	static const u16 i801_idf_pci_device_ids[] = {
> +		0x1d70, /* Patsburg (PCH) */
> +		0x1d71, /* Patsburg (PCH) */
> +		0x1d72, /* Patsburg (PCH) */
> +		0x8d7d, /* Wellsburg (PCH) */
> +		0x8d7e, /* Wellsburg (PCH) */
> +		0x8d7f, /* Wellsburg (PCH) */
> +	};

I'm not happy with seeing another hardcoded list of device ids in the
driver. Are not we able to find compatible i801 adapter without need to
hardcode this list there in smo driver? And if really not, what about
sharing (or exporting) list from the i801 driver?

I'm just worried that this PCI id list will increase in the future and
it would be required to add a new and new id into it...

> +	struct i2c_adapter *adap, **adap_ret = data;
> +	struct pci_dev *pdev;
> +	int i;
> +
> +	adap = i2c_verify_adapter(dev);
> +	if (!adap)
> +		return 0;
> +
> +	if (!strstarts(adap->name, "SMBus I801 adapter"))
> +		return 0;
> +
> +	/* The parent of an I801 adapter is always a PCI device */
> +	pdev = to_pci_dev(adap->dev.parent);
> +	for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) {
> +		if (pdev->device == i801_idf_pci_device_ids[i])
> +			return 0; /* Only register client on main SMBus channel */
> +	}
> +
> +	*adap_ret = i2c_get_adapter(adap->nr);
> +	return 1;
> +}

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

[Hans de Goede]

Hi Pali,

On 12/24/23 22:55, Pali Rohár wrote:
> On Sunday 24 December 2023 22:36:19 Hans de Goede wrote:
>> +static int smo8800_find_i801(struct device *dev, void *data)
>> +{
>> +	static const u16 i801_idf_pci_device_ids[] = {
>> +		0x1d70, /* Patsburg (PCH) */
>> +		0x1d71, /* Patsburg (PCH) */
>> +		0x1d72, /* Patsburg (PCH) */
>> +		0x8d7d, /* Wellsburg (PCH) */
>> +		0x8d7e, /* Wellsburg (PCH) */
>> +		0x8d7f, /* Wellsburg (PCH) */
>> +	};
> 
> I'm not happy with seeing another hardcoded list of device ids in the
> driver. Are not we able to find compatible i801 adapter without need to
> hardcode this list there in smo driver?

I agree that having this hardcoded is not ideal.

The problem is that for a couple of generations (Patsburg is for
Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell)
intel had multiple i2c-i801 controllers / I2C-busses in the PCH
and the i2c_client needs to be instantiated on the primary
i2c-i801 (compatible) controller.

Luckily Intel has only done this for these 2 generations PCH
all older and newer PCHs only have 1 i2c-i801 I2C bus.

So even though having this hardcoded is not ideal,
the list should never change since it is only for
this 2 somewhat old PCH generations and new generations
are not impacted. So I believe that this is the best
solution.

Regards,

Hans



	

> And if really not, what about
> sharing (or exporting) list from the i801 driver?
> 
> I'm just worried that this PCI id list will increase in the future and
> it would be required to add a new and new id into it...
> 
>> +	struct i2c_adapter *adap, **adap_ret = data;
>> +	struct pci_dev *pdev;
>> +	int i;
>> +
>> +	adap = i2c_verify_adapter(dev);
>> +	if (!adap)
>> +		return 0;
>> +
>> +	if (!strstarts(adap->name, "SMBus I801 adapter"))
>> +		return 0;
>> +
>> +	/* The parent of an I801 adapter is always a PCI device */
>> +	pdev = to_pci_dev(adap->dev.parent);
>> +	for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) {
>> +		if (pdev->device == i801_idf_pci_device_ids[i])
>> +			return 0; /* Only register client on main SMBus channel */
>> +	}
>> +
>> +	*adap_ret = i2c_get_adapter(adap->nr);
>> +	return 1;
>> +}
> 

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

[Pali Rohár]

On Friday 05 January 2024 17:31:32 Hans de Goede wrote:
> Hi Pali,
> 
> On 12/24/23 22:55, Pali Rohár wrote:
> > On Sunday 24 December 2023 22:36:19 Hans de Goede wrote:
> >> +static int smo8800_find_i801(struct device *dev, void *data)
> >> +{
> >> +	static const u16 i801_idf_pci_device_ids[] = {
> >> +		0x1d70, /* Patsburg (PCH) */
> >> +		0x1d71, /* Patsburg (PCH) */
> >> +		0x1d72, /* Patsburg (PCH) */
> >> +		0x8d7d, /* Wellsburg (PCH) */
> >> +		0x8d7e, /* Wellsburg (PCH) */
> >> +		0x8d7f, /* Wellsburg (PCH) */
> >> +	};
> > 
> > I'm not happy with seeing another hardcoded list of device ids in the
> > driver. Are not we able to find compatible i801 adapter without need to
> > hardcode this list there in smo driver?
> 
> I agree that having this hardcoded is not ideal.
> 
> The problem is that for a couple of generations (Patsburg is for
> Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell)
> intel had multiple i2c-i801 controllers / I2C-busses in the PCH
> and the i2c_client needs to be instantiated on the primary
> i2c-i801 (compatible) controller.
> 
> Luckily Intel has only done this for these 2 generations PCH
> all older and newer PCHs only have 1 i2c-i801 I2C bus.
> 
> So even though having this hardcoded is not ideal,
> the list should never change since it is only for
> this 2 somewhat old PCH generations and new generations
> are not impacted. So I believe that this is the best
> solution.

I see. Seems that this is the best solution which we have.

Anyway, is not possible to use pci_dev_driver() to find i801 driver and
from it takes that list of devices which have FEATURE_IDF flag? I have
looked at the code only quickly and maybe it could be possible. Just an
idea.

> Regards,
> 
> Hans
> 
> 
> 
> 	
> 
> > And if really not, what about
> > sharing (or exporting) list from the i801 driver?
> > 
> > I'm just worried that this PCI id list will increase in the future and
> > it would be required to add a new and new id into it...
> > 
> >> +	struct i2c_adapter *adap, **adap_ret = data;
> >> +	struct pci_dev *pdev;
> >> +	int i;
> >> +
> >> +	adap = i2c_verify_adapter(dev);
> >> +	if (!adap)
> >> +		return 0;
> >> +
> >> +	if (!strstarts(adap->name, "SMBus I801 adapter"))
> >> +		return 0;
> >> +
> >> +	/* The parent of an I801 adapter is always a PCI device */
> >> +	pdev = to_pci_dev(adap->dev.parent);
> >> +	for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) {
> >> +		if (pdev->device == i801_idf_pci_device_ids[i])
> >> +			return 0; /* Only register client on main SMBus channel */
> >> +	}
> >> +
> >> +	*adap_ret = i2c_get_adapter(adap->nr);
> >> +	return 1;
> >> +}
> > 
> 

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

[Hans de Goede]

Hi,

On 1/5/24 19:26, Pali Rohár wrote:
> On Friday 05 January 2024 17:31:32 Hans de Goede wrote:
>> Hi Pali,
>>
>> On 12/24/23 22:55, Pali Rohár wrote:
>>> On Sunday 24 December 2023 22:36:19 Hans de Goede wrote:
>>>> +static int smo8800_find_i801(struct device *dev, void *data)
>>>> +{
>>>> +	static const u16 i801_idf_pci_device_ids[] = {
>>>> +		0x1d70, /* Patsburg (PCH) */
>>>> +		0x1d71, /* Patsburg (PCH) */
>>>> +		0x1d72, /* Patsburg (PCH) */
>>>> +		0x8d7d, /* Wellsburg (PCH) */
>>>> +		0x8d7e, /* Wellsburg (PCH) */
>>>> +		0x8d7f, /* Wellsburg (PCH) */
>>>> +	};
>>>
>>> I'm not happy with seeing another hardcoded list of device ids in the
>>> driver. Are not we able to find compatible i801 adapter without need to
>>> hardcode this list there in smo driver?
>>
>> I agree that having this hardcoded is not ideal.
>>
>> The problem is that for a couple of generations (Patsburg is for
>> Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell)
>> intel had multiple i2c-i801 controllers / I2C-busses in the PCH
>> and the i2c_client needs to be instantiated on the primary
>> i2c-i801 (compatible) controller.
>>
>> Luckily Intel has only done this for these 2 generations PCH
>> all older and newer PCHs only have 1 i2c-i801 I2C bus.
>>
>> So even though having this hardcoded is not ideal,
>> the list should never change since it is only for
>> this 2 somewhat old PCH generations and new generations
>> are not impacted. So I believe that this is the best
>> solution.
> 
> I see. Seems that this is the best solution which we have.
> 
> Anyway, is not possible to use pci_dev_driver() to find i801 driver and
> from it takes that list of devices which have FEATURE_IDF flag? I have
> looked at the code only quickly and maybe it could be possible. Just an
> idea.

That is an interesting idea, but ...

that would mean interpreting the driver_data set by the i2c-i801
driver inside the dell-smo8800 code, so this would basically rely on
the meaning of that driver_data never changing. I would rather just
duplicate the 6 PCI ids and decouple the 2 drivers.

Regards,

Hans




>>>> +	struct i2c_adapter *adap, **adap_ret = data;
>>>> +	struct pci_dev *pdev;
>>>> +	int i;
>>>> +
>>>> +	adap = i2c_verify_adapter(dev);
>>>> +	if (!adap)
>>>> +		return 0;
>>>> +
>>>> +	if (!strstarts(adap->name, "SMBus I801 adapter"))
>>>> +		return 0;
>>>> +
>>>> +	/* The parent of an I801 adapter is always a PCI device */
>>>> +	pdev = to_pci_dev(adap->dev.parent);
>>>> +	for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) {
>>>> +		if (pdev->device == i801_idf_pci_device_ids[i])
>>>> +			return 0; /* Only register client on main SMBus channel */
>>>> +	}
>>>> +
>>>> +	*adap_ret = i2c_get_adapter(adap->nr);
>>>> +	return 1;
>>>> +}
>>>
>>
> 

Re: [PATCH 3/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 13:13:01 Hans de Goede wrote:
> Hi,
> 
> On 1/5/24 19:26, Pali Rohár wrote:
> > On Friday 05 January 2024 17:31:32 Hans de Goede wrote:
> >> Hi Pali,
> >>
> >> On 12/24/23 22:55, Pali Rohár wrote:
> >>> On Sunday 24 December 2023 22:36:19 Hans de Goede wrote:
> >>>> +static int smo8800_find_i801(struct device *dev, void *data)
> >>>> +{
> >>>> +	static const u16 i801_idf_pci_device_ids[] = {
> >>>> +		0x1d70, /* Patsburg (PCH) */
> >>>> +		0x1d71, /* Patsburg (PCH) */
> >>>> +		0x1d72, /* Patsburg (PCH) */
> >>>> +		0x8d7d, /* Wellsburg (PCH) */
> >>>> +		0x8d7e, /* Wellsburg (PCH) */
> >>>> +		0x8d7f, /* Wellsburg (PCH) */
> >>>> +	};
> >>>
> >>> I'm not happy with seeing another hardcoded list of device ids in the
> >>> driver. Are not we able to find compatible i801 adapter without need to
> >>> hardcode this list there in smo driver?
> >>
> >> I agree that having this hardcoded is not ideal.
> >>
> >> The problem is that for a couple of generations (Patsburg is for
> >> Sandy Bridge and Ivybridge and Wellsburg is for Haswell and Broadwell)
> >> intel had multiple i2c-i801 controllers / I2C-busses in the PCH
> >> and the i2c_client needs to be instantiated on the primary
> >> i2c-i801 (compatible) controller.
> >>
> >> Luckily Intel has only done this for these 2 generations PCH
> >> all older and newer PCHs only have 1 i2c-i801 I2C bus.
> >>
> >> So even though having this hardcoded is not ideal,
> >> the list should never change since it is only for
> >> this 2 somewhat old PCH generations and new generations
> >> are not impacted. So I believe that this is the best
> >> solution.
> > 
> > I see. Seems that this is the best solution which we have.
> > 
> > Anyway, is not possible to use pci_dev_driver() to find i801 driver and
> > from it takes that list of devices which have FEATURE_IDF flag? I have
> > looked at the code only quickly and maybe it could be possible. Just an
> > idea.
> 
> That is an interesting idea, but ...
> 
> that would mean interpreting the driver_data set by the i2c-i801
> driver inside the dell-smo8800 code, so this would basically rely on
> the meaning of that driver_data never changing. I would rather just
> duplicate the 6 PCI ids and decouple the 2 drivers.

It was just an alternative idea.

In case of code duplication I would suggest to write a comment on both
places (into i801 and smo drivers) that this information is duplicated
in both drivers and should be synchronized in case somebody would need
to modify it at either place.

> Regards,
> 
> Hans
> 
> 
> 
> 
> >>>> +	struct i2c_adapter *adap, **adap_ret = data;
> >>>> +	struct pci_dev *pdev;
> >>>> +	int i;
> >>>> +
> >>>> +	adap = i2c_verify_adapter(dev);
> >>>> +	if (!adap)
> >>>> +		return 0;
> >>>> +
> >>>> +	if (!strstarts(adap->name, "SMBus I801 adapter"))
> >>>> +		return 0;
> >>>> +
> >>>> +	/* The parent of an I801 adapter is always a PCI device */
> >>>> +	pdev = to_pci_dev(adap->dev.parent);
> >>>> +	for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) {
> >>>> +		if (pdev->device == i801_idf_pci_device_ids[i])
> >>>> +			return 0; /* Only register client on main SMBus channel */
> >>>> +	}
> >>>> +
> >>>> +	*adap_ret = i2c_get_adapter(adap->nr);
> >>>> +	return 1;
> >>>> +}
> >>>
> >>
> > 
> 

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

[Andy Shevchenko]

On Sun, Dec 24, 2023 at 11:36 PM Hans de Goede <hdegoede@redhat.com> 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.

...

> +       static const u16 i801_idf_pci_device_ids[] = {
> +               0x1d70, /* Patsburg (PCH) */
> +               0x1d71, /* Patsburg (PCH) */
> +               0x1d72, /* Patsburg (PCH) */
> +               0x8d7d, /* Wellsburg (PCH) */
> +               0x8d7e, /* Wellsburg (PCH) */
> +               0x8d7f, /* Wellsburg (PCH) */
> +       };

I prefer to see this as a PCI ID table (yes, I know the downsides).

...

> +/* Ensure the i2c-801 driver is loaded for i2c_client instantiation */
> +MODULE_SOFTDEP("pre: i2c-i801");

JFYI: software module dependencies are not supported by all kmod
implementations in the user space. I don't expect people to complain,
but just let you know that this kind of change needs to be done with
care.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/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-20240108]
[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-Only-load-on-Dell-laptops/20231225-152720
base:   linus/master
patch link:    https://lore.kernel.org/r/20231224213629.395741-4-hdegoede%40redhat.com
patch subject: [PATCH 3/6] platform/x86: dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800
config: i386-randconfig-003-20240106 (https://download.01.org/0day-ci/archive/20240108/202401081755.B1WHdRuF-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240108/202401081755.B1WHdRuF-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/202401081755.B1WHdRuF-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_remove':
>> drivers/platform/x86/dell/dell-smo8800.c:278: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_instantiate_i2c_client':
>> drivers/platform/x86/dell/dell-smo8800.c:179: undefined reference to `i2c_bus_type'
>> ld: drivers/platform/x86/dell/dell-smo8800.c:211: undefined reference to `i2c_put_adapter'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_probe':
   drivers/platform/x86/dell/dell-smo8800.c:267: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_instantiate_i2c_client':
   drivers/platform/x86/dell/dell-smo8800.c:201: undefined reference to `i2c_new_client_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_find_i801':
>> drivers/platform/x86/dell/dell-smo8800.c:126: undefined reference to `i2c_verify_adapter'
>> ld: drivers/platform/x86/dell/dell-smo8800.c:140: undefined reference to `i2c_get_adapter'


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

   111	
   112	static int smo8800_find_i801(struct device *dev, void *data)
   113	{
   114		static const u16 i801_idf_pci_device_ids[] = {
   115			0x1d70, /* Patsburg (PCH) */
   116			0x1d71, /* Patsburg (PCH) */
   117			0x1d72, /* Patsburg (PCH) */
   118			0x8d7d, /* Wellsburg (PCH) */
   119			0x8d7e, /* Wellsburg (PCH) */
   120			0x8d7f, /* Wellsburg (PCH) */
   121		};
   122		struct i2c_adapter *adap, **adap_ret = data;
   123		struct pci_dev *pdev;
   124		int i;
   125	
 > 126		adap = i2c_verify_adapter(dev);
   127		if (!adap)
   128			return 0;
   129	
   130		if (!strstarts(adap->name, "SMBus I801 adapter"))
   131			return 0;
   132	
   133		/* The parent of an I801 adapter is always a PCI device */
   134		pdev = to_pci_dev(adap->dev.parent);
   135		for (i = 0; i < ARRAY_SIZE(i801_idf_pci_device_ids); i++) {
   136			if (pdev->device == i801_idf_pci_device_ids[i])
   137				return 0; /* Only register client on main SMBus channel */
   138		}
   139	
 > 140		*adap_ret = i2c_get_adapter(adap->nr);
   141		return 1;
   142	}
   143	
   144	/*
   145	 * Accelerometer's I2C address is not specified in DMI nor ACPI,
   146	 * so it is needed to define mapping table based on DMI product names.
   147	 */
   148	static const struct {
   149		const char *dmi_product_name;
   150		unsigned short i2c_addr;
   151	} dell_lis3lv02d_devices[] = {
   152		/*
   153		 * Dell platform team told us that these Latitude devices have
   154		 * ST microelectronics accelerometer at I2C address 0x29.
   155		 */
   156		{ "Latitude E5250",     0x29 },
   157		{ "Latitude E5450",     0x29 },
   158		{ "Latitude E5550",     0x29 },
   159		{ "Latitude E6440",     0x29 },
   160		{ "Latitude E6440 ATG", 0x29 },
   161		{ "Latitude E6540",     0x29 },
   162		/*
   163		 * Additional individual entries were added after verification.
   164		 */
   165		{ "Latitude 5480",      0x29 },
   166		{ "Latitude E6330",     0x29 },
   167		{ "Vostro V131",        0x1d },
   168		{ "Vostro 5568",        0x29 },
   169	};
   170	
   171	static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
   172	{
   173		struct i2c_board_info info = { };
   174		struct i2c_adapter *adap = NULL;
   175		const char *dmi_product_name;
   176		u8 addr = 0;
   177		int i;
   178	
 > 179		bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
   180		if (!adap)
   181			return;
   182	
   183		dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
   184		for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
   185			if (strcmp(dmi_product_name,
   186				   dell_lis3lv02d_devices[i].dmi_product_name) == 0) {
   187				addr = dell_lis3lv02d_devices[i].i2c_addr;
   188				break;
   189			}
   190		}
   191	
   192		if (!addr) {
   193			dev_warn(smo8800->dev,
   194				 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
   195			goto put_adapter;
   196		}
   197	
   198		info.addr = addr;
   199		strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
   200	
   201		smo8800->i2c_dev = i2c_new_client_device(adap, &info);
   202		if (IS_ERR(smo8800->i2c_dev)) {
   203			dev_err_probe(smo8800->dev, PTR_ERR(smo8800->i2c_dev),
   204				      "registering accel i2c_client\n");
   205			smo8800->i2c_dev = NULL;
   206		} else {
   207			dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n",
   208				 info.type, info.addr);
   209		}
   210	put_adapter:
 > 211		i2c_put_adapter(adap);
   212	}
   213	
   214	static int smo8800_probe(struct platform_device *device)
   215	{
   216		int err;
   217		struct smo8800_device *smo8800;
   218	
   219		if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
   220			return false;
   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	
   271	static void smo8800_remove(struct platform_device *device)
   272	{
   273		struct smo8800_device *smo8800 = platform_get_drvdata(device);
   274	
   275		free_irq(smo8800->irq, smo8800);
   276		misc_deregister(&smo8800->miscdev);
   277		dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
 > 278		i2c_unregister_device(smo8800->i2c_dev);
   279	}
   280	

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

[PATCH 4/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 416b399cb9a1..7f7c9452a983 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -196,6 +196,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

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

[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-20240108]
[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-Only-load-on-Dell-laptops/20231225-152720
base:   linus/master
patch link:    https://lore.kernel.org/r/20231224213629.395741-5-hdegoede%40redhat.com
patch subject: [PATCH 4/6] platform/x86: dell-smo8800: Pass the IRQ to the lis3lv02d i2c_client
config: i386-randconfig-003-20240106 (https://download.01.org/0day-ci/archive/20240109/202401090124.XEXEJx2K-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240109/202401090124.XEXEJx2K-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/202401090124.XEXEJx2K-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_remove':
   drivers/platform/x86/dell/dell-smo8800.c:284: undefined reference to `i2c_unregister_device'
>> ld: drivers/platform/x86/dell/dell-smo8800.c:284: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_instantiate_i2c_client':
   drivers/platform/x86/dell/dell-smo8800.c:179: undefined reference to `i2c_bus_type'
   ld: drivers/platform/x86/dell/dell-smo8800.c:212: undefined reference to `i2c_put_adapter'
>> ld: drivers/platform/x86/dell/dell-smo8800.c:202: undefined reference to `i2c_new_client_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_probe':
   drivers/platform/x86/dell/dell-smo8800.c:271: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_find_i801':
   drivers/platform/x86/dell/dell-smo8800.c:126: undefined reference to `i2c_verify_adapter'
   ld: drivers/platform/x86/dell/dell-smo8800.c:140: undefined reference to `i2c_get_adapter'

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

[PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

[Hans de Goede]

Instead of instantiating an i2c_client for the old misc joystick emulation
and freefall driver: drivers/misc/lis3lv02d/lis3lv02d.c use
i2c_client_id-s from the IIO st_accel driver so that the accelerometer
gets presented to userspace as an IIO device like all modern accelerometer
drivers do.

Add a new use_misc_lis3lv02d module-parameter which can be set to restore
the old behavior in case someone has a use-case depending on this.

When the st_accel IIO driver is used, also pass the IRQ to the i2c_client
and disable the /dev/freefall chardev.

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

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 7f7c9452a983..bb1d3e439761 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_misc_lis3lv02d;
+module_param(use_misc_lis3lv02d, bool, 0444);
+MODULE_PARM_DESC(use_misc_lis3lv02d, "Use /dev/freefall chardev + evdev joystick emulation instead of iio accel driver");
+
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
 	atomic_t counter;            /* count after last read */
@@ -141,6 +146,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.
@@ -174,7 +238,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)
@@ -195,9 +259,19 @@ 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;
+
+	/*
+	 * If requested override detected type with "lis3lv02d" i2c_client_id,
+	 * for the old drivers/misc/lis3lv02d/lis3lv02d.c driver.
+	 */
+	if (use_misc_lis3lv02d) {
+		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 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

[Pali Rohár]

On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
> Instead of instantiating an i2c_client for the old misc joystick emulation
> and freefall driver: drivers/misc/lis3lv02d/lis3lv02d.c use
> i2c_client_id-s from the IIO st_accel driver so that the accelerometer
> gets presented to userspace as an IIO device like all modern accelerometer
> drivers do.
> 
> Add a new use_misc_lis3lv02d module-parameter which can be set to restore
> the old behavior in case someone has a use-case depending on this.
> 
> When the st_accel IIO driver is used, also pass the IRQ to the i2c_client
> and disable the /dev/freefall chardev.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell/dell-smo8800.c | 82 ++++++++++++++++++++++--
>  1 file changed, 78 insertions(+), 4 deletions(-)

Sorry for the stupid question there, but what is the replacement for the
/dev/freefall when using new st_accel IIO driver?

Re: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

[Hans de Goede]

Hi,

On 12/24/23 23:03, Pali Rohár wrote:
> On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
>> Instead of instantiating an i2c_client for the old misc joystick emulation
>> and freefall driver: drivers/misc/lis3lv02d/lis3lv02d.c use
>> i2c_client_id-s from the IIO st_accel driver so that the accelerometer
>> gets presented to userspace as an IIO device like all modern accelerometer
>> drivers do.
>>
>> Add a new use_misc_lis3lv02d module-parameter which can be set to restore
>> the old behavior in case someone has a use-case depending on this.
>>
>> When the st_accel IIO driver is used, also pass the IRQ to the i2c_client
>> and disable the /dev/freefall chardev.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/platform/x86/dell/dell-smo8800.c | 82 ++++++++++++++++++++++--
>>  1 file changed, 78 insertions(+), 4 deletions(-)
> 
> Sorry for the stupid question there, but what is the replacement for the
> /dev/freefall when using new st_accel IIO driver?

There is no replacement for /dev/freefall. I realize this is not ideal
and if this turns out to be a problem the default of the module option
can be reverted.

But AFAIK / AFAICT there are no actual userspace consumers of
/dev/freefall so removing it should not be an issue.

Specifically I checked smartmontools which ships smartd which
is the only daemon which I know of for hdd monitoring and
that does not have /dev/freefall support. So /dev/freefall
appears to be unused to me ?

For completeness I also checked libatasmart which also does
not access /dev/freefall.

Regards,

Hans

Re: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

[Pali Rohár]

On Friday 05 January 2024 17:34:07 Hans de Goede wrote:
> Hi,
> 
> On 12/24/23 23:03, Pali Rohár wrote:
> > On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
> >> Instead of instantiating an i2c_client for the old misc joystick emulation
> >> and freefall driver: drivers/misc/lis3lv02d/lis3lv02d.c use
> >> i2c_client_id-s from the IIO st_accel driver so that the accelerometer
> >> gets presented to userspace as an IIO device like all modern accelerometer
> >> drivers do.
> >>
> >> Add a new use_misc_lis3lv02d module-parameter which can be set to restore
> >> the old behavior in case someone has a use-case depending on this.
> >>
> >> When the st_accel IIO driver is used, also pass the IRQ to the i2c_client
> >> and disable the /dev/freefall chardev.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >>  drivers/platform/x86/dell/dell-smo8800.c | 82 ++++++++++++++++++++++--
> >>  1 file changed, 78 insertions(+), 4 deletions(-)
> > 
> > Sorry for the stupid question there, but what is the replacement for the
> > /dev/freefall when using new st_accel IIO driver?
> 
> There is no replacement for /dev/freefall.

That is a big problem if there is no replacement. The primary usage of
that hardware and all these drivers is that freefall ability. It was
originally written for HP laptops and later extended for Dell. Access to
accelerometer axes was just a secondary functions for linux fans.

> I realize this is not ideal
> and if this turns out to be a problem the default of the module option
> can be reverted.
> 
> But AFAIK / AFAICT there are no actual userspace consumers of
> /dev/freefall so removing it should not be an issue.

Userspace tool is directly in the kernel tree. Somewhere in tools dir
now as it was moved (if it was not removed).

> Specifically I checked smartmontools which ships smartd which
> is the only daemon which I know of for hdd monitoring and
> that does not have /dev/freefall support. So /dev/freefall
> appears to be unused to me ?
> 
> For completeness I also checked libatasmart which also does
> not access /dev/freefall.

I guess nobody ported it to these tools. IIRC the freefall design comes
from the Suse the tool was also used on preloaded HP laptops. So maybe
Suse have custom tool? I do not know.

> Regards,
> 
> Hans
> 
> 

Re: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

[Andy Shevchenko]

On Fri, Jan 5, 2024 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 12/24/23 23:03, Pali Rohár wrote:
> > On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:

...

> But AFAIK / AFAICT there are no actual userspace consumers of
> /dev/freefall so removing it should not be an issue.

IIRC/AFAIK there is at least one (simple) computer game using it as a joystick.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

[Andy Shevchenko]

On Fri, Jan 5, 2024 at 8:37 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 5, 2024 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 12/24/23 23:03, Pali Rohár wrote:
> > > On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:

...

> > But AFAIK / AFAICT there are no actual userspace consumers of
> > /dev/freefall so removing it should not be an issue.
>
> IIRC/AFAIK there is at least one (simple) computer game using it as a joystick.

Okay, I can't google for it and now I realised that it was my x60s,
which has no freefall, but another interface to it. In any case the
side effect of that googling is this (maybe more, I just took this one
as example):
https://github.com/linux-thinkpad/hdapsd/blob/master/README.md

So, dropping it will break at least this tool.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

[Pali Rohár]

On Friday 05 January 2024 21:04:59 Andy Shevchenko wrote:
> On Fri, Jan 5, 2024 at 8:37 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 5, 2024 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > > On 12/24/23 23:03, Pali Rohár wrote:
> > > > On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
> 
> ...
> 
> > > But AFAIK / AFAICT there are no actual userspace consumers of
> > > /dev/freefall so removing it should not be an issue.
> >
> > IIRC/AFAIK there is at least one (simple) computer game using it as a joystick.
> 
> Okay, I can't google for it and now I realised that it was my x60s,
> which has no freefall, but another interface to it. In any case the
> side effect of that googling is this (maybe more, I just took this one
> as example):
> https://github.com/linux-thinkpad/hdapsd/blob/master/README.md
> 
> So, dropping it will break at least this tool.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Yes, this is that correct one. I forget the name of this daemon.

Just to note /dev/freefall does not provide axes state, it just send
signal to process when interrupt is triggered. Process than park disk
heads.

Axes state are/were exported throw /dev/js* interface and those games
uses just js interface. I remember Tux Racer.

Interrupt on HP and Dell is triggered only when laptop fall is detected,
so games did not used it (hopefully!)

Re: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver

[Hans de Goede]

Hi,

On 1/5/24 20:20, Pali Rohár wrote:
> On Friday 05 January 2024 21:04:59 Andy Shevchenko wrote:
>> On Fri, Jan 5, 2024 at 8:37 PM Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>> On Fri, Jan 5, 2024 at 6:34 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 12/24/23 23:03, Pali Rohár wrote:
>>>>> On Sunday 24 December 2023 22:36:21 Hans de Goede wrote:
>>
>> ...
>>
>>>> But AFAIK / AFAICT there are no actual userspace consumers of
>>>> /dev/freefall so removing it should not be an issue.
>>>
>>> IIRC/AFAIK there is at least one (simple) computer game using it as a joystick.
>>
>> Okay, I can't google for it and now I realised that it was my x60s,
>> which has no freefall, but another interface to it. In any case the
>> side effect of that googling is this (maybe more, I just took this one
>> as example):
>> https://github.com/linux-thinkpad/hdapsd/blob/master/README.md
>>
>> So, dropping it will break at least this tool.
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko
> 
> Yes, this is that correct one. I forget the name of this daemon.
> 
> Just to note /dev/freefall does not provide axes state, it just send
> signal to process when interrupt is triggered. Process than park disk
> heads.
> 
> Axes state are/were exported throw /dev/js* interface and those games
> uses just js interface. I remember Tux Racer.
> 
> Interrupt on HP and Dell is triggered only when laptop fall is detected,
> so games did not used it (hopefully!)

Ok, so I clearly need to change the module parameter so that we stick with
the drivers/char/misc/lis3lv02d driver as default and offer using
the i2c-client-id which results in loading the iio driver instead as
an option enabled by a module parameter.

I'll fix this for v2.

Regards,

Hans

Re: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for 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-20240108]
[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-Only-load-on-Dell-laptops/20231225-152720
base:   linus/master
patch link:    https://lore.kernel.org/r/20231224213629.395741-6-hdegoede%40redhat.com
patch subject: [PATCH 5/6] platform/x86: dell-smo8800: Instantiate an i2c_client for the IIO st_accel driver
config: i386-randconfig-003-20240106 (https://download.01.org/0day-ci/archive/20240109/202401090941.FHkrtPXf-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240109/202401090941.FHkrtPXf-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/202401090941.FHkrtPXf-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_remove':
   drivers/platform/x86/dell/dell-smo8800.c:358: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.c:358: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_instantiate_i2c_client':
   drivers/platform/x86/dell/dell-smo8800.c:243: undefined reference to `i2c_bus_type'
   ld: drivers/platform/x86/dell/dell-smo8800.c:286: undefined reference to `i2c_put_adapter'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_detect_accel':
>> drivers/platform/x86/dell/dell-smo8800.c:170: undefined reference to `i2c_smbus_xfer'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_instantiate_i2c_client':
   drivers/platform/x86/dell/dell-smo8800.c:276: undefined reference to `i2c_new_client_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_probe':
   drivers/platform/x86/dell/dell-smo8800.c:345: undefined reference to `i2c_unregister_device'
   ld: drivers/platform/x86/dell/dell-smo8800.o: in function `smo8800_find_i801':
   drivers/platform/x86/dell/dell-smo8800.c:131: undefined reference to `i2c_verify_adapter'
   ld: drivers/platform/x86/dell/dell-smo8800.c:145: undefined reference to `i2c_get_adapter'


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

   161	
   162	static int smo8800_detect_accel(struct smo8800_device *smo8800,
   163					struct i2c_adapter *adap, u8 addr,
   164					struct i2c_board_info *info)
   165	{
   166		union i2c_smbus_data smbus_data;
   167		const char *type;
   168		int err;
   169	
 > 170		err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
   171				     I2C_SMBUS_BYTE_DATA, &smbus_data);
   172		if (err < 0) {
   173			dev_warn(smo8800->dev, "Failed to read who-am-i register: %d\n", err);
   174			return err;
   175		}
   176	
   177		/*
   178		 * These who-am-i register mappings to model strings have been
   179		 * taken from the old /dev/freefall chardev and joystick driver:
   180		 * drivers/misc/lis3lv02d/lis3lv02d.c
   181		 */
   182		switch (smbus_data.byte) {
   183		case 0x32:
   184			type = "lis331dlh";
   185			break;
   186		case 0x33:
   187			type = "lis2de12"; /* LIS3DC / HP3DC in drivers/misc/lis3lv02d/lis3lv02d.c */
   188			break;
   189		case 0x3a:
   190			type = "lis3lv02dl_accel";
   191			break;
   192		case 0x3b:
   193			type = "lis302dl";
   194			break;
   195		default:
   196			dev_warn(smo8800->dev, "Unknown who-am-i register value 0x%02x\n",
   197				 smbus_data.byte);
   198			return -ENODEV;
   199		}
   200	
   201		strscpy(info->type, type, I2C_NAME_SIZE);
   202		info->addr = addr;
   203		info->irq = smo8800->irq;
   204		info->swnode = &smo8800_accel_node;
   205		return 0;
   206	}
   207	

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

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

[Hans de Goede]

Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
of the accelerometer. So a DMI product-name to address mapping table
is used.

At support to have the kernel probe for the i2c-address for modesl
which are not on the list.

The new probing code sits behind a new probe_i2c_addr module parameter,
which is disabled by default because probing might be dangerous.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell/dell-smo8800.c | 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 bb1d3e439761..60ce2695ce01 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -29,6 +29,10 @@ static bool use_misc_lis3lv02d;
 module_param(use_misc_lis3lv02d, bool, 0444);
 MODULE_PARM_DESC(use_misc_lis3lv02d, "Use /dev/freefall chardev + evdev joystick emulation instead of iio accel driver");
 
+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 */
@@ -146,6 +150,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.
@@ -161,12 +241,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) {
@@ -253,17 +340,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;
-
 	/*
 	 * If requested override detected type with "lis3lv02d" i2c_client_id,
 	 * for the old drivers/misc/lis3lv02d/lis3lv02d.c driver.
@@ -281,6 +376,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

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

[Pali Rohár]

On Sunday 24 December 2023 22:36:22 Hans de Goede wrote:
> Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
> of the accelerometer. So a DMI product-name to address mapping table
> is used.
> 
> At support to have the kernel probe for the i2c-address for modesl
> which are not on the list.
> 
> The new probing code sits behind a new probe_i2c_addr module parameter,
> which is disabled by default because probing might be dangerous.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

I would really like to hear Dell opinion about this change, and if there
is really no way to get i2c address. Could you ask Dell people about it?
Always it is better to use official / vendor provided steps of hardware
detection, instead of inventing something new / own which would be there
for a long time...

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

[Hans de Goede]

Hi Pali,

On 12/24/23 23:07, Pali Rohár wrote:
> On Sunday 24 December 2023 22:36:22 Hans de Goede wrote:
>> Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
>> of the accelerometer. So a DMI product-name to address mapping table
>> is used.
>>
>> At support to have the kernel probe for the i2c-address for modesl
>> which are not on the list.
>>
>> The new probing code sits behind a new probe_i2c_addr module parameter,
>> which is disabled by default because probing might be dangerous.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> I would really like to hear Dell opinion about this change, and if there
> is really no way to get i2c address. Could you ask Dell people about it?
> Always it is better to use official / vendor provided steps of hardware
> detection, instead of inventing something new / own which would be there
> for a long time...

Unfortunately I no longer have any contacts inside Dell for
this and given Dell's non response in the original thread
which started this I'm not hopefull for help from Dell here.

Also there original reaction indicated that the info is not
available in ACPI, so probing + extending the DMI match
list seems to be the only way.

Regards,

Hans

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

[Pali Rohár]

On Friday 05 January 2024 17:36:03 Hans de Goede wrote:
> Hi Pali,
> 
> On 12/24/23 23:07, Pali Rohár wrote:
> > On Sunday 24 December 2023 22:36:22 Hans de Goede wrote:
> >> Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
> >> of the accelerometer. So a DMI product-name to address mapping table
> >> is used.
> >>
> >> At support to have the kernel probe for the i2c-address for modesl
> >> which are not on the list.
> >>
> >> The new probing code sits behind a new probe_i2c_addr module parameter,
> >> which is disabled by default because probing might be dangerous.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > 
> > I would really like to hear Dell opinion about this change, and if there
> > is really no way to get i2c address. Could you ask Dell people about it?
> > Always it is better to use official / vendor provided steps of hardware
> > detection, instead of inventing something new / own which would be there
> > for a long time...
> 
> Unfortunately I no longer have any contacts inside Dell for
> this and given Dell's non response in the original thread
> which started this I'm not hopefull for help from Dell here.

Well, writing an email to hundred of receivers, or writing 10 or more
emails at the same time is nowadays an example how to get your email
into spam box in lot of companies.

> Also there original reaction indicated that the info is not
> available in ACPI, so probing + extending the DMI match
> list seems to be the only way.

I have verified this statement years ago and therefore it applies only
for old models (about 10 years old). So using this statement is not
valid for new models anymore.

> Regards,
> 
> Hans
> 
> 

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

[Hans de Goede]

Hi,

On 1/5/24 19:51, Pali Rohár wrote:
> On Friday 05 January 2024 17:36:03 Hans de Goede wrote:
>> Hi Pali,
>>
>> On 12/24/23 23:07, Pali Rohár wrote:
>>> On Sunday 24 December 2023 22:36:22 Hans de Goede wrote:
>>>> Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
>>>> of the accelerometer. So a DMI product-name to address mapping table
>>>> is used.
>>>>
>>>> At support to have the kernel probe for the i2c-address for modesl
>>>> which are not on the list.
>>>>
>>>> The new probing code sits behind a new probe_i2c_addr module parameter,
>>>> which is disabled by default because probing might be dangerous.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>
>>> I would really like to hear Dell opinion about this change, and if there
>>> is really no way to get i2c address. Could you ask Dell people about it?
>>> Always it is better to use official / vendor provided steps of hardware
>>> detection, instead of inventing something new / own which would be there
>>> for a long time...
>>
>> Unfortunately I no longer have any contacts inside Dell for
>> this and given Dell's non response in the original thread
>> which started this I'm not hopefull for help from Dell here.
> 
> Well, writing an email to hundred of receivers, or writing 10 or more
> emails at the same time is nowadays an example how to get your email
> into spam box in lot of companies.
> 
>> Also there original reaction indicated that the info is not
>> available in ACPI, so probing + extending the DMI match
>> list seems to be the only way.
> 
> I have verified this statement years ago and therefore it applies only
> for old models (about 10 years old). So using this statement is not
> valid for new models anymore.

The latest Dell model which I have which still has a SM088xx ACPI
device is a Dell XPS 15 9550 from ~2016 this one still only has
an interrupt resource for the SMO8810 ACPI device and no 
I2cSerialBus[V2] resource.

On my Dell XPS 15 9550 the SMO8810 ACPI device is disabled,
still the accelerometer is actually there, but the IRQ is not setup
by the BIOS. So it can only work as an accelerometer and not
for /dev/freefall, likely because my model has the big battery
which covers the room for a 2.5" sata disk, so the freefall
function is disabled in the BIOS.

I have a patch series to still make the accelerometer work
on the XPS 15 9550 even though it is disabled in the BIOS,
but first lets get this series merged and then I'll send
the XPS 15 9550 series to be applied on top later.

Regards,

Hans

Dell contacts (was: [PATCH 6/6] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address)

[Paul Menzel]

Dear Greg,


Am 05.01.24 um 17:36 schrieb Hans de Goede:

> On 12/24/23 23:07, Pali Rohár wrote:
>> On Sunday 24 December 2023 22:36:22 Hans de Goede wrote:
>>> Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
>>> of the accelerometer. So a DMI product-name to address mapping table
>>> is used.
>>>
>>> At support to have the kernel probe for the i2c-address for models
>>> which are not on the list.
>>>
>>> The new probing code sits behind a new probe_i2c_addr module parameter,
>>> which is disabled by default because probing might be dangerous.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> I would really like to hear Dell opinion about this change, and if there
>> is really no way to get i2c address. Could you ask Dell people about it?
>> Always it is better to use official / vendor provided steps of hardware
>> detection, instead of inventing something new / own which would be there
>> for a long time...
> 
> Unfortunately I no longer have any contacts inside Dell for
> this and given Dell's non response in the original thread
> which started this I'm not hopeful for help from Dell here.

Unfortunately, since Mario Limonciello left Dell and works at AMD now, 
despite adding Dell.Client.Kernel@dell.com to Cc: I never received a 
reply from them. Do you have any contacts?

(Dell ships Ubuntu on the “developer machines”, but I could never figure 
out, how the relationship works. At least the Dell support always said, 
GNU/Linux support is provided by “the community”.)


Kind regards,

Paul

Re: Dell contacts (was: [PATCH 6/6] platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address)

[Greg KH]

On Mon, Jan 08, 2024 at 02:22:09PM +0100, Paul Menzel wrote:
> Dear Greg,
> 
> 
> Am 05.01.24 um 17:36 schrieb Hans de Goede:
> 
> > On 12/24/23 23:07, Pali Rohár wrote:
> > > On Sunday 24 December 2023 22:36:22 Hans de Goede wrote:
> > > > Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
> > > > of the accelerometer. So a DMI product-name to address mapping table
> > > > is used.
> > > > 
> > > > At support to have the kernel probe for the i2c-address for models
> > > > which are not on the list.
> > > > 
> > > > The new probing code sits behind a new probe_i2c_addr module parameter,
> > > > which is disabled by default because probing might be dangerous.
> > > > 
> > > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > 
> > > I would really like to hear Dell opinion about this change, and if there
> > > is really no way to get i2c address. Could you ask Dell people about it?
> > > Always it is better to use official / vendor provided steps of hardware
> > > detection, instead of inventing something new / own which would be there
> > > for a long time...
> > 
> > Unfortunately I no longer have any contacts inside Dell for
> > this and given Dell's non response in the original thread
> > which started this I'm not hopeful for help from Dell here.
> 
> Unfortunately, since Mario Limonciello left Dell and works at AMD now,
> despite adding Dell.Client.Kernel@dell.com to Cc: I never received a reply
> from them. Do you have any contacts?

I do not, sorry.

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

[Paul Menzel]

Dear Hans,


Thank you very much for working on this issue and even sending patches 
so quickly.


Am 24.12.23 um 22:36 schrieb Hans de Goede:

> 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 3/6 after some small prep patches
> 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 3/6? I think you'll like the changes there since they only
> remove code :)

> Hans de Goede (6):
>    platform/x86: dell-smo8800: Only load on Dell laptops
>    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: Instantiate an i2c_client for the IIO st_accel driver
>    platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address
> 
>   drivers/i2c/busses/i2c-i801.c            | 122 --------
>   drivers/platform/x86/dell/dell-smo8800.c | 337 +++++++++++++++++++++--
>   2 files changed, 316 insertions(+), 143 deletions(-)

This Thursday, I tested this on the Dell Inc. XPS 15 7590/0VYV0G, BIOS 
1.24.0 09/11/2023.

First just with your patch-set and trying the parameter:

     [    0.000000] Command line: BOOT_IMAGE=/vmlinuz-6.7.0-rc8+ 
root=UUID=9fa41e21-7a5f-479e-afdc-9a5503368d8e ro quiet rd.luks=1 
rd.auto=1 dell-smo8800.probe_i2c_addr=0x29
     […]
     [   28.826356] smo8800 SMO8810:00: Accelerometer lis3lv02d is 
present on SMBus but its address is unknown, skipping registration
     [   28.826359] smo8800 SMO8810:00: Pass 
dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this 
may be dangerous!

I misread the parameter documentation, but didn’t see the message back 
then, and just saved the log files.

So, I added an entry for the device, and got:

     [   19.197838] smo8800 SMO8810:00: Registered lis2de12 
accelerometer on address 0x29


Kind regards,

Paul


PS: I still seem to miss some config option in my custom Linux kernel 
configuration, as with my self-built Linux kernel, the accelerometer is 
not detected as an input device.

```
$ sudo dmesg | grep input
[    0.648449] input: AT Translated Set 2 keyboard as 
/devices/platform/i8042/serio0/input/input0
[   19.164633] input: Intel HID events as 
/devices/platform/INT33D5:00/input/input2
[   19.176109] input: Intel HID 5 button array as 
/devices/platform/INT33D5:00/input/input3
[   19.200645] input: Lid Switch as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0D:00/input/input4
[   19.230941] input: Power Button as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input5
[   19.231434] input: Sleep Button as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0E:00/input/input6
[   19.350390] input: PC Speaker as /devices/platform/pcspkr/input/input7
[   19.996196] input: Dell WMI hotkeys as 
/devices/platform/PNP0C14:05/wmi_bus/wmi_bus-PNP0C14:05/9DBB5994-A997-11DA-B012-B622A1EF5492/input/input9
[   20.014546] input: SYNA2393:00 06CB:7A13 Mouse as 
/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input10
[   20.047534] input: SYNA2393:00 06CB:7A13 Touchpad as 
/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input11
[   20.047667] hid-generic 0018:06CB:7A13.0001: input,hidraw0: I2C HID 
v1.00 Mouse [SYNA2393:00 06CB:7A13] on i2c-SYNA2393:00
[   20.048874] input: WCOM490B:00 056A:490B Touchscreen as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input13
[   20.049014] input: WCOM490B:00 056A:490B as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input14
[   20.049089] input: WCOM490B:00 056A:490B Stylus as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input15
[   20.049186] input: WCOM490B:00 056A:490B as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input16
[   20.065066] input: WCOM490B:00 056A:490B Mouse as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input17
[   20.065360] hid-generic 0018:056A:490B.0002: input,hidraw1: I2C HID 
v1.00 Mouse [WCOM490B:00 056A:490B] on i2c-WCOM490B:00
[   20.760879] input: SYNA2393:00 06CB:7A13 Mouse as 
/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input18
[   20.760979] input: SYNA2393:00 06CB:7A13 Touchpad as 
/devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input19
[   20.761032] hid-multitouch 0018:06CB:7A13.0001: input,hidraw0: I2C 
HID v1.00 Mouse [SYNA2393:00 06CB:7A13] on i2c-SYNA2393:00
[   21.083016] input: Wacom HID 490B Pen as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input21
[   21.083149] input: Wacom HID 490B Finger as 
/devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input22
[   25.850198] input: Video Bus as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input24
[   25.850344] input: Video Bus as 
/devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:0b/LNXVIDEO:01/input/input25
[   26.027649] snd_hda_codec_realtek hdaudioC0D0:    inputs:
[   26.132076] input: HDA Intel PCH Headphone Mic as 
/devices/pci0000:00/0000:00:1f.3/sound/card0/input26
[   26.132148] input: HDA Intel PCH HDMI/DP,pcm=3 as 
/devices/pci0000:00/0000:00:1f.3/sound/card0/input27
[   26.132192] input: HDA Intel PCH HDMI/DP,pcm=7 as 
/devices/pci0000:00/0000:00:1f.3/sound/card0/input28
[   26.132233] input: HDA Intel PCH HDMI/DP,pcm=8 as 
/devices/pci0000:00/0000:00:1f.3/sound/card0/input29
[   28.659169] rfkill: input handler disabled
[   47.283492] rfkill: input handler enabled
[   52.883611] rfkill: input handler disabled
```

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

[Hans de Goede]

Hi Paul,

On 1/6/24 15:23, Paul Menzel wrote:
> Dear Hans,
> 
> 
> Thank you very much for working on this issue and even sending patches so quickly.

You're welcome this was a nice little project to work on during the holidays.

> Am 24.12.23 um 22:36 schrieb Hans de Goede:
> 
>> 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 3/6 after some small prep patches
>> 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 3/6? I think you'll like the changes there since they only
>> remove code :)
> 
>> Hans de Goede (6):
>>    platform/x86: dell-smo8800: Only load on Dell laptops
>>    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: Instantiate an i2c_client for the IIO st_accel driver
>>    platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address
>>
>>   drivers/i2c/busses/i2c-i801.c            | 122 --------
>>   drivers/platform/x86/dell/dell-smo8800.c | 337 +++++++++++++++++++++--
>>   2 files changed, 316 insertions(+), 143 deletions(-)
> 
> This Thursday, I tested this on the Dell Inc. XPS 15 7590/0VYV0G, BIOS 1.24.0 09/11/2023.

Interesting, can you run:

sudo acpidump -o acpidump.txt and then send me a private email
with the generated acpidump.txt attached ?

> 
> First just with your patch-set and trying the parameter:
> 
>     [    0.000000] Command line: BOOT_IMAGE=/vmlinuz-6.7.0-rc8+ root=UUID=9fa41e21-7a5f-479e-afdc-9a5503368d8e ro quiet rd.luks=1 rd.auto=1 dell-smo8800.probe_i2c_addr=0x29
>     […]
>     [   28.826356] smo8800 SMO8810:00: Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration
>     [   28.826359] smo8800 SMO8810:00: Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!
> 
> I misread the parameter documentation, but didn’t see the message back then, and just saved the log files.
> 
> So, I added an entry for the device, and got:
> 
>     [   19.197838] smo8800 SMO8810:00: Registered lis2de12 accelerometer on address 0x29

Ok, that looks good. Can you provide the output of:

cat /sys/class/dmi/id/product_name

So that we can also add an entry for this upstream ?

> PS: I still seem to miss some config option in my custom Linux kernel configuration, as with my self-built Linux kernel, the accelerometer is not detected as an input device.

Right, v1 of my patches changed the code to by default instantiate an i2c_client
to which the st_accel IIO driver will bind. Using the IIO framework is
how accelerometers are handled normally and the handling of these "freefall" 
sensors so far has been a bit different, so I tried to make them more like
normal accelerometers which don't do the joystick emulation.

But Pali and Andy pointed out to me that there is userspace code out
there relying on /dev/freefall, so for v2 of the patches I've kept
the old behavior by default.

I've just posted v2 of the patches.

Note with v1 you can also get the old behavior by adding
dell_smo8800.use_misc_lis3lv02d=1 to the kernel commandline.

Adding that (or switching to the v2 patches) should give you
an input device.

Regards,

Hans






> 
> ```
> $ sudo dmesg | grep input
> [    0.648449] input: AT Translated Set 2 keyboard as /devices/platform/i8042/serio0/input/input0
> [   19.164633] input: Intel HID events as /devices/platform/INT33D5:00/input/input2
> [   19.176109] input: Intel HID 5 button array as /devices/platform/INT33D5:00/input/input3
> [   19.200645] input: Lid Switch as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0D:00/input/input4
> [   19.230941] input: Power Button as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input5
> [   19.231434] input: Sleep Button as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0E:00/input/input6
> [   19.350390] input: PC Speaker as /devices/platform/pcspkr/input/input7
> [   19.996196] input: Dell WMI hotkeys as /devices/platform/PNP0C14:05/wmi_bus/wmi_bus-PNP0C14:05/9DBB5994-A997-11DA-B012-B622A1EF5492/input/input9
> [   20.014546] input: SYNA2393:00 06CB:7A13 Mouse as /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input10
> [   20.047534] input: SYNA2393:00 06CB:7A13 Touchpad as /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input11
> [   20.047667] hid-generic 0018:06CB:7A13.0001: input,hidraw0: I2C HID v1.00 Mouse [SYNA2393:00 06CB:7A13] on i2c-SYNA2393:00
> [   20.048874] input: WCOM490B:00 056A:490B Touchscreen as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input13
> [   20.049014] input: WCOM490B:00 056A:490B as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input14
> [   20.049089] input: WCOM490B:00 056A:490B Stylus as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input15
> [   20.049186] input: WCOM490B:00 056A:490B as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input16
> [   20.065066] input: WCOM490B:00 056A:490B Mouse as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input17
> [   20.065360] hid-generic 0018:056A:490B.0002: input,hidraw1: I2C HID v1.00 Mouse [WCOM490B:00 056A:490B] on i2c-WCOM490B:00
> [   20.760879] input: SYNA2393:00 06CB:7A13 Mouse as /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input18
> [   20.760979] input: SYNA2393:00 06CB:7A13 Touchpad as /devices/pci0000:00/0000:00:15.1/i2c_designware.1/i2c-2/i2c-SYNA2393:00/0018:06CB:7A13.0001/input/input19
> [   20.761032] hid-multitouch 0018:06CB:7A13.0001: input,hidraw0: I2C HID v1.00 Mouse [SYNA2393:00 06CB:7A13] on i2c-SYNA2393:00
> [   21.083016] input: Wacom HID 490B Pen as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input21
> [   21.083149] input: Wacom HID 490B Finger as /devices/pci0000:00/0000:00:15.0/i2c_designware.0/i2c-1/i2c-WCOM490B:00/0018:056A:490B.0002/input/input22
> [   25.850198] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input24
> [   25.850344] input: Video Bus as /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/device:0b/LNXVIDEO:01/input/input25
> [   26.027649] snd_hda_codec_realtek hdaudioC0D0:    inputs:
> [   26.132076] input: HDA Intel PCH Headphone Mic as /devices/pci0000:00/0000:00:1f.3/sound/card0/input26
> [   26.132148] input: HDA Intel PCH HDMI/DP,pcm=3 as /devices/pci0000:00/0000:00:1f.3/sound/card0/input27
> [   26.132192] input: HDA Intel PCH HDMI/DP,pcm=7 as /devices/pci0000:00/0000:00:1f.3/sound/card0/input28
> [   26.132233] input: HDA Intel PCH HDMI/DP,pcm=8 as /devices/pci0000:00/0000:00:1f.3/sound/card0/input29
> [   28.659169] rfkill: input handler disabled
> [   47.283492] rfkill: input handler enabled
> [   52.883611] rfkill: input handler disabled
> ```
> 

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

[Paul Menzel]

Dear Hans,


Again, thank you for your reply.


Am 06.01.24 um 17:15 schrieb Hans de Goede:

> On 1/6/24 15:23, Paul Menzel wrote:

[…]

>> Am 24.12.23 um 22:36 schrieb Hans de Goede:
>>
>>> 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 3/6 after some small prep patches
>>> 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 3/6? I think you'll like the changes there since they only
>>> remove code :)
>>
>>> Hans de Goede (6):
>>>     platform/x86: dell-smo8800: Only load on Dell laptops
>>>     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: Instantiate an i2c_client for the IIO st_accel driver
>>>     platform/x86: dell-smo8800: Add support for probing for the accelerometer i2c address
>>>
>>>    drivers/i2c/busses/i2c-i801.c            | 122 --------
>>>    drivers/platform/x86/dell/dell-smo8800.c | 337 +++++++++++++++++++++--
>>>    2 files changed, 316 insertions(+), 143 deletions(-)
>>
>> This Thursday, I tested this on the Dell Inc. XPS 15 7590/0VYV0G, BIOS 1.24.0 09/11/2023.
> 
> Interesting, can you run:
> 
> sudo acpidump -o acpidump.txt and then send me a private email
> with the generated acpidump.txt attached ?

Please find it in the Linux Kernel Bugzilla [1], where I attached it to 
another issue.

>> First just with your patch-set and trying the parameter:
>>
>>      [    0.000000] Command line: BOOT_IMAGE=/vmlinuz-6.7.0-rc8+ root=UUID=9fa41e21-7a5f-479e-afdc-9a5503368d8e ro quiet rd.luks=1 rd.auto=1 dell-smo8800.probe_i2c_addr=0x29
>>      […]
>>      [   28.826356] smo8800 SMO8810:00: Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration
>>      [   28.826359] smo8800 SMO8810:00: Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!
>>
>> I misread the parameter documentation, but didn’t see the message back then, and just saved the log files.
>>
>> So, I added an entry for the device, and got:
>>
>>      [   19.197838] smo8800 SMO8810:00: Registered lis2de12 accelerometer on address 0x29
> 
> Ok, that looks good. Can you provide the output of:
> 
> cat /sys/class/dmi/id/product_name

 From my upload to Hardware for Linux [2]:

XPS 15 7590

> So that we can also add an entry for this upstream ?

I already sent a patch, that got applied [3].

>> PS: I still seem to miss some config option in my custom Linux
>> kernel configuration, as with my self-built Linux kernel, the
>> accelerometer is not detected as an input device.
> 
> Right, v1 of my patches changed the code to by default instantiate an i2c_client
> to which the st_accel IIO driver will bind. Using the IIO framework is
> how accelerometers are handled normally and the handling of these "freefall"
> sensors so far has been a bit different, so I tried to make them more like
> normal accelerometers which don't do the joystick emulation.
> 
> But Pali and Andy pointed out to me that there is userspace code out
> there relying on /dev/freefall, so for v2 of the patches I've kept
> the old behavior by default.
> 
> I've just posted v2 of the patches.
> 
> Note with v1 you can also get the old behavior by adding
> dell_smo8800.use_misc_lis3lv02d=1 to the kernel commandline.
> 
> Adding that (or switching to the v2 patches) should give you
> an input device.

Thank you very much. I am going to test them as soon as possible.


Kind regards,

Paul


[1]: https://bugzilla.kernel.org/show_bug.cgi?id=218287
      "`ACPI Error: AE_ERROR, Returned by Handler for [PCI_Config] 
(20230628/evregion-300)`"
[2]: https://linux-hardware.org/?probe=74136911a0&log=dmidecode
[3]: 
https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git/commit/?h=i2c/for-next&id=dc3293b460db70e3b5b76175d1a158dc802b9740