diff mbox series

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

Message ID 20240621122503.10034-7-hdegoede@redhat.com
State Superseded
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:25 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.

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

Comments

Andy Shevchenko June 21, 2024, 3:37 p.m. UTC | #1
On Fri, Jun 21, 2024 at 2:26 PM Hans de Goede <hdegoede@redhat.com> 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

At --> Add ?

models

> which are not on the list.
>
> The new probing code sits behind a new probe_i2c_addr module parameter,
> which is disabled by default because probing might be dangerous.

...

> +/*
> + * This is the kernel version of the single register device sanity checks from
> + * the i2c_safety_check function from lm_sensors sensor-detect script:

i2c_safety_check()

> + * 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.
> + */

...

> +       err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,

Is 0x01 defined already?
Or BIT(0) ?

> +                            I2C_SMBUS_BYTE_DATA, &smbus_data);
> +       if (err < 0)
> +               return err;
> +
> +       if (smbus_data.byte != (data ^ 0x01))

Ditto.

> +               return 0; /* Not a 1-register-only device. */

...

> +               dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");

command line
...or...
cmdline
Pali Rohár June 22, 2024, 1:32 p.m. UTC | #2
On Friday 21 June 2024 14:25:01 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.

This is statement which I got from Dell for 10 years old Dell models.

I have already stated that poking of address in kernel is a big risk
specially for all current and any future dell hardware. Hiding it just
under the kernel parameter is still a risk, specially as neither its
name, nor description say that it is dangerous.

But anyway, why this code is being introduced? Have you communicated
with Dell about this problem?

> At support to have the kernel probe for the i2c-address for modesl
> which are not on the list.
> 
> The new probing code sits behind a new probe_i2c_addr module parameter,
> which is disabled by default because probing might be dangerous.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/dell/dell-smo8800.c | 152 ++++++++++++++++++++++-
>  1 file changed, 147 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
> index 4c79b2599d96..d64d200e927a 100644
> --- a/drivers/platform/x86/dell/dell-smo8800.c
> +++ b/drivers/platform/x86/dell/dell-smo8800.c
> @@ -10,6 +10,7 @@
>   */
>  
>  #define DRIVER_NAME "smo8800"
> +#define LIS3_WHO_AM_I 0x0f
>  
>  #include <linux/device/bus.h>
>  #include <linux/dmi.h>
> @@ -25,6 +26,10 @@
>  #include <linux/uaccess.h>
>  #include <linux/workqueue.h>
>  
> +static bool probe_i2c_addr;
> +module_param(probe_i2c_addr, bool, 0444);
> +MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown");
> +
>  struct smo8800_device {
>  	u32 irq;                     /* acpi device irq */
>  	atomic_t counter;            /* count after last read */
> @@ -225,6 +230,130 @@ static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
>  	{ }
>  };
>  
> +/*
> + * 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 device *dev, struct i2c_adapter *adap, u8 addr)
> +{
> +	union i2c_smbus_data smbus_data;
> +	int err;
> +	u8 data;
> +
> +	/*
> +	 * First receive a byte from the chip, and remember it. This
> +	 * also checks if there is a device at the address at all.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +			     I2C_SMBUS_BYTE, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	data = smbus_data.byte;
> +
> +	/*
> +	 * Receive a byte again; very likely to be the same for
> +	 * 1-register-only devices.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
> +			     I2C_SMBUS_BYTE, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != data)
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Then try a standard byte read, with a register offset equal to
> +	 * the read byte; for 1-register-only device this should read
> +	 * the same byte value in return.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != data)
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Then try a standard byte read, with a slightly different register
> +	 * offset; this should again read the register offset in return.
> +	 */
> +	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
> +			     I2C_SMBUS_BYTE_DATA, &smbus_data);
> +	if (err < 0)
> +		return err;
> +
> +	if (smbus_data.byte != (data ^ 0x01))
> +		return 0; /* Not a 1-register-only device. */
> +
> +	/*
> +	 * Apparently this is a 1-register-only device, restore the original
> +	 * register value and leave it alone.
> +	 */
> +	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
> +		       I2C_SMBUS_BYTE, NULL);
> +	dev_warn(dev, "I2C safety check for address 0x%02x failed, skipping\n", addr);
> +	return -ENODEV;
> +}
> +
> +static int smo8800_detect_accel(struct smo8800_device *smo8800,
> +				struct i2c_adapter *adap, u8 addr,
> +				struct i2c_board_info *info, bool probe)
> +{
> +	union i2c_smbus_data smbus_data;
> +	const char *type;
> +	int err;
> +
> +	if (probe) {
> +		dev_info(smo8800->dev, "Probing for accelerometer on address 0x%02x\n", addr);
> +		err = i2c_safety_check(smo8800->dev, 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) {
> +		dev_warn(smo8800->dev, "Failed to read who-am-i register: %d\n", err);
> +		return err;
> +	}
> +
> +	/* who-am-i register mappings from drivers/misc/lis3lv02d/lis3lv02d.c */
> +	switch (smbus_data.byte) {
> +	case 0x32:
> +		type = "lis331dlh";
> +		break;
> +	case 0x33:
> +		type = "lis2de12"; /* LIS3DC / HP3DC in drivers/misc/lis3lv02d/lis3lv02d.c */
> +		break;
> +	case 0x3a:
> +		type = "lis3lv02dl_accel";
> +		break;
> +	case 0x3b:
> +		type = "lis302dl";
> +		break;
> +	default:
> +		dev_warn(smo8800->dev, "Unknown who-am-i register value 0x%02x\n",
> +			 smbus_data.byte);
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(smo8800->dev, "Detected %s accelerometer on address 0x%02x\n", type, addr);
> +	strscpy(info->type, "lis3lv02d", I2C_NAME_SIZE);
> +	info->addr = addr;
> +	return 0;
> +}
> +
>  static int smo8800_find_i801(struct device *dev, void *data)
>  {
>  	struct i2c_adapter *adap, **adap_ret = data;
> @@ -247,6 +376,7 @@ static void smo8800_instantiate_i2c_client(struct work_struct *work)
>  	const struct dmi_system_id *lis3lv02d_dmi_id;
>  	struct i2c_board_info info = { };
>  	struct i2c_adapter *adap = NULL;
> +	int err;
>  
>  	if (smo8800->i2c_dev)
>  		return;
> @@ -256,11 +386,22 @@ static void smo8800_instantiate_i2c_client(struct work_struct *work)
>  		return;
>  
>  	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);
> -	if (!lis3lv02d_dmi_id)
> +	if (lis3lv02d_dmi_id) {
> +		info.addr = (long)lis3lv02d_dmi_id->driver_data;
> +		/* Always detect the accel-type, this also checks the accel is actually there */
> +		err = smo8800_detect_accel(smo8800, adap, info.addr, &info, false);
> +		if (err)
> +			goto out_put_adapter;
> +	} else if (probe_i2c_addr) {
> +		/* First try address 0x29 (most used) and then try 0x1d */
> +		if (smo8800_detect_accel(smo8800, adap, 0x29, &info, true) != 0 &&
> +		    smo8800_detect_accel(smo8800, adap, 0x1d, &info, true) != 0) {
> +			dev_warn(smo8800->dev, "failed to probe for lis3lv02d I2C address\n");
> +			goto out_put_adapter;
> +		}
> +	} else {
>  		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)) {
> @@ -357,7 +498,7 @@ static int smo8800_probe(struct platform_device *device)
>  			 smo8800->irq);
>  	}
>  
> -	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
> +	if (dmi_check_system(smo8800_lis3lv02d_devices) || probe_i2c_addr) {
>  		/*
>  		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
>  		 * i2c_client instantiation.
> @@ -370,6 +511,7 @@ static int smo8800_probe(struct platform_device *device)
>  	} else {
>  		dev_warn(&device->dev,
>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> +		dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
>  		if (!smo8800->irq)
>  			return -ENODEV;
>  	}
> -- 
> 2.45.1
>
Hans de Goede June 22, 2024, 2:21 p.m. UTC | #3
Hi Pali,

On 6/22/24 3:32 PM, Pali Rohár wrote:
> On Friday 21 June 2024 14:25:01 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.
> 
> This is statement which I got from Dell for 10 years old Dell models.
> 
> I have already stated that poking of address in kernel is a big risk
> specially for all current and any future dell hardware. Hiding it just
> under the kernel parameter is still a risk, specially as neither its
> name, nor description say that it is dangerous:

I think you are overstating how dangerous this is. lm-sensors detect
scripts has been poking i2c addresses for years without problems (1) and
still does so till today.

Besides the kernel message telling users about this option does mention that
it is dangerous:

>> @@ -370,6 +511,7 @@ static int smo8800_probe(struct platform_device *device)
>>  	} else {
>>  		dev_warn(&device->dev,
>>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
>> +		dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
>>  		if (!smo8800->irq)
>>  			return -ENODEV;
>>  	}

> But anyway, why this code is being introduced?

Because users have been asking about an easier way to find the address for
not yet supported Dell models:

https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/

This is the whole reason why I started working on this patch-set in
the first place.

> Have you communicated
> with Dell about this problem?

Dell is on the Cc of this thread, as well as the previous v2 posting:

Cc: Dell.Client.Kernel@dell.com

Regards,

Hans


1) There were some problems more then a decade ago, but those were only
at specific addresses on some really old (by now) ThinkPads and for
the other case the i2c_safety_check() function was added.
Pali Rohár June 22, 2024, 2:50 p.m. UTC | #4
On Saturday 22 June 2024 16:21:28 Hans de Goede wrote:
> Hi Pali,
> 
> On 6/22/24 3:32 PM, Pali Rohár wrote:
> > On Friday 21 June 2024 14:25:01 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.
> > 
> > This is statement which I got from Dell for 10 years old Dell models.
> > 
> > I have already stated that poking of address in kernel is a big risk
> > specially for all current and any future dell hardware. Hiding it just
> > under the kernel parameter is still a risk, specially as neither its
> > name, nor description say that it is dangerous:
> 
> I think you are overstating how dangerous this is. lm-sensors detect
> scripts has been poking i2c addresses for years without problems (1) and
> still does so till today.
> 
> Besides the kernel message telling users about this option does mention that
> it is dangerous:

But description from modinfo -p and neither parameter name itself does not.
And neither no kernel message when the parameter was enabled.

> >> @@ -370,6 +511,7 @@ static int smo8800_probe(struct platform_device *device)
> >>  	} else {
> >>  		dev_warn(&device->dev,
> >>  			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
> >> +		dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
> >>  		if (!smo8800->irq)
> >>  			return -ENODEV;
> >>  	}
> 
> > But anyway, why this code is being introduced?
> 
> Because users have been asking about an easier way to find the address for
> not yet supported Dell models:
> 
> https://lore.kernel.org/linux-i2c/4820e280-9ca4-4d97-9d21-059626161bfc@molgen.mpg.de/
> 
> This is the whole reason why I started working on this patch-set in
> the first place.
> 
> > Have you communicated
> > with Dell about this problem?
> 
> Dell is on the Cc of this thread, as well as the previous v2 posting:
> 
> Cc: Dell.Client.Kernel@dell.com

And what do you think, that if you send tons of long emails with C code
to lot of recipients and add a carbon copy of all those your emails to
some corporate group address, that somebody on that group address will
read every one paragraph of every your email and tries to detect in if
there is not hidden some question to which you want to know answer?
No, with this behavior you are going to be put on the corporate
spam list by automated systems.

This looks like you just wanted to mark your personal checkbox
"I sent email to some dell address" and let it as is.

In past when I sent private email to dell I normally received responses.
Also they said to me that group address is (or at that time was)
monitored.

So it would be nice to start communication with dell and figure out what
is the current state of smbus address detection via ACPI/WMI/DMI/whatever,
instead of adding this hacks via poking of smbus addresses.

And in case the mentioned group address does not work anymore there are
still other linux developers from dell who could be able to figure
something out.

> Regards,
> 
> Hans
> 
> 
> 1) There were some problems more then a decade ago, but those were only
> at specific addresses on some really old (by now) ThinkPads and for
> the other case the i2c_safety_check() function was added.
> 
>
Andy Shevchenko June 22, 2024, 10:50 p.m. UTC | #5
On Sat, Jun 22, 2024 at 4:51 PM Pali Rohár <pali@kernel.org> wrote:
> On Saturday 22 June 2024 16:21:28 Hans de Goede wrote:
> > On 6/22/24 3:32 PM, Pali Rohár wrote:
> > > On Friday 21 June 2024 14:25:01 Hans de Goede wrote:

...

> > > Have you communicated
> > > with Dell about this problem?
> >
> > Dell is on the Cc of this thread, as well as the previous v2 posting:
> >
> > Cc: Dell.Client.Kernel@dell.com
>
> And what do you think, that if you send tons of long emails with C code
> to lot of recipients and add a carbon copy of all those your emails to
> some corporate group address, that somebody on that group address will
> read every one paragraph of every your email and tries to detect in if
> there is not hidden some question to which you want to know answer?
> No, with this behavior you are going to be put on the corporate
> spam list by automated systems.
>
> This looks like you just wanted to mark your personal checkbox
> "I sent email to some dell address" and let it as is.
>
> In past when I sent private email to dell I normally received responses.
> Also they said to me that group address is (or at that time was)
> monitored.
>
> So it would be nice to start communication with dell and figure out what
> is the current state of smbus address detection via ACPI/WMI/DMI/whatever,
> instead of adding this hacks via poking of smbus addresses.
>
> And in case the mentioned group address does not work anymore there are
> still other linux developers from dell who could be able to figure
> something out.

I would put it as "does Dell care about Linux?". If they do, they
should react to the messages in the Cc list. I was trying, for
example, to communicate with Relatek on some ACPI ID matters and
despite having tons of active developers it was no help. They
evidently don't care, so why would I?
diff mbox series

Patch

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index 4c79b2599d96..d64d200e927a 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -10,6 +10,7 @@ 
  */
 
 #define DRIVER_NAME "smo8800"
+#define LIS3_WHO_AM_I 0x0f
 
 #include <linux/device/bus.h>
 #include <linux/dmi.h>
@@ -25,6 +26,10 @@ 
 #include <linux/uaccess.h>
 #include <linux/workqueue.h>
 
+static bool probe_i2c_addr;
+module_param(probe_i2c_addr, bool, 0444);
+MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown");
+
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
 	atomic_t counter;            /* count after last read */
@@ -225,6 +230,130 @@  static const struct dmi_system_id smo8800_lis3lv02d_devices[] = {
 	{ }
 };
 
+/*
+ * 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 device *dev, struct i2c_adapter *adap, u8 addr)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+	u8 data;
+
+	/*
+	 * First receive a byte from the chip, and remember it. This
+	 * also checks if there is a device at the address at all.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	data = smbus_data.byte;
+
+	/*
+	 * Receive a byte again; very likely to be the same for
+	 * 1-register-only devices.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a register offset equal to
+	 * the read byte; for 1-register-only device this should read
+	 * the same byte value in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a slightly different register
+	 * offset; this should again read the register offset in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != (data ^ 0x01))
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Apparently this is a 1-register-only device, restore the original
+	 * register value and leave it alone.
+	 */
+	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
+		       I2C_SMBUS_BYTE, NULL);
+	dev_warn(dev, "I2C safety check for address 0x%02x failed, skipping\n", addr);
+	return -ENODEV;
+}
+
+static int smo8800_detect_accel(struct smo8800_device *smo8800,
+				struct i2c_adapter *adap, u8 addr,
+				struct i2c_board_info *info, bool probe)
+{
+	union i2c_smbus_data smbus_data;
+	const char *type;
+	int err;
+
+	if (probe) {
+		dev_info(smo8800->dev, "Probing for accelerometer on address 0x%02x\n", addr);
+		err = i2c_safety_check(smo8800->dev, 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) {
+		dev_warn(smo8800->dev, "Failed to read who-am-i register: %d\n", err);
+		return err;
+	}
+
+	/* who-am-i register mappings from drivers/misc/lis3lv02d/lis3lv02d.c */
+	switch (smbus_data.byte) {
+	case 0x32:
+		type = "lis331dlh";
+		break;
+	case 0x33:
+		type = "lis2de12"; /* LIS3DC / HP3DC in drivers/misc/lis3lv02d/lis3lv02d.c */
+		break;
+	case 0x3a:
+		type = "lis3lv02dl_accel";
+		break;
+	case 0x3b:
+		type = "lis302dl";
+		break;
+	default:
+		dev_warn(smo8800->dev, "Unknown who-am-i register value 0x%02x\n",
+			 smbus_data.byte);
+		return -ENODEV;
+	}
+
+	dev_dbg(smo8800->dev, "Detected %s accelerometer on address 0x%02x\n", type, addr);
+	strscpy(info->type, "lis3lv02d", I2C_NAME_SIZE);
+	info->addr = addr;
+	return 0;
+}
+
 static int smo8800_find_i801(struct device *dev, void *data)
 {
 	struct i2c_adapter *adap, **adap_ret = data;
@@ -247,6 +376,7 @@  static void smo8800_instantiate_i2c_client(struct work_struct *work)
 	const struct dmi_system_id *lis3lv02d_dmi_id;
 	struct i2c_board_info info = { };
 	struct i2c_adapter *adap = NULL;
+	int err;
 
 	if (smo8800->i2c_dev)
 		return;
@@ -256,11 +386,22 @@  static void smo8800_instantiate_i2c_client(struct work_struct *work)
 		return;
 
 	lis3lv02d_dmi_id = dmi_first_match(smo8800_lis3lv02d_devices);
-	if (!lis3lv02d_dmi_id)
+	if (lis3lv02d_dmi_id) {
+		info.addr = (long)lis3lv02d_dmi_id->driver_data;
+		/* Always detect the accel-type, this also checks the accel is actually there */
+		err = smo8800_detect_accel(smo8800, adap, info.addr, &info, false);
+		if (err)
+			goto out_put_adapter;
+	} else if (probe_i2c_addr) {
+		/* First try address 0x29 (most used) and then try 0x1d */
+		if (smo8800_detect_accel(smo8800, adap, 0x29, &info, true) != 0 &&
+		    smo8800_detect_accel(smo8800, adap, 0x1d, &info, true) != 0) {
+			dev_warn(smo8800->dev, "failed to probe for lis3lv02d I2C address\n");
+			goto out_put_adapter;
+		}
+	} else {
 		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)) {
@@ -357,7 +498,7 @@  static int smo8800_probe(struct platform_device *device)
 			 smo8800->irq);
 	}
 
-	if (dmi_check_system(smo8800_lis3lv02d_devices)) {
+	if (dmi_check_system(smo8800_lis3lv02d_devices) || probe_i2c_addr) {
 		/*
 		 * Register i2c-bus notifier + queue initial scan for lis3lv02d
 		 * i2c_client instantiation.
@@ -370,6 +511,7 @@  static int smo8800_probe(struct platform_device *device)
 	} else {
 		dev_warn(&device->dev,
 			 "lis3lv02d accelerometer is present on SMBus but its address is unknown, skipping registration\n");
+		dev_info(&device->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
 		if (!smo8800->irq)
 			return -ENODEV;
 	}