diff mbox series

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

Message ID 20240621122503.10034-4-hdegoede@redhat.com
State New
Headers show
Series i2c-i801 / dell-smo8800: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-smo8800 | expand

Commit Message

Hans de Goede June 21, 2024, 12:24 p.m. UTC 
It is not necessary to handle the Dell specific instantiation of
i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
inside the generic i801 I2C adapter driver.

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

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

Moving the i2c_client instantiation here has the following advantages:

1. This moves the SMO88xx ACPI device quirk handling away from the generic
i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
specific dell-smo8800 module where it belongs.

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

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

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note the goto out_put_adapter, which can be avoided by moving the DMI check
up, is there deliberately as preparation for adding support to probe for
the i2c address in case there is no DMI match.
---
Changes in v3:
- Use an i2c bus notifier so that the i2c_client will still be instantiated if
  the i801 i2c_adapter shows up later or is re-probed (removed + added again)
- Switch to standard dmi_system_id matching to check both sys-vendor +
  product-name DMI fields
- Use unique i2c_adapter->name prefix for primary i2c_801 controller
  to avoid needing to duplicate PCI ids for extra IDF i2c_801 i2c_adapter-s
- Drop MODULE_SOFTDEP("pre: i2c-i801"), this is now no longer necessary
- Rebase on Torvalds master for recent additions of extra models in
  the dell_lis3lv02d_devices[] list

Changes in v2:
- Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
- Add a comment documenting the IDF PCI device ids
---
 drivers/i2c/busses/i2c-i801.c            | 124 -------------
 drivers/platform/x86/dell/dell-smo8800.c | 214 ++++++++++++++++++++++-
 2 files changed, 213 insertions(+), 125 deletions(-)

Comments

Andy Shevchenko June 21, 2024, 3:24 p.m. UTC | #1 
On Fri, Jun 21, 2024 at 2:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> It is not necessary to handle the Dell specific instantiation of
> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
> inside the generic i801 I2C adapter driver.
>
> The kernel already instantiates platform_device-s for these ACPI devices
> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> platform drivers.
>
> Move the i2c_client instantiation from the generic i2c-i801 driver to
> the SMO88xx specific dell-smo8800 driver.
>
> Moving the i2c_client instantiation here has the following advantages:
>
> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
> i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
> specific dell-smo8800 module where it belongs.
>
> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
> between the i2c-i801 and dell-smo8800 drivers.
>
> 3. This allows extending the quirk handling by adding new code and related
> module parameters to the dell-smo8800 driver, without needing to modify
> the i2c-i801 code.

...


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

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

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

...

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

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

...

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

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

--
With Best Regards,
Andy Shevchenko
Pali Rohár June 22, 2024, 1:16 p.m. UTC | #2 
On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> It is not necessary to handle the Dell specific instantiation of
> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
> inside the generic i801 I2C adapter driver.
> 
> The kernel already instantiates platform_device-s for these ACPI devices
> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> platform drivers.
> 
> Move the i2c_client instantiation from the generic i2c-i801 driver to
> the SMO88xx specific dell-smo8800 driver.

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

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

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

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

> Moving the i2c_client instantiation here has the following advantages:
> 
> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
> i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
> specific dell-smo8800 module where it belongs.
> 
> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
> between the i2c-i801 and dell-smo8800 drivers.
> 
> 3. This allows extending the quirk handling by adding new code and related
> module parameters to the dell-smo8800 driver, without needing to modify
> the i2c-i801 code.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Note the goto out_put_adapter, which can be avoided by moving the DMI check
> up, is there deliberately as preparation for adding support to probe for
> the i2c address in case there is no DMI match.
> ---
> Changes in v3:
> - Use an i2c bus notifier so that the i2c_client will still be instantiated if
>   the i801 i2c_adapter shows up later or is re-probed (removed + added again)
> - Switch to standard dmi_system_id matching to check both sys-vendor +
>   product-name DMI fields
> - Use unique i2c_adapter->name prefix for primary i2c_801 controller
>   to avoid needing to duplicate PCI ids for extra IDF i2c_801 i2c_adapter-s
> - Drop MODULE_SOFTDEP("pre: i2c-i801"), this is now no longer necessary
> - Rebase on Torvalds master for recent additions of extra models in
>   the dell_lis3lv02d_devices[] list
> 
> Changes in v2:
> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
> - Add a comment documenting the IDF PCI device ids
> ---
>  drivers/i2c/busses/i2c-i801.c            | 124 -------------
>  drivers/platform/x86/dell/dell-smo8800.c | 214 ++++++++++++++++++++++-
>  2 files changed, 213 insertions(+), 125 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 5ac5bbd60d45..db8d31411148 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1153,127 +1153,6 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
>  	}
>  }
>  
> -/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */
> -static const char *const acpi_smo8800_ids[] = {
> -	"SMO8800",
> -	"SMO8801",
> -	"SMO8810",
> -	"SMO8811",
> -	"SMO8820",
> -	"SMO8821",
> -	"SMO8830",
> -	"SMO8831",
> -};
> -
> -static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
> -					     u32 nesting_level,
> -					     void *context,
> -					     void **return_value)
> -{
> -	struct acpi_device_info *info;
> -	acpi_status status;
> -	char *hid;
> -	int i;
> -
> -	status = acpi_get_object_info(obj_handle, &info);
> -	if (ACPI_FAILURE(status))
> -		return AE_OK;
> -
> -	if (!(info->valid & ACPI_VALID_HID))
> -		goto smo88xx_not_found;
> -
> -	hid = info->hardware_id.string;
> -	if (!hid)
> -		goto smo88xx_not_found;
> -
> -	i = match_string(acpi_smo8800_ids, ARRAY_SIZE(acpi_smo8800_ids), hid);
> -	if (i < 0)
> -		goto smo88xx_not_found;
> -
> -	kfree(info);
> -
> -	*return_value = NULL;
> -	return AE_CTRL_TERMINATE;
> -
> -smo88xx_not_found:
> -	kfree(info);
> -	return AE_OK;
> -}
> -
> -static bool is_dell_system_with_lis3lv02d(void)
> -{
> -	void *err = ERR_PTR(-ENOENT);
> -
> -	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
> -		return false;
> -
> -	/*
> -	 * Check that ACPI device SMO88xx is present and is functioning.
> -	 * Function acpi_get_devices() already filters all ACPI devices
> -	 * which are not present or are not functioning.
> -	 * ACPI device SMO88xx represents our ST microelectronics lis3lv02d
> -	 * accelerometer but unfortunately ACPI does not provide any other
> -	 * information (like I2C address).
> -	 */
> -	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
> -
> -	return !IS_ERR(err);
> -}
> -
> -/*
> - * Accelerometer's I2C address is not specified in DMI nor ACPI,
> - * so it is needed to define mapping table based on DMI product names.
> - */
> -static const struct {
> -	const char *dmi_product_name;
> -	unsigned short i2c_addr;
> -} dell_lis3lv02d_devices[] = {
> -	/*
> -	 * Dell platform team told us that these Latitude devices have
> -	 * ST microelectronics accelerometer at I2C address 0x29.
> -	 */
> -	{ "Latitude E5250",     0x29 },
> -	{ "Latitude E5450",     0x29 },
> -	{ "Latitude E5550",     0x29 },
> -	{ "Latitude E6440",     0x29 },
> -	{ "Latitude E6440 ATG", 0x29 },
> -	{ "Latitude E6540",     0x29 },
> -	/*
> -	 * Additional individual entries were added after verification.
> -	 */
> -	{ "Latitude 5480",      0x29 },
> -	{ "Precision 3540",     0x29 },
> -	{ "Vostro V131",        0x1d },
> -	{ "Vostro 5568",        0x29 },
> -	{ "XPS 15 7590",        0x29 },
> -};
> -
> -static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
> -{
> -	struct i2c_board_info info;
> -	const char *dmi_product_name;
> -	int i;
> -
> -	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> -	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
> -		if (strcmp(dmi_product_name,
> -			   dell_lis3lv02d_devices[i].dmi_product_name) == 0)
> -			break;
> -	}
> -
> -	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
> -		dev_warn(&priv->pci_dev->dev,
> -			 "Accelerometer lis3lv02d is present on SMBus but its"
> -			 " address is unknown, skipping registration\n");
> -		return;
> -	}
> -
> -	memset(&info, 0, sizeof(struct i2c_board_info));
> -	info.addr = dell_lis3lv02d_devices[i].i2c_addr;
> -	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> -	i2c_new_client_device(&priv->adapter, &info);
> -}
> -
>  /* Register optional slaves */
>  static void i801_probe_optional_slaves(struct i801_priv *priv)
>  {
> @@ -1293,9 +1172,6 @@ static void i801_probe_optional_slaves(struct i801_priv *priv)
>  	if (dmi_name_in_vendors("FUJITSU"))
>  		dmi_walk(dmi_check_onboard_devices, &priv->adapter);
>  
> -	if (is_dell_system_with_lis3lv02d())
> -		register_dell_lis3lv02d_i2c_device(priv);
> -
>  	/* Instantiate SPD EEPROMs unless the SMBus is multiplexed */
>  #ifdef CONFIG_I2C_I801_MUX
>  	if (!priv->mux_pdev)
> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> index f7ec17c56833..cd2e48405859 100644
> --- a/drivers/platform/x86/dell/dell-smo8800.c
> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> @@ -4,20 +4,26 @@
>   *
>   *  Copyright (C) 2012 Sonal Santan <sonal.santan@gmail.com>
>   *  Copyright (C) 2014 Pali Rohár <pali@kernel.org>
> + *  Copyright (C) 2023 Hans de Goede <hansg@kernel.org>
>   *
>   *  This is loosely based on lis3lv02d driver.
>   */
>  
>  #define DRIVER_NAME "smo8800"
>  
> +#include <linux/device/bus.h>
> +#include <linux/dmi.h>
>  #include <linux/fs.h>
> +#include <linux/i2c.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/miscdevice.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/notifier.h>
>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
> +#include <linux/workqueue.h>
>  
>  struct smo8800_device {
>  	u32 irq;                     /* acpi device irq */
> @@ -25,6 +31,9 @@ struct smo8800_device {
>  	struct miscdevice miscdev;   /* for /dev/freefall */
>  	unsigned long misc_opened;   /* whether the device is open */
>  	wait_queue_head_t misc_wait; /* Wait queue for the misc dev */
> +	struct notifier_block i2c_nb;/* i2c bus notifier */
> +	struct work_struct i2c_work; /* Work for instantiating lis3lv02d i2c_client */
> +	struct i2c_client *i2c_dev;  /* i2c_client for lis3lv02d */
>  	struct device *dev;          /* acpi device */
>  };
>  
> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
>  	.release = smo8800_misc_release,
>  };
>  
> +/*
> + * Accelerometer's I2C address is not specified in DMI nor ACPI,
> + * so it is needed to define mapping table based on DMI product names.
> + */
> +static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> +	/*
> +	 * Dell platform team told us that these Latitude devices have
> +	 * ST microelectronics accelerometer at I2C address 0x29.
> +	 */
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	/*
> +	 * Additional individual entries were added after verification.
> +	 */
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> +		},
> +		.driver_data = (void *)0x1dL,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> +		},
> +		.driver_data = (void *)0x29L,
> +	},
> +	{
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> +		},
> +		.driver_data = (void *)0x29L,

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

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

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

| static const struct {
| 	const char *dmi_product_name;
| 	unsigned short i2c_addr;
| } dell_lis3lv02d_devices[] = {
| 	/*
| 	 * Dell platform team told us that these Latitude devices have
| 	 * ST microelectronics accelerometer at I2C address 0x29.
| 	 */
| 	{ "Latitude E5250",     0x29 },
| 	{ "Latitude E5450",     0x29 },
| 	{ "Latitude E5550",     0x29 },
| 	{ "Latitude E6440",     0x29 },
| 	{ "Latitude E6440 ATG", 0x29 },
| 	{ "Latitude E6540",     0x29 },

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

On 6/21/24 5:24 PM, Andy Shevchenko wrote:
> On Fri, Jun 21, 2024 at 2:25 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> It is not necessary to handle the Dell specific instantiation of
>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
>> inside the generic i801 I2C adapter driver.
>>
>> The kernel already instantiates platform_device-s for these ACPI devices
>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
>> platform drivers.
>>
>> Move the i2c_client instantiation from the generic i2c-i801 driver to
>> the SMO88xx specific dell-smo8800 driver.
>>
>> Moving the i2c_client instantiation here has the following advantages:
>>
>> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
>> i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
>> specific dell-smo8800 module where it belongs.
>>
>> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
>> between the i2c-i801 and dell-smo8800 drivers.
>>
>> 3. This allows extending the quirk handling by adding new code and related
>> module parameters to the dell-smo8800 driver, without needing to modify
>> the i2c-i801 code.
> 
> ...
> 
> 
>> +static int smo8800_find_i801(struct device *dev, void *data)
>> +{
>> +       struct i2c_adapter *adap, **adap_ret = data;
>> +
>> +       adap = i2c_verify_adapter(dev);
>> +       if (!adap)
>> +               return 0;
>> +
>> +       if (!strstarts(adap->name, "SMBus I801 adapter"))
> 
> With the comment on the previous patch I'm wondering if it makes sense
> to have this to be as simple as strstr("I801") or strstr("I801 IDF")?

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

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

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

Regards,

Hans
Hans de Goede June 22, 2024, 2:06 p.m. UTC | #4 
Hi Pali,

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

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

>> Moving the i2c_client instantiation here has the following advantages:
>>
>> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
>> i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
>> specific dell-smo8800 module where it belongs.
>>
>> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
>> between the i2c-i801 and dell-smo8800 drivers.
>>
>> 3. This allows extending the quirk handling by adding new code and related
>> module parameters to the dell-smo8800 driver, without needing to modify
>> the i2c-i801 code.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Note the goto out_put_adapter, which can be avoided by moving the DMI check
>> up, is there deliberately as preparation for adding support to probe for
>> the i2c address in case there is no DMI match.
>> ---
>> Changes in v3:
>> - Use an i2c bus notifier so that the i2c_client will still be instantiated if
>>   the i801 i2c_adapter shows up later or is re-probed (removed + added again)
>> - Switch to standard dmi_system_id matching to check both sys-vendor +
>>   product-name DMI fields
>> - Use unique i2c_adapter->name prefix for primary i2c_801 controller
>>   to avoid needing to duplicate PCI ids for extra IDF i2c_801 i2c_adapter-s
>> - Drop MODULE_SOFTDEP("pre: i2c-i801"), this is now no longer necessary
>> - Rebase on Torvalds master for recent additions of extra models in
>>   the dell_lis3lv02d_devices[] list
>>
>> Changes in v2:
>> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
>> - Add a comment documenting the IDF PCI device ids
>> ---
>>  drivers/i2c/busses/i2c-i801.c            | 124 -------------
>>  drivers/platform/x86/dell/dell-smo8800.c | 214 ++++++++++++++++++++++-
>>  2 files changed, 213 insertions(+), 125 deletions(-)

<snip>

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

...

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

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

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

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

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

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

Regards,

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

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

> >> Moving the i2c_client instantiation here has the following advantages:
> >>
> >> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
> >> i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
> >> specific dell-smo8800 module where it belongs.
> >>
> >> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
> >> between the i2c-i801 and dell-smo8800 drivers.
> >>
> >> 3. This allows extending the quirk handling by adding new code and related
> >> module parameters to the dell-smo8800 driver, without needing to modify
> >> the i2c-i801 code.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >> ---
> >> Note the goto out_put_adapter, which can be avoided by moving the DMI check
> >> up, is there deliberately as preparation for adding support to probe for
> >> the i2c address in case there is no DMI match.
> >> ---
> >> Changes in v3:
> >> - Use an i2c bus notifier so that the i2c_client will still be instantiated if
> >>   the i801 i2c_adapter shows up later or is re-probed (removed + added again)
> >> - Switch to standard dmi_system_id matching to check both sys-vendor +
> >>   product-name DMI fields
> >> - Use unique i2c_adapter->name prefix for primary i2c_801 controller
> >>   to avoid needing to duplicate PCI ids for extra IDF i2c_801 i2c_adapter-s
> >> - Drop MODULE_SOFTDEP("pre: i2c-i801"), this is now no longer necessary
> >> - Rebase on Torvalds master for recent additions of extra models in
> >>   the dell_lis3lv02d_devices[] list
> >>
> >> Changes in v2:
> >> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
> >> - Add a comment documenting the IDF PCI device ids
> >> ---
> >>  drivers/i2c/busses/i2c-i801.c            | 124 -------------
> >>  drivers/platform/x86/dell/dell-smo8800.c | 214 ++++++++++++++++++++++-
> >>  2 files changed, 213 insertions(+), 125 deletions(-)
> 
> <snip>
> 
> >> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> >> index f7ec17c56833..cd2e48405859 100644
> >> --- a/drivers/platform/x86/dell/dell-smo8800.c
> >> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> 
> ...
> 
> >> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
> >>  	.release = smo8800_misc_release,
> >>  };
> >>  
> >> +/*
> >> + * Accelerometer's I2C address is not specified in DMI nor ACPI,
> >> + * so it is needed to define mapping table based on DMI product names.
> >> + */
> >> +static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> >> +	/*
> >> +	 * Dell platform team told us that these Latitude devices have
> >> +	 * ST microelectronics accelerometer at I2C address 0x29.
> >> +	 */
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	/*
> >> +	 * Additional individual entries were added after verification.
> >> +	 */
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> >> +		},
> >> +		.driver_data = (void *)0x1dL,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> >> +	},
> >> +	{
> >> +		.matches = {
> >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> >> +		},
> >> +		.driver_data = (void *)0x29L,
> > 
> > At least for me, casting i2c address to LONG and then to pointer looks
> > very strange. If I look at this code without knowing what the number
> > 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
> > address.
> > 
> > Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
> > instead of ".something = (void *)0x29L" to make it readable?
> 
> struct dmi_system_id is an existing structure and we cannot just go adding
> fields to it. driver_data is intended to tie driver specific data to
> each DMI match, often pointing to some struct, so it is a void *, but

Yes, I know it.

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

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

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

Ok, then let that table as you have it now.
Hans de Goede June 22, 2024, 2:26 p.m. UTC | #6 
Hi Pali,

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

The driver needs to bind to something ...

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

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

Regards,

Hans






> 
>>>> Moving the i2c_client instantiation here has the following advantages:
>>>>
>>>> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
>>>> i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
>>>> specific dell-smo8800 module where it belongs.
>>>>
>>>> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
>>>> between the i2c-i801 and dell-smo8800 drivers.
>>>>
>>>> 3. This allows extending the quirk handling by adding new code and related
>>>> module parameters to the dell-smo8800 driver, without needing to modify
>>>> the i2c-i801 code.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>> Note the goto out_put_adapter, which can be avoided by moving the DMI check
>>>> up, is there deliberately as preparation for adding support to probe for
>>>> the i2c address in case there is no DMI match.
>>>> ---
>>>> Changes in v3:
>>>> - Use an i2c bus notifier so that the i2c_client will still be instantiated if
>>>>   the i801 i2c_adapter shows up later or is re-probed (removed + added again)
>>>> - Switch to standard dmi_system_id matching to check both sys-vendor +
>>>>   product-name DMI fields
>>>> - Use unique i2c_adapter->name prefix for primary i2c_801 controller
>>>>   to avoid needing to duplicate PCI ids for extra IDF i2c_801 i2c_adapter-s
>>>> - Drop MODULE_SOFTDEP("pre: i2c-i801"), this is now no longer necessary
>>>> - Rebase on Torvalds master for recent additions of extra models in
>>>>   the dell_lis3lv02d_devices[] list
>>>>
>>>> Changes in v2:
>>>> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
>>>> - Add a comment documenting the IDF PCI device ids
>>>> ---
>>>>  drivers/i2c/busses/i2c-i801.c            | 124 -------------
>>>>  drivers/platform/x86/dell/dell-smo8800.c | 214 ++++++++++++++++++++++-
>>>>  2 files changed, 213 insertions(+), 125 deletions(-)
>>
>> <snip>
>>
>>>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
>>>> index f7ec17c56833..cd2e48405859 100644
>>>> --- a/drivers/platform/x86/dell/dell-smo8800.c
>>>> +++ b/drivers/platform/x86/dell/dell-smo8800.c
>>
>> ...
>>
>>>> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
>>>>  	.release = smo8800_misc_release,
>>>>  };
>>>>  
>>>> +/*
>>>> + * Accelerometer's I2C address is not specified in DMI nor ACPI,
>>>> + * so it is needed to define mapping table based on DMI product names.
>>>> + */
>>>> +static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
>>>> +	/*
>>>> +	 * Dell platform team told us that these Latitude devices have
>>>> +	 * ST microelectronics accelerometer at I2C address 0x29.
>>>> +	 */
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	/*
>>>> +	 * Additional individual entries were added after verification.
>>>> +	 */
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
>>>> +		},
>>>> +		.driver_data = (void *)0x1dL,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>> +	},
>>>> +	{
>>>> +		.matches = {
>>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
>>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
>>>> +		},
>>>> +		.driver_data = (void *)0x29L,
>>>
>>> At least for me, casting i2c address to LONG and then to pointer looks
>>> very strange. If I look at this code without knowing what the number
>>> 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
>>> address.
>>>
>>> Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
>>> instead of ".something = (void *)0x29L" to make it readable?
>>
>> struct dmi_system_id is an existing structure and we cannot just go adding
>> fields to it. driver_data is intended to tie driver specific data to
>> each DMI match, often pointing to some struct, so it is a void *, but
> 
> Yes, I know it.
> 
>> in this case we only need a single integer, so we store that in the
>> pointer. That is is the address becomes obvious when looking at the code
>> which consumes the data.
> 
> Ok, this makes sense. Anyway, is explicit void* cast and L suffix
> required?
> 
>>> Also does the whole device table has to be such verbose with lot of
>>> duplicated information (which probably also increase size of every linux
>>> image which includes this driver into it)?
>>
>> struct dmi_system_id is the default way to specify DMI matches in
>> the kernel. This avoids code duplication in the form of writing
>> a DYI function to do the matching.
>>
>> In v2 of the patch-set I only matched on product-name, but you asked
>> in the review of v2 to also match on sys-vendor and you mentioned
>> we may want to support other sys-vendors too, since some other brands
>> have SMO88xx ACPI devices too. This more or less automatically leads
>> to using the kernel's standard, existing, DMI matching mechanism.
>>
>> We really want to avoid coming up with something "new" ourselves here
>> leading to unnecessary code duplication.
>>
>> Regards,
>>
>> Hans
> 
> Ok, then let that table as you have it now.
>
Pali Rohár June 22, 2024, 3:12 p.m. UTC | #7 
On Saturday 22 June 2024 16:26:13 Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 4:20 PM, Pali Rohár wrote:
> > On Saturday 22 June 2024 16:06:01 Hans de Goede wrote:
> >> Hi Pali,
> >>
> >> On 6/22/24 3:16 PM, Pali Rohár wrote:
> >>> On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> >>>> It is not necessary to handle the Dell specific instantiation of
> >>>> i2c_client-s for SMO88xx ACPI devices without an ACPI I2cResource
> >>>> inside the generic i801 I2C adapter driver.
> >>>>
> >>>> The kernel already instantiates platform_device-s for these ACPI devices
> >>>> and the drivers/platform/x86/dell/dell-smo8800.c driver binds to these
> >>>> platform drivers.
> >>>>
> >>>> Move the i2c_client instantiation from the generic i2c-i801 driver to
> >>>> the SMO88xx specific dell-smo8800 driver.
> >>>
> >>> Why it has to be in dell-smo8800 driver? Code for registering lis3lv02d
> >>> and freefall code for smo88xx are basically independent.
> >>>
> >>> lis3lv02d is for accelerometer axes and smo88xx is for freefall hardisk
> >>> detection. The only thing which have these "drivers" common is the ACPI
> >>> detection mechanism based on presence of SMO88?? identifiers from
> >>> acpi_smo8800_ids[] array.
> >>>
> >>> I think it makes both "drivers" cleaner if they are put into separate
> >>> files as they are independent of each one.
> >>>
> >>> What about moving it into drivers/platform/x86/dell/dell-lis3lv02d.c
> >>> instead (or similar name)? And just share list of ACPI ids via some
> >>> header file (or something like that).
> >>
> >> Interesting idea, but that will not work, only 1 driver can bind to
> >> the platform_device instantiated by the ACPI code for the SMO88xx ACPI device.
> > 
> > And it is required to bind lis3 device to ACPI code? What is needed is
> > just to check if system matches DMI strings and ACPI strings. You are
> > not binding device to DMI strings, so I think there is no need to bind
> > it neither to ACPI strings.
> 
> The driver needs to bind to something ...

Why?

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

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

So I do not see where is that binding requirement.

> This is code for special handling required for SMO88xx ACPI devices,
> dell-smo8800 is *the* driver for those ACPI devices. So this code clearly
> belongs here.
> 
> According to diffstat this adds about 400 lines of code that is really not
> that big, so I see no urgent reason to introduce weird tricks instead of
> standard driver binding for this.
> 
> Regards,
> 
> Hans
> 
> 
> 
> 
> 
> 
> > 
> >>>> Moving the i2c_client instantiation here has the following advantages:
> >>>>
> >>>> 1. This moves the SMO88xx ACPI device quirk handling away from the generic
> >>>> i2c-i801 module which is loaded on all Intel x86 machines to the SMO88xx
> >>>> specific dell-smo8800 module where it belongs.
> >>>>
> >>>> 2. This removes the duplication of the SMO88xx ACPI Hardware ID (HID) table
> >>>> between the i2c-i801 and dell-smo8800 drivers.
> >>>>
> >>>> 3. This allows extending the quirk handling by adding new code and related
> >>>> module parameters to the dell-smo8800 driver, without needing to modify
> >>>> the i2c-i801 code.
> >>>>
> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>> ---
> >>>> Note the goto out_put_adapter, which can be avoided by moving the DMI check
> >>>> up, is there deliberately as preparation for adding support to probe for
> >>>> the i2c address in case there is no DMI match.
> >>>> ---
> >>>> Changes in v3:
> >>>> - Use an i2c bus notifier so that the i2c_client will still be instantiated if
> >>>>   the i801 i2c_adapter shows up later or is re-probed (removed + added again)
> >>>> - Switch to standard dmi_system_id matching to check both sys-vendor +
> >>>>   product-name DMI fields
> >>>> - Use unique i2c_adapter->name prefix for primary i2c_801 controller
> >>>>   to avoid needing to duplicate PCI ids for extra IDF i2c_801 i2c_adapter-s
> >>>> - Drop MODULE_SOFTDEP("pre: i2c-i801"), this is now no longer necessary
> >>>> - Rebase on Torvalds master for recent additions of extra models in
> >>>>   the dell_lis3lv02d_devices[] list
> >>>>
> >>>> Changes in v2:
> >>>> - Use a pci_device_id table to check for IDF (non main) i2c-i801 SMBusses
> >>>> - Add a comment documenting the IDF PCI device ids
> >>>> ---
> >>>>  drivers/i2c/busses/i2c-i801.c            | 124 -------------
> >>>>  drivers/platform/x86/dell/dell-smo8800.c | 214 ++++++++++++++++++++++-
> >>>>  2 files changed, 213 insertions(+), 125 deletions(-)
> >>
> >> <snip>
> >>
> >>>> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> >>>> index f7ec17c56833..cd2e48405859 100644
> >>>> --- a/drivers/platform/x86/dell/dell-smo8800.c
> >>>> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> >>
> >> ...
> >>
> >>>> @@ -103,6 +112,184 @@ static const struct file_operations smo8800_misc_fops = {
> >>>>  	.release = smo8800_misc_release,
> >>>>  };
> >>>>  
> >>>> +/*
> >>>> + * Accelerometer's I2C address is not specified in DMI nor ACPI,
> >>>> + * so it is needed to define mapping table based on DMI product names.
> >>>> + */
> >>>> +static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> >>>> +	/*
> >>>> +	 * Dell platform team told us that these Latitude devices have
> >>>> +	 * ST microelectronics accelerometer at I2C address 0x29.
> >>>> +	 */
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	/*
> >>>> +	 * Additional individual entries were added after verification.
> >>>> +	 */
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x1dL,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>> +	},
> >>>> +	{
> >>>> +		.matches = {
> >>>> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> >>>> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> >>>> +		},
> >>>> +		.driver_data = (void *)0x29L,
> >>>
> >>> At least for me, casting i2c address to LONG and then to pointer looks
> >>> very strange. If I look at this code without knowing what the number
> >>> 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
> >>> address.
> >>>
> >>> Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
> >>> instead of ".something = (void *)0x29L" to make it readable?
> >>
> >> struct dmi_system_id is an existing structure and we cannot just go adding
> >> fields to it. driver_data is intended to tie driver specific data to
> >> each DMI match, often pointing to some struct, so it is a void *, but
> > 
> > Yes, I know it.
> > 
> >> in this case we only need a single integer, so we store that in the
> >> pointer. That is is the address becomes obvious when looking at the code
> >> which consumes the data.
> > 
> > Ok, this makes sense. Anyway, is explicit void* cast and L suffix
> > required?
> > 
> >>> Also does the whole device table has to be such verbose with lot of
> >>> duplicated information (which probably also increase size of every linux
> >>> image which includes this driver into it)?
> >>
> >> struct dmi_system_id is the default way to specify DMI matches in
> >> the kernel. This avoids code duplication in the form of writing
> >> a DYI function to do the matching.
> >>
> >> In v2 of the patch-set I only matched on product-name, but you asked
> >> in the review of v2 to also match on sys-vendor and you mentioned
> >> we may want to support other sys-vendors too, since some other brands
> >> have SMO88xx ACPI devices too. This more or less automatically leads
> >> to using the kernel's standard, existing, DMI matching mechanism.
> >>
> >> We really want to avoid coming up with something "new" ourselves here
> >> leading to unnecessary code duplication.
> >>
> >> Regards,
> >>
> >> Hans
> > 
> > Ok, then let that table as you have it now.
> > 
>
Pali Rohár June 22, 2024, 3:35 p.m. UTC | #8 
On Friday 21 June 2024 14:24:58 Hans de Goede wrote:
> +static void smo8800_instantiate_i2c_client(struct work_struct *work)
> +{
> +	struct smo8800_device *smo8800 =
> +		container_of(work, struct smo8800_device, i2c_work);
> +	const struct dmi_system_id *lis3lv02d_dmi_id;
> +	struct i2c_board_info info = { };
> +	struct i2c_adapter *adap = NULL;
> +
> +	if (smo8800->i2c_dev)
> +		return;
> +
> +	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
> +	if (!adap)
> +		return;
> +
> +	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);

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

> +	if (!lis3lv02d_dmi_id)
> +		goto out_put_adapter;
> +
> +	info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> +
> +	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
> +	if (IS_ERR(smo8800->i2c_dev)) {
> +		dev_err(smo8800->dev, "error %ld registering %s i2c_client\n",
> +			PTR_ERR(smo8800->i2c_dev), info.type);
> +		smo8800->i2c_dev = NULL;
> +	} else {
> +		dev_dbg(smo8800->dev, "registered %s i2c_client on address 0x%02x\n",
> +			info.type, info.addr);
> +	}
> +
> +out_put_adapter:
> +	i2c_put_adapter(adap);
> +}
Pali Rohár June 22, 2024, 4:26 p.m. UTC | #9 
On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:
> > >> +	{
> > >> +		.matches = {
> > >> +			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> > >> +			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> > >> +		},
> > >> +		.driver_data = (void *)0x29L,
> > > 
> > > At least for me, casting i2c address to LONG and then to pointer looks
> > > very strange. If I look at this code without knowing what the number
> > > 0x29 means I would not figure out that expression "(void *)0x29L" is i2c
> > > address.
> > > 
> > > Is not there a better way to write i2c address? E.g. ".i2c_addr = 0x29"
> > > instead of ".something = (void *)0x29L" to make it readable?
> > 
> > struct dmi_system_id is an existing structure and we cannot just go adding
> > fields to it. driver_data is intended to tie driver specific data to
> > each DMI match, often pointing to some struct, so it is a void *, but
> 
> Yes, I know it.
> 
> > in this case we only need a single integer, so we store that in the
> > pointer. That is is the address becomes obvious when looking at the code
> > which consumes the data.
> 
> Ok, this makes sense. Anyway, is explicit void* cast and L suffix
> required?

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

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

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

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


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

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

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

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

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

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

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

	if (dell_lis3lv02d_i2c_dev)
		return;

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

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

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

	dell_lis3lv02d_i2c_dev = client;

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

static int dell_lis3lv02d_i2c_bus_notify(struct notifier_block *nb,
					 unsigned long action, void *data)
{
	struct device *dev = data;
	struct i2c_client *client;
	struct i2c_adapter *adap;

	switch (action) {
	case BUS_NOTIFY_ADD_DEVICE:
		adap = i2c_verify_adapter(dev);
		if (!adap)
			break;

		if (strstarts(adap->name, "SMBus I801 adapter"))
			queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
		break;
	case BUS_NOTIFY_REMOVED_DEVICE:
		client = i2c_verify_client(dev);
		if (!client)
			break;

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

	return 0;
}

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

static acpi_status __init check_acpi_smo88xx_device(acpi_handle obj_handle,
					     u32 nesting_level,
					     void *context,
					     void **return_value)
{
	struct acpi_device_info *info;
	acpi_status status;
	char *hid;
	int i;

	status = acpi_get_object_info(obj_handle, &info);
	if (ACPI_FAILURE(status))
		return AE_OK;

	if (!(info->valid & ACPI_VALID_HID))
		goto smo88xx_not_found;

	hid = info->hardware_id.string;
	if (!hid)
		goto smo88xx_not_found;

	i = match_string(acpi_smo8800_ids, ARRAY_SIZE(acpi_smo8800_ids), hid);
	if (i < 0)
		goto smo88xx_not_found;

	kfree(info);

	*return_value = NULL;
	return AE_CTRL_TERMINATE;

smo88xx_not_found:
	kfree(info);
	return AE_OK;
}

static bool __init has_acpi_smo88xx_device(void)
{
	void *err = ERR_PTR(-ENOENT);

	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
	return !IS_ERR(err);
}

/*
 * Accelerometer's I2C address is not specified in DMI nor ACPI,
 * so it is needed to define mapping table based on DMI product names.
 */
static const struct dmi_system_id dell_lis3lv02d_devices[] __initconst = {
	/*
	 * Dell platform team told us that these Latitude devices have
	 * ST microelectronics accelerometer at I2C address 0x29.
	 */
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
		},
		.driver_data = (void *)0x29,
	},
	/*
	 * Additional individual entries were added after verification.
	 */
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
		},
		.driver_data = (void *)0x1d,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
		},
		.driver_data = (void *)0x29,
	},
	{
		.matches = {
			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
		},
		.driver_data = (void *)0x29,
	},
	{ }
};
MODULE_DEVICE_TABLE(dmi, dell_lis3lv02d_devices);

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

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

	if (!has_acpi_smo88xx_device())
		return -ENODEV;

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

	dell_lis3lv02d_i2c_addr = (uintptr_t)lis3lv02d_dmi_id->driver_data;

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

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

	return 0;
}

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

module_init(dell_lis3lv02d_init);
module_exit(dell_lis3lv02d_exit);

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

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

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

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

Any opinion about this?
Andy Shevchenko June 22, 2024, 10:36 p.m. UTC | #12 
On Sat, Jun 22, 2024 at 4:26 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 6/22/24 4:20 PM, Pali Rohár wrote:


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

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

In example which I sent in <20240622163518.rfm2wa2kzucy7in4@pali> I used
MODULE_DEVICE_TABLE with those DMI tables.
Andy Shevchenko June 22, 2024, 10:43 p.m. UTC | #14 
On Sat, Jun 22, 2024 at 6:43 PM Pali Rohár <pali@kernel.org> wrote:
> On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:

...

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

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

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

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

> >         }
> >
> > static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> >         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250", 0x29),
> >         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450", 0x29),
> >         ...
> >         { }
> > };
> >
> > Any opinion about this?
> 
> 
> 
> -- 
> With Best Regards,
> Andy Shevchenko
Andy Shevchenko June 22, 2024, 10:53 p.m. UTC | #16 
On Sun, Jun 23, 2024 at 12:50 AM Pali Rohár <pali@kernel.org> wrote:
> On Sunday 23 June 2024 00:43:17 Andy Shevchenko wrote:
> > On Sat, Jun 22, 2024 at 6:43 PM Pali Rohár <pali@kernel.org> wrote:
> > > On Saturday 22 June 2024 16:20:15 Pali Rohár wrote:

...

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

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

>
> > >         }
> > >
> > > static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
> > >         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5250", 0x29),
> > >         DELL_LIS3LV02D_DMI_ENTRY("Latitude E5450", 0x29),
> > >         ...
> > >         { }
> > > };
> > >
> > > Any opinion about this?
Hans de Goede June 23, 2024, 1:45 p.m. UTC | #17 
Hi,

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

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

Regards,

Hans


> 
>> +	if (!lis3lv02d_dmi_id)
>> +		goto out_put_adapter;
>> +
>> +	info.addr = (long)lis3lv02d_dmi_id->driver_data;
>> +	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>> +
>> +	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
>> +	if (IS_ERR(smo8800->i2c_dev)) {
>> +		dev_err(smo8800->dev, "error %ld registering %s i2c_client\n",
>> +			PTR_ERR(smo8800->i2c_dev), info.type);
>> +		smo8800->i2c_dev = NULL;
>> +	} else {
>> +		dev_dbg(smo8800->dev, "registered %s i2c_client on address 0x%02x\n",
>> +			info.type, info.addr);
>> +	}
>> +
>> +out_put_adapter:
>> +	i2c_put_adapter(adap);
>> +}
>
Hans de Goede June 23, 2024, 1:46 p.m. UTC | #18 
Hi,

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

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

Regards,

Hans
Hans de Goede June 23, 2024, 1:56 p.m. UTC | #19 
Hi Pali,

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

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

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

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

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

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

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

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

Regards,

Hans




> #include <linux/acpi.h>
> #include <linux/dmi.h>
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/notifier.h>
> #include <linux/workqueue.h>
> 
> static struct work_struct dell_lis3lv02d_i2c_work;
> static struct i2c_client *dell_lis3lv02d_i2c_dev;
> static unsigned short dell_lis3lv02d_i2c_addr;
> 
> static int dell_lis3lv02d_find_i801(struct device *dev, void *data)
> {
> 	struct i2c_adapter *adap, **adap_ret = data;
> 
> 	adap = i2c_verify_adapter(dev);
> 	if (!adap)
> 		return 0;
> 
> 	if (!strstarts(adap->name, "SMBus I801 adapter"))
> 		return 0;
> 
> 	*adap_ret = i2c_get_adapter(adap->nr);
> 	return 1;
> }
> 
> static void dell_lis3lv02d_instantiate_i2c_client(struct work_struct *work)
> {
> 	struct i2c_board_info info = { };
> 	struct i2c_adapter *adap = NULL;
> 	struct i2c_client *client;
> 
> 	if (dell_lis3lv02d_i2c_dev)
> 		return;
> 
> 	bus_for_each_dev(&i2c_bus_type, NULL, &adap, dell_lis3lv02d_find_i801);
> 	if (!adap)
> 		return;
> 
> 	info.addr = dell_lis3lv02d_i2c_addr;
> 	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
> 
> 	client = i2c_new_client_device(adap, &info);
> 	if (IS_ERR(client)) {
> 		pr_err("error %ld registering %s i2c_client\n",
> 			PTR_ERR(client), info.type);
> 		return;
> 	}
> 
> 	dell_lis3lv02d_i2c_dev = client;
> 
> 	pr_err("registered %s i2c_client on address 0x%02x\n",
> 		info.type, info.addr);
> }
> 
> static int dell_lis3lv02d_i2c_bus_notify(struct notifier_block *nb,
> 					 unsigned long action, void *data)
> {
> 	struct device *dev = data;
> 	struct i2c_client *client;
> 	struct i2c_adapter *adap;
> 
> 	switch (action) {
> 	case BUS_NOTIFY_ADD_DEVICE:
> 		adap = i2c_verify_adapter(dev);
> 		if (!adap)
> 			break;
> 
> 		if (strstarts(adap->name, "SMBus I801 adapter"))
> 			queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
> 		break;
> 	case BUS_NOTIFY_REMOVED_DEVICE:
> 		client = i2c_verify_client(dev);
> 		if (!client)
> 			break;
> 
> 		if (dell_lis3lv02d_i2c_dev == client) {
> 			pr_debug("accelerometer i2c_client removed\n");
> 			dell_lis3lv02d_i2c_dev = NULL;
> 		}
> 		break;
> 	default:
> 		break;
> 	}
> 
> 	return 0;
> }
> 
> // TODO: move this array into dell-smo8800.h header file
> static const char *const acpi_smo8800_ids[] __initconst = {
> 	"SMO8800",
> 	"SMO8801",
> 	"SMO8810",
> 	"SMO8811",
> 	"SMO8820",
> 	"SMO8821",
> 	"SMO8830",
> 	"SMO8831",
> };
> 
> static acpi_status __init check_acpi_smo88xx_device(acpi_handle obj_handle,
> 					     u32 nesting_level,
> 					     void *context,
> 					     void **return_value)
> {
> 	struct acpi_device_info *info;
> 	acpi_status status;
> 	char *hid;
> 	int i;
> 
> 	status = acpi_get_object_info(obj_handle, &info);
> 	if (ACPI_FAILURE(status))
> 		return AE_OK;
> 
> 	if (!(info->valid & ACPI_VALID_HID))
> 		goto smo88xx_not_found;
> 
> 	hid = info->hardware_id.string;
> 	if (!hid)
> 		goto smo88xx_not_found;
> 
> 	i = match_string(acpi_smo8800_ids, ARRAY_SIZE(acpi_smo8800_ids), hid);
> 	if (i < 0)
> 		goto smo88xx_not_found;
> 
> 	kfree(info);
> 
> 	*return_value = NULL;
> 	return AE_CTRL_TERMINATE;
> 
> smo88xx_not_found:
> 	kfree(info);
> 	return AE_OK;
> }
> 
> static bool __init has_acpi_smo88xx_device(void)
> {
> 	void *err = ERR_PTR(-ENOENT);
> 
> 	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
> 	return !IS_ERR(err);
> }
> 
> /*
>  * Accelerometer's I2C address is not specified in DMI nor ACPI,
>  * so it is needed to define mapping table based on DMI product names.
>  */
> static const struct dmi_system_id dell_lis3lv02d_devices[] __initconst = {
> 	/*
> 	 * Dell platform team told us that these Latitude devices have
> 	 * ST microelectronics accelerometer at I2C address 0x29.
> 	 */
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	/*
> 	 * Additional individual entries were added after verification.
> 	 */
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
> 		},
> 		.driver_data = (void *)0x1d,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{
> 		.matches = {
> 			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> 			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
> 		},
> 		.driver_data = (void *)0x29,
> 	},
> 	{ }
> };
> MODULE_DEVICE_TABLE(dmi, dell_lis3lv02d_devices);
> 
> static struct notifier_block dell_lis3lv02d_i2c_nb = {
> 	.notifier_call = dell_lis3lv02d_i2c_bus_notify,
> };
> 
> static int __init dell_lis3lv02d_init(void)
> {
> 	const struct dmi_system_id *lis3lv02d_dmi_id;
> 	int err;
> 
> 	if (!has_acpi_smo88xx_device())
> 		return -ENODEV;
> 
> 	lis3lv02d_dmi_id = dmi_first_match(dell_lis3lv02d_devices);
> 	if (!lis3lv02d_dmi_id)
> 		return -ENODEV;
> 
> 	dell_lis3lv02d_i2c_addr = (uintptr_t)lis3lv02d_dmi_id->driver_data;
> 
> 	err = bus_register_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
> 	if (err)
> 		return err;
> 
> 	INIT_WORK(&dell_lis3lv02d_i2c_work, dell_lis3lv02d_instantiate_i2c_client);
> 	queue_work(system_long_wq, &dell_lis3lv02d_i2c_work);
> 
> 	return 0;
> }
> 
> static void __exit dell_lis3lv02d_exit(void)
> {
> 	bus_unregister_notifier(&i2c_bus_type, &dell_lis3lv02d_i2c_nb);
> 	cancel_work_sync(&dell_lis3lv02d_i2c_work);
> 	if (dell_lis3lv02d_i2c_dev)
> 		i2c_unregister_device(dell_lis3lv02d_i2c_dev);
> }
> 
> module_init(dell_lis3lv02d_init);
> module_exit(dell_lis3lv02d_exit);
> 
> MODULE_LICENSE("GPL");
>
Hans de Goede June 23, 2024, 2 p.m. UTC | #20 
Hi Pali,

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

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

Regards,

Hans
Hans de Goede June 23, 2024, 2:09 p.m. UTC | #21 
Hi,

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

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

static const struct acpi_device_id smo8800_ids[] = {
        { "SMO8800", 0 },
        { "SMO8801", 0 },
        { "SMO8810", 0 },
        { "SMO8811", 0 },
        { "SMO8820", 0 },
        { "SMO8821", 0 },
        { "SMO8830", 0 },
        { "SMO8831", 0 },
        { "", 0 },
};
MODULE_DEVICE_TABLE(acpi, smo8800_ids);

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

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

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

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

Regards,

Hans




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

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

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

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

Patch

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 5ac5bbd60d45..db8d31411148 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1153,127 +1153,6 @@  static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
 	}
 }
 
-/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */
-static const char *const acpi_smo8800_ids[] = {
-	"SMO8800",
-	"SMO8801",
-	"SMO8810",
-	"SMO8811",
-	"SMO8820",
-	"SMO8821",
-	"SMO8830",
-	"SMO8831",
-};
-
-static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle,
-					     u32 nesting_level,
-					     void *context,
-					     void **return_value)
-{
-	struct acpi_device_info *info;
-	acpi_status status;
-	char *hid;
-	int i;
-
-	status = acpi_get_object_info(obj_handle, &info);
-	if (ACPI_FAILURE(status))
-		return AE_OK;
-
-	if (!(info->valid & ACPI_VALID_HID))
-		goto smo88xx_not_found;
-
-	hid = info->hardware_id.string;
-	if (!hid)
-		goto smo88xx_not_found;
-
-	i = match_string(acpi_smo8800_ids, ARRAY_SIZE(acpi_smo8800_ids), hid);
-	if (i < 0)
-		goto smo88xx_not_found;
-
-	kfree(info);
-
-	*return_value = NULL;
-	return AE_CTRL_TERMINATE;
-
-smo88xx_not_found:
-	kfree(info);
-	return AE_OK;
-}
-
-static bool is_dell_system_with_lis3lv02d(void)
-{
-	void *err = ERR_PTR(-ENOENT);
-
-	if (!dmi_match(DMI_SYS_VENDOR, "Dell Inc."))
-		return false;
-
-	/*
-	 * Check that ACPI device SMO88xx is present and is functioning.
-	 * Function acpi_get_devices() already filters all ACPI devices
-	 * which are not present or are not functioning.
-	 * ACPI device SMO88xx represents our ST microelectronics lis3lv02d
-	 * accelerometer but unfortunately ACPI does not provide any other
-	 * information (like I2C address).
-	 */
-	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, &err);
-
-	return !IS_ERR(err);
-}
-
-/*
- * Accelerometer's I2C address is not specified in DMI nor ACPI,
- * so it is needed to define mapping table based on DMI product names.
- */
-static const struct {
-	const char *dmi_product_name;
-	unsigned short i2c_addr;
-} dell_lis3lv02d_devices[] = {
-	/*
-	 * Dell platform team told us that these Latitude devices have
-	 * ST microelectronics accelerometer at I2C address 0x29.
-	 */
-	{ "Latitude E5250",     0x29 },
-	{ "Latitude E5450",     0x29 },
-	{ "Latitude E5550",     0x29 },
-	{ "Latitude E6440",     0x29 },
-	{ "Latitude E6440 ATG", 0x29 },
-	{ "Latitude E6540",     0x29 },
-	/*
-	 * Additional individual entries were added after verification.
-	 */
-	{ "Latitude 5480",      0x29 },
-	{ "Precision 3540",     0x29 },
-	{ "Vostro V131",        0x1d },
-	{ "Vostro 5568",        0x29 },
-	{ "XPS 15 7590",        0x29 },
-};
-
-static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
-{
-	struct i2c_board_info info;
-	const char *dmi_product_name;
-	int i;
-
-	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
-	for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) {
-		if (strcmp(dmi_product_name,
-			   dell_lis3lv02d_devices[i].dmi_product_name) == 0)
-			break;
-	}
-
-	if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) {
-		dev_warn(&priv->pci_dev->dev,
-			 "Accelerometer lis3lv02d is present on SMBus but its"
-			 " address is unknown, skipping registration\n");
-		return;
-	}
-
-	memset(&info, 0, sizeof(struct i2c_board_info));
-	info.addr = dell_lis3lv02d_devices[i].i2c_addr;
-	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
-	i2c_new_client_device(&priv->adapter, &info);
-}
-
 /* Register optional slaves */
 static void i801_probe_optional_slaves(struct i801_priv *priv)
 {
@@ -1293,9 +1172,6 @@  static void i801_probe_optional_slaves(struct i801_priv *priv)
 	if (dmi_name_in_vendors("FUJITSU"))
 		dmi_walk(dmi_check_onboard_devices, &priv->adapter);
 
-	if (is_dell_system_with_lis3lv02d())
-		register_dell_lis3lv02d_i2c_device(priv);
-
 	/* Instantiate SPD EEPROMs unless the SMBus is multiplexed */
 #ifdef CONFIG_I2C_I801_MUX
 	if (!priv->mux_pdev)
diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index f7ec17c56833..cd2e48405859 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -4,20 +4,26 @@ 
  *
  *  Copyright (C) 2012 Sonal Santan <sonal.santan@gmail.com>
  *  Copyright (C) 2014 Pali Rohár <pali@kernel.org>
+ *  Copyright (C) 2023 Hans de Goede <hansg@kernel.org>
  *
  *  This is loosely based on lis3lv02d driver.
  */
 
 #define DRIVER_NAME "smo8800"
 
+#include <linux/device/bus.h>
+#include <linux/dmi.h>
 #include <linux/fs.h>
+#include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/miscdevice.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
+#include <linux/notifier.h>
 #include <linux/platform_device.h>
 #include <linux/uaccess.h>
+#include <linux/workqueue.h>
 
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
@@ -25,6 +31,9 @@  struct smo8800_device {
 	struct miscdevice miscdev;   /* for /dev/freefall */
 	unsigned long misc_opened;   /* whether the device is open */
 	wait_queue_head_t misc_wait; /* Wait queue for the misc dev */
+	struct notifier_block i2c_nb;/* i2c bus notifier */
+	struct work_struct i2c_work; /* Work for instantiating lis3lv02d i2c_client */
+	struct i2c_client *i2c_dev;  /* i2c_client for lis3lv02d */
 	struct device *dev;          /* acpi device */
 };
 
@@ -103,6 +112,184 @@  static const struct file_operations smo8800_misc_fops = {
 	.release = smo8800_misc_release,
 };
 
+/*
+ * Accelerometer's I2C address is not specified in DMI nor ACPI,
+ * so it is needed to define mapping table based on DMI product names.
+ */
+static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
+	/*
+	 * Dell platform team told us that these Latitude devices have
+	 * ST microelectronics accelerometer at I2C address 0x29.
+	 */
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5250"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5450"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E5550"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6440 ATG"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude E6540"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	/*
+	 * Additional individual entries were added after verification.
+	 */
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Latitude 5480"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Precision 3540"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro V131"),
+		},
+		.driver_data = (void *)0x1dL,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Vostro 5568"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "XPS 15 7590"),
+		},
+		.driver_data = (void *)0x29L,
+	},
+	{ }
+};
+
+static int smo8800_find_i801(struct device *dev, void *data)
+{
+	struct i2c_adapter *adap, **adap_ret = data;
+
+	adap = i2c_verify_adapter(dev);
+	if (!adap)
+		return 0;
+
+	if (!strstarts(adap->name, "SMBus I801 adapter"))
+		return 0;
+
+	*adap_ret = i2c_get_adapter(adap->nr);
+	return 1;
+}
+
+static void smo8800_instantiate_i2c_client(struct work_struct *work)
+{
+	struct smo8800_device *smo8800 =
+		container_of(work, struct smo8800_device, i2c_work);
+	const struct dmi_system_id *lis3lv02d_dmi_id;
+	struct i2c_board_info info = { };
+	struct i2c_adapter *adap = NULL;
+
+	if (smo8800->i2c_dev)
+		return;
+
+	bus_for_each_dev(&i2c_bus_type, NULL, &adap, smo8800_find_i801);
+	if (!adap)
+		return;
+
+	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);
+	if (!lis3lv02d_dmi_id)
+		goto out_put_adapter;
+
+	info.addr = (long)lis3lv02d_dmi_id->driver_data;
+	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+
+	smo8800->i2c_dev = i2c_new_client_device(adap, &info);
+	if (IS_ERR(smo8800->i2c_dev)) {
+		dev_err(smo8800->dev, "error %ld registering %s i2c_client\n",
+			PTR_ERR(smo8800->i2c_dev), info.type);
+		smo8800->i2c_dev = NULL;
+	} else {
+		dev_dbg(smo8800->dev, "registered %s i2c_client on address 0x%02x\n",
+			info.type, info.addr);
+	}
+
+out_put_adapter:
+	i2c_put_adapter(adap);
+}
+
+static int smo8800_i2c_bus_notify(struct notifier_block *nb,
+				  unsigned long action, void *data)
+{
+	struct smo8800_device *smo8800 =
+		container_of(nb, struct smo8800_device, i2c_nb);
+	struct device *dev = data;
+	struct i2c_client *client;
+	struct i2c_adapter *adap;
+
+	switch (action) {
+	case BUS_NOTIFY_ADD_DEVICE:
+		adap = i2c_verify_adapter(dev);
+		if (!adap)
+			break;
+
+		if (strstarts(adap->name, "SMBus I801 adapter"))
+			queue_work(system_long_wq, &smo8800->i2c_work);
+		break;
+	case BUS_NOTIFY_REMOVED_DEVICE:
+		client = i2c_verify_client(dev);
+		if (!client)
+			break;
+
+		if (smo8800->i2c_dev == client) {
+			dev_dbg(smo8800->dev, "accelerometer i2c_client removed\n");
+			smo8800->i2c_dev = NULL;
+		}
+		break;
+	default:
+		break;
+	}
+
+	return 0;
+}
+
 static int smo8800_probe(struct platform_device *device)
 {
 	int err;
@@ -118,8 +305,10 @@  static int smo8800_probe(struct platform_device *device)
 	smo8800->miscdev.minor = MISC_DYNAMIC_MINOR;
 	smo8800->miscdev.name = "freefall";
 	smo8800->miscdev.fops = &smo8800_misc_fops;
+	smo8800->i2c_nb.notifier_call = smo8800_i2c_bus_notify;
 
 	init_waitqueue_head(&smo8800->misc_wait);
+	INIT_WORK(&smo8800->i2c_work, smo8800_instantiate_i2c_client);
 
 	err = misc_register(&smo8800->miscdev);
 	if (err) {
@@ -147,8 +336,26 @@  static int smo8800_probe(struct platform_device *device)
 
 	dev_dbg(&device->dev, "device /dev/freefall registered with IRQ %d\n",
 		 smo8800->irq);
+
+	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
+		/*
+		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
+		 * i2c_client instantiation.
+		 */
+		err = bus_register_notifier(&i2c_bus_type, &smo8800->i2c_nb);
+		if (err)
+			goto error_free_irq;
+
+		queue_work(system_long_wq, &smo8800->i2c_work);
+	} else {
+		dev_warn(&device->dev,
+			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
+	}
+
 	return 0;
 
+error_free_irq:
+	free_irq(smo8800->irq, smo8800);
 error:
 	misc_deregister(&smo8800->miscdev);
 	return err;
@@ -158,12 +365,17 @@  static void smo8800_remove(struct platform_device *device)
 {
 	struct smo8800_device *smo8800 = platform_get_drvdata(device);
 
+	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
+		bus_unregister_notifier(&i2c_bus_type, &smo8800->i2c_nb);
+		cancel_work_sync(&smo8800->i2c_work);
+		i2c_unregister_device(smo8800->i2c_dev);
+	}
+
 	free_irq(smo8800->irq, smo8800);
 	misc_deregister(&smo8800->miscdev);
 	dev_dbg(&device->dev, "device /dev/freefall unregistered\n");
 }
 
-/* NOTE: Keep this list in sync with drivers/i2c/busses/i2c-i801.c */
 static const struct acpi_device_id smo8800_ids[] = {
 	{ "SMO8800", 0 },
 	{ "SMO8801", 0 },