diff mbox series

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

Message ID 20240624111519.15652-7-hdegoede@redhat.com
State Superseded
Headers show
Series i2c-i801 / dell-lis3lv02d: Move instantiation of lis3lv02d i2c_client from i2c-i801 to dell-lis3lv02d | expand

Commit Message

Hans de Goede June 24, 2024, 11:15 a.m. UTC
Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
of the accelerometer. So a DMI product-name to address mapping table
is used.

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

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

Link: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/dell/dell-lis3lv02d.c | 133 ++++++++++++++++++++-
 1 file changed, 131 insertions(+), 2 deletions(-)

Comments

Pali Rohár June 24, 2024, 6:21 p.m. UTC | #1
On Monday 24 June 2024 13:15:18 Hans de Goede wrote:
> Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
> of the accelerometer. So a DMI product-name to address mapping table
> is used.
> 
> At support to have the kernel probe for the i2c-address for modesl
> which are not on the list.
> 
> The new probing code sits behind a new probe_i2c_addr module parameter,
> which is disabled by default because probing might be dangerous.
> 
> Link: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

> ---
>  drivers/platform/x86/dell/dell-lis3lv02d.c | 133 ++++++++++++++++++++-
>  1 file changed, 131 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
> index a7409db0505b..173615fd2646 100644
> --- a/drivers/platform/x86/dell/dell-lis3lv02d.c
> +++ b/drivers/platform/x86/dell/dell-lis3lv02d.c
> @@ -15,6 +15,8 @@
>  #include <linux/workqueue.h>
>  #include "dell-smo8800-ids.h"
>  
> +#define LIS3_WHO_AM_I 0x0f
> +
>  #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr)                 \
>  	{                                                                \
>  		.matches = {                                             \
> @@ -57,6 +59,121 @@ static const struct dmi_system_id *lis3lv02d_dmi_id;
>  static struct i2c_client *i2c_dev;
>  static bool notifier_registered;
>  
> +static bool probe_i2c_addr;
> +module_param(probe_i2c_addr, bool, 0444);
> +MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown");
> +
> +/*
> + * This is the kernel version of the single register device sanity checks from
> + * the i2c_safety_check function from lm_sensors sensor-detect script:
> + * This is meant to prevent access to 1-register-only devices,
> + * which are designed to be accessed with SMBus receive byte and SMBus send
> + * byte transactions (i.e. short reads and short writes) and treat SMBus
> + * read byte as a real write followed by a read. The device detection
> + * routines would write random values to the chip with possibly very nasty
> + * results for the hardware. Note that this function won't catch all such
> + * chips, as it assumes that reads and writes relate to the same register,
> + * but that's the best we can do.
> + */
> +static int i2c_safety_check(struct i2c_adapter *adap, u8 addr)
> +{
> +	union i2c_smbus_data smbus_data;
> +	int err;
> +	u8 data;
> +
> +	/*
> +	 * First receive a byte from the chip, and remember it. This
> +	 * also checks if there is a device at the address at all.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +			     I2C_SMBUS_BYTE, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	data = smbus_data.byte;
> +
> +	/*
> +	 * Receive a byte again; very likely to be the same for
> +	 * 1-register-only devices.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +			     I2C_SMBUS_BYTE, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != data)
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Then try a standard byte read, with a register offset equal to
> +	 * the read byte; for 1-register-only device this should read
> +	 * the same byte value in return.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != data)
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Then try a standard byte read, with a slightly different register
> +	 * offset; this should again read the register offset in return.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != (data ^ 0x01))
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Apparently this is a 1-register-only device, restore the original
> +	 * register value and leave it alone.
> +	 */
> +	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
> +		       I2C_SMBUS_BYTE, NULL);
> +	pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr);
> +	return -ENODEV;
> +}
> +
> +static int detect_lis3lv02d(struct i2c_adapter *adap, u8 addr,
> +			    struct i2c_board_info *info)
> +{
> +	union i2c_smbus_data smbus_data;
> +	int err;
> +
> +	pr_info("Probing for lis3lv02d on address 0x%02x\n", addr);
> +	err = i2c_safety_check(adap, addr);
> +	if (err < 0)
> +		return err;
> +
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0) {
> +		pr_warn("Failed to read who-am-i register: %d\n", err);
> +		return err;
> +	}
> +
> +	/* valid who-am-i values are from drivers/misc/lis3lv02d/lis3lv02d.c */
> +	switch (smbus_data.byte) {
> +	case 0x32:
> +	case 0x33:
> +	case 0x3a:
> +	case 0x3b:
> +		break;
> +	default:
> +		pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte);
> +		return -ENODEV;
> +	}
> +
> +	pr_debug("Detected lis3lv02d on address 0x%02x\n", addr);
> +	info->addr = addr;
> +	return 0;
> +}
> +
>  static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
>  {
>  	/*
> @@ -93,7 +210,17 @@ static void instantiate_i2c_client(struct work_struct *work)
>  	if (!adap)
>  		return;
>  
> -	info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +	if (lis3lv02d_dmi_id) {
> +		info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +	} else {
> +		/* First try address 0x29 (most used) and then try 0x1d */
> +		if (detect_lis3lv02d(adap, 0x29, &info) != 0 &&
> +		    detect_lis3lv02d(adap, 0x1d, &info) != 0) {
> +			pr_warn("failed to probe for lis3lv02d I2C address\n");
> +			goto out_put_adapter;
> +		}
> +	}
> +
>  	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>  
>  	i2c_dev = i2c_new_client_device(adap, &info);
> @@ -104,6 +231,7 @@ static void instantiate_i2c_client(struct work_struct *work)
>  		pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr);
>  	}
>  
> +out_put_adapter:
>  	i2c_put_adapter(adap);
>  }
>  static DECLARE_WORK(i2c_work, instantiate_i2c_client);
> @@ -166,8 +294,9 @@ static int __init dell_lis3lv02d_init(void)
>  	put_device(dev);
>  
>  	lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices);
> -	if (!lis3lv02d_dmi_id) {
> +	if (!lis3lv02d_dmi_id && !probe_i2c_addr) {
>  		pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> +		pr_info("Pass dell_lis3lv02d.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
>  		return 0;
>  	}
>  
> -- 
> 2.45.1
>
kernel test robot June 28, 2024, 1:42 a.m. UTC | #2
Hi Hans,

kernel test robot noticed the following build errors:

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

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

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

All errors (new ones prefixed by >>):

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


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

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

On 6/24/24 8:21 PM, Pali Rohár wrote:
> On Monday 24 June 2024 13:15:18 Hans de Goede wrote:
>> Unfortunately the SMOxxxx ACPI device does not contain the i2c-address
>> of the accelerometer. So a DMI product-name to address mapping table
>> is used.
>>
>> At support to have the kernel probe for the i2c-address for modesl
>> which are not on the list.
>>
>> The new probing code sits behind a new probe_i2c_addr module parameter,
>> which is disabled by default because probing might be dangerous.
>>
>> Link: https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> My comments from the previous version still apply there. There is no
> dangerous warning neither in parameter name and its description, there
> is no warning once the parameter was specified.

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

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

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

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

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

Regards,

Hans



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

Patch

diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
index a7409db0505b..173615fd2646 100644
--- a/drivers/platform/x86/dell/dell-lis3lv02d.c
+++ b/drivers/platform/x86/dell/dell-lis3lv02d.c
@@ -15,6 +15,8 @@ 
 #include <linux/workqueue.h>
 #include "dell-smo8800-ids.h"
 
+#define LIS3_WHO_AM_I 0x0f
+
 #define DELL_LIS3LV02D_DMI_ENTRY(product_name, i2c_addr)                 \
 	{                                                                \
 		.matches = {                                             \
@@ -57,6 +59,121 @@  static const struct dmi_system_id *lis3lv02d_dmi_id;
 static struct i2c_client *i2c_dev;
 static bool notifier_registered;
 
+static bool probe_i2c_addr;
+module_param(probe_i2c_addr, bool, 0444);
+MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown");
+
+/*
+ * This is the kernel version of the single register device sanity checks from
+ * the i2c_safety_check function from lm_sensors sensor-detect script:
+ * This is meant to prevent access to 1-register-only devices,
+ * which are designed to be accessed with SMBus receive byte and SMBus send
+ * byte transactions (i.e. short reads and short writes) and treat SMBus
+ * read byte as a real write followed by a read. The device detection
+ * routines would write random values to the chip with possibly very nasty
+ * results for the hardware. Note that this function won't catch all such
+ * chips, as it assumes that reads and writes relate to the same register,
+ * but that's the best we can do.
+ */
+static int i2c_safety_check(struct i2c_adapter *adap, u8 addr)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+	u8 data;
+
+	/*
+	 * First receive a byte from the chip, and remember it. This
+	 * also checks if there is a device at the address at all.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	data = smbus_data.byte;
+
+	/*
+	 * Receive a byte again; very likely to be the same for
+	 * 1-register-only devices.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a register offset equal to
+	 * the read byte; for 1-register-only device this should read
+	 * the same byte value in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a slightly different register
+	 * offset; this should again read the register offset in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != (data ^ 0x01))
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Apparently this is a 1-register-only device, restore the original
+	 * register value and leave it alone.
+	 */
+	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
+		       I2C_SMBUS_BYTE, NULL);
+	pr_warn("I2C safety check for address 0x%02x failed, skipping\n", addr);
+	return -ENODEV;
+}
+
+static int detect_lis3lv02d(struct i2c_adapter *adap, u8 addr,
+			    struct i2c_board_info *info)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+
+	pr_info("Probing for lis3lv02d on address 0x%02x\n", addr);
+	err = i2c_safety_check(adap, addr);
+	if (err < 0)
+		return err;
+
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0) {
+		pr_warn("Failed to read who-am-i register: %d\n", err);
+		return err;
+	}
+
+	/* valid who-am-i values are from drivers/misc/lis3lv02d/lis3lv02d.c */
+	switch (smbus_data.byte) {
+	case 0x32:
+	case 0x33:
+	case 0x3a:
+	case 0x3b:
+		break;
+	default:
+		pr_warn("Unknown who-am-i register value 0x%02x\n", smbus_data.byte);
+		return -ENODEV;
+	}
+
+	pr_debug("Detected lis3lv02d on address 0x%02x\n", addr);
+	info->addr = addr;
+	return 0;
+}
+
 static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
 {
 	/*
@@ -93,7 +210,17 @@  static void instantiate_i2c_client(struct work_struct *work)
 	if (!adap)
 		return;
 
-	info.addr = (long)lis3lv02d_dmi_id->driver_data;
+	if (lis3lv02d_dmi_id) {
+		info.addr = (long)lis3lv02d_dmi_id->driver_data;
+	} else {
+		/* First try address 0x29 (most used) and then try 0x1d */
+		if (detect_lis3lv02d(adap, 0x29, &info) != 0 &&
+		    detect_lis3lv02d(adap, 0x1d, &info) != 0) {
+			pr_warn("failed to probe for lis3lv02d I2C address\n");
+			goto out_put_adapter;
+		}
+	}
+
 	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
 
 	i2c_dev = i2c_new_client_device(adap, &info);
@@ -104,6 +231,7 @@  static void instantiate_i2c_client(struct work_struct *work)
 		pr_debug("registered lis3lv02d on address 0x%02x\n", info.addr);
 	}
 
+out_put_adapter:
 	i2c_put_adapter(adap);
 }
 static DECLARE_WORK(i2c_work, instantiate_i2c_client);
@@ -166,8 +294,9 @@  static int __init dell_lis3lv02d_init(void)
 	put_device(dev);
 
 	lis3lv02d_dmi_id = dmi_first_match(lis3lv02d_devices);
-	if (!lis3lv02d_dmi_id) {
+	if (!lis3lv02d_dmi_id && !probe_i2c_addr) {
 		pr_warn("accelerometer is present on SMBus but its address is unknown, skipping registration\n");
+		pr_info("Pass dell_lis3lv02d.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
 		return 0;
 	}