diff mbox series

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

Message ID 20240704125643.22946-7-hdegoede@redhat.com (mailing list archive)
State Superseded, archived
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 July 4, 2024, 12:56 p.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>
---
Changes in v6:
- Use i2c_new_scanned_device() instead of re-inventing it

Changes in v5:
- Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr)
---
 drivers/platform/x86/dell/dell-lis3lv02d.c | 52 ++++++++++++++++++++--
 1 file changed, 48 insertions(+), 4 deletions(-)

Comments

Pali Rohár July 4, 2024, 9:37 p.m. UTC | #1
On Thursday 04 July 2024 14:56:43 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>

NAK.

This is a hack, which should be avoided as specified in previous
discussions (e.g. it can cause regression for future or also existing
products).

Author refused to improve the code, also refused to ask vendor about the
details and proper implementation and author also refused to do any
future discussion about it.

Based on this state, this patch 6/6 should not be merged at all.

> ---
> Changes in v6:
> - Use i2c_new_scanned_device() instead of re-inventing it
> 
> Changes in v5:
> - Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr)
> ---
>  drivers/platform/x86/dell/dell-lis3lv02d.c | 52 ++++++++++++++++++++--
>  1 file changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
> index ab02ad93758a..21390e6302a0 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,38 @@ static u8 i2c_addr;
>  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 may be dangerous.");
> +
> +static int detect_lis3lv02d(struct i2c_adapter *adap, unsigned short addr)
> +{
> +	union i2c_smbus_data smbus_data;
> +	int err;
> +
> +	pr_info("Probing for lis3lv02d on address 0x%02x\n", addr);
> +
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return 0; /* Not found */
> +
> +	/* 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 0; /* Not found */
> +	}
> +
> +	pr_debug("Detected lis3lv02d on address 0x%02x\n", addr);
> +	return 1; /* Found */
> +}
> +
>  static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
>  {
>  	/*
> @@ -93,10 +127,18 @@ static void instantiate_i2c_client(struct work_struct *work)
>  	if (!adap)
>  		return;
>  
> -	info.addr = i2c_addr;
>  	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>  
> -	i2c_dev = i2c_new_client_device(adap, &info);
> +	if (i2c_addr) {
> +		info.addr = i2c_addr;
> +		i2c_dev = i2c_new_client_device(adap, &info);
> +	} else {
> +		/* First try address 0x29 (most used) and then try 0x1d */
> +		static const unsigned short addr_list[] = { 0x29, 0x1d, I2C_CLIENT_END };
> +
> +		i2c_dev = i2c_new_scanned_device(adap, &info, addr_list, detect_lis3lv02d);
> +	}
> +
>  	if (IS_ERR(i2c_dev)) {
>  		pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev));
>  		i2c_dev = NULL;
> @@ -167,12 +209,14 @@ 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;
>  	}
>  
> -	i2c_addr = (long)lis3lv02d_dmi_id->driver_data;
> +	if (lis3lv02d_dmi_id)
> +		i2c_addr = (long)lis3lv02d_dmi_id->driver_data;
>  
>  	/*
>  	 * Register i2c-bus notifier + queue initial scan for lis3lv02d
> -- 
> 2.45.1
>
Hans de Goede July 5, 2024, 7:10 a.m. UTC | #2
Hi,

On 7/4/24 11:37 PM, Pali Rohár wrote:
> On Thursday 04 July 2024 14:56:43 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>
> 
> NAK.
> 
> This is a hack

This is not a hack, notice the existing i2c_new_scanned_device() i2c-core
function exists for a reason. As I have tried to explain before scanning
for i2c-devices if we don't know the address is something which the kernel
has been doing for a long time already.

Current kernels scan for i2c devices on pretty much any Intel PC in the form
of i2c_register_spd() running.

>, which should be avoided as specified in previous
> discussions (e.g. it can cause regression for future or also existing
> products).

You have provided 0 proof or even any hypothesis / speculations how this can
cause regressions. Al you have done is said this may cause regressions
without providing any details as to how you believe this would cause
regressions please provide details.

Also the new code is only activated if a new module option is st. By default
this options is not set, so this cannot cause any regressions since it
does not change anything for end users unless they explicitly enable it.

You have made it plenty clear that you don't like this approach, but
claiming it can cause regressions when by default it does not do anything
is just dishonest.

> Author refused to improve the code,

Really? I have gone out of my way to please you, I've moved all of the i2c
handling to a separate file because you asked for this, adding a text to
both the kernel message informing users about the module-parameter to
probe and the module-param help text that this may be dangerous.

Also after I last discussion I moved to i2c_new_scanned_device() instead
of DIY code. There is a reason why this patch-set is at v6 and it is not
because I'm refusing to improve it.

> also refused to ask vendor about the
> details and proper implementation and author also refused to do any
> future discussion about it.

As I have explained countless times I have no contacts inside Dell
to ask about this. If e.g. Mario was still at Dell I would have asked
Mario about this immediately back when the discussion started.

Besides the Dell.Client.Kernel@dell.com which no-one seems to be
reading, there is only one other @dell.com address in all of MAINTAINERS:
Prasanth Ksr <prasanth.ksr@dell.com>

To put an end to this whole discussion about contacting Dell I'll email
them with you (Pali) in the Cc. I don't expect much to come from this
but we will see.

> Based on this state, this patch 6/6 should not be merged at all.

Lets move forward with merging patches 1-5 and wait to see if we get
any response from Dell. But I do very much want to move forward
with this if contacting Dell does not result in another solution
to allow users to easily find out what the i2c-address is.

Regards,

Hans


> 
>> ---
>> Changes in v6:
>> - Use i2c_new_scanned_device() instead of re-inventing it
>>
>> Changes in v5:
>> - Add "this may be dangerous warning" to MODULE_PARM_DESC(probe_i2c_addr)
>> ---
>>  drivers/platform/x86/dell/dell-lis3lv02d.c | 52 ++++++++++++++++++++--
>>  1 file changed, 48 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell/dell-lis3lv02d.c b/drivers/platform/x86/dell/dell-lis3lv02d.c
>> index ab02ad93758a..21390e6302a0 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,38 @@ static u8 i2c_addr;
>>  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 may be dangerous.");
>> +
>> +static int detect_lis3lv02d(struct i2c_adapter *adap, unsigned short addr)
>> +{
>> +	union i2c_smbus_data smbus_data;
>> +	int err;
>> +
>> +	pr_info("Probing for lis3lv02d on address 0x%02x\n", addr);
>> +
>> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
>> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
>> +	if (err < 0)
>> +		return 0; /* Not found */
>> +
>> +	/* 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 0; /* Not found */
>> +	}
>> +
>> +	pr_debug("Detected lis3lv02d on address 0x%02x\n", addr);
>> +	return 1; /* Found */
>> +}
>> +
>>  static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
>>  {
>>  	/*
>> @@ -93,10 +127,18 @@ static void instantiate_i2c_client(struct work_struct *work)
>>  	if (!adap)
>>  		return;
>>  
>> -	info.addr = i2c_addr;
>>  	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
>>  
>> -	i2c_dev = i2c_new_client_device(adap, &info);
>> +	if (i2c_addr) {
>> +		info.addr = i2c_addr;
>> +		i2c_dev = i2c_new_client_device(adap, &info);
>> +	} else {
>> +		/* First try address 0x29 (most used) and then try 0x1d */
>> +		static const unsigned short addr_list[] = { 0x29, 0x1d, I2C_CLIENT_END };
>> +
>> +		i2c_dev = i2c_new_scanned_device(adap, &info, addr_list, detect_lis3lv02d);
>> +	}
>> +
>>  	if (IS_ERR(i2c_dev)) {
>>  		pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev));
>>  		i2c_dev = NULL;
>> @@ -167,12 +209,14 @@ 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;
>>  	}
>>  
>> -	i2c_addr = (long)lis3lv02d_dmi_id->driver_data;
>> +	if (lis3lv02d_dmi_id)
>> +		i2c_addr = (long)lis3lv02d_dmi_id->driver_data;
>>  
>>  	/*
>>  	 * Register i2c-bus notifier + queue initial scan for lis3lv02d
>> -- 
>> 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 ab02ad93758a..21390e6302a0 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,38 @@  static u8 i2c_addr;
 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 may be dangerous.");
+
+static int detect_lis3lv02d(struct i2c_adapter *adap, unsigned short addr)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+
+	pr_info("Probing for lis3lv02d on address 0x%02x\n", addr);
+
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return 0; /* Not found */
+
+	/* 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 0; /* Not found */
+	}
+
+	pr_debug("Detected lis3lv02d on address 0x%02x\n", addr);
+	return 1; /* Found */
+}
+
 static bool i2c_adapter_is_main_i801(struct i2c_adapter *adap)
 {
 	/*
@@ -93,10 +127,18 @@  static void instantiate_i2c_client(struct work_struct *work)
 	if (!adap)
 		return;
 
-	info.addr = i2c_addr;
 	strscpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
 
-	i2c_dev = i2c_new_client_device(adap, &info);
+	if (i2c_addr) {
+		info.addr = i2c_addr;
+		i2c_dev = i2c_new_client_device(adap, &info);
+	} else {
+		/* First try address 0x29 (most used) and then try 0x1d */
+		static const unsigned short addr_list[] = { 0x29, 0x1d, I2C_CLIENT_END };
+
+		i2c_dev = i2c_new_scanned_device(adap, &info, addr_list, detect_lis3lv02d);
+	}
+
 	if (IS_ERR(i2c_dev)) {
 		pr_err("error %ld registering i2c_client\n", PTR_ERR(i2c_dev));
 		i2c_dev = NULL;
@@ -167,12 +209,14 @@  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;
 	}
 
-	i2c_addr = (long)lis3lv02d_dmi_id->driver_data;
+	if (lis3lv02d_dmi_id)
+		i2c_addr = (long)lis3lv02d_dmi_id->driver_data;
 
 	/*
 	 * Register i2c-bus notifier + queue initial scan for lis3lv02d