diff mbox series

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

Message ID 20231224213629.395741-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 Dec. 24, 2023, 9:36 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 | 112 +++++++++++++++++++++--
 1 file changed, 105 insertions(+), 7 deletions(-)

Comments

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

I would really like to hear Dell opinion about this change, and if there
is really no way to get i2c address. Could you ask Dell people about it?
Always it is better to use official / vendor provided steps of hardware
detection, instead of inventing something new / own which would be there
for a long time...
Hans de Goede Jan. 5, 2024, 4:36 p.m. UTC | #2
Hi Pali,

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

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

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

Regards,

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

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

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

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

> Regards,
> 
> Hans
> 
>
Paul Menzel Jan. 8, 2024, 1:22 p.m. UTC | #4
Dear Greg,


Am 05.01.24 um 17:36 schrieb Hans de Goede:

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

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

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


Kind regards,

Paul
diff mbox series

Patch

diff --git a/drivers/platform/x86/dell/dell-smo8800.c b/drivers/platform/x86/dell/dell-smo8800.c
index bb1d3e439761..60ce2695ce01 100644
--- a/drivers/platform/x86/dell/dell-smo8800.c
+++ b/drivers/platform/x86/dell/dell-smo8800.c
@@ -29,6 +29,10 @@  static bool use_misc_lis3lv02d;
 module_param(use_misc_lis3lv02d, bool, 0444);
 MODULE_PARM_DESC(use_misc_lis3lv02d, "Use /dev/freefall chardev + evdev joystick emulation instead of iio accel driver");
 
+static bool probe_i2c_addr;
+module_param(probe_i2c_addr, bool, 0444);
+MODULE_PARM_DESC(probe_i2c_addr, "Probe the i801 I2C bus for the accelerometer on models where the address is unknown");
+
 struct smo8800_device {
 	u32 irq;                     /* acpi device irq */
 	atomic_t counter;            /* count after last read */
@@ -146,6 +150,82 @@  static int smo8800_find_i801(struct device *dev, void *data)
 	return 1;
 }
 
+/*
+ * This is the kernel version of the single register device sanity checks from
+ * the i2c_safety_check function from lm_sensors sensor-detect script:
+ * This is meant to prevent access to 1-register-only devices,
+ * which are designed to be accessed with SMBus receive byte and SMBus send
+ * byte transactions (i.e. short reads and short writes) and treat SMBus
+ * read byte as a real write followed by a read. The device detection
+ * routines would write random values to the chip with possibly very nasty
+ * results for the hardware. Note that this function won't catch all such
+ * chips, as it assumes that reads and writes relate to the same register,
+ * but that's the best we can do.
+ */
+static int i2c_safety_check(struct smo8800_device *smo8800, struct i2c_adapter *adap, u8 addr)
+{
+	union i2c_smbus_data smbus_data;
+	int err;
+	u8 data;
+
+	/*
+	 * First receive a byte from the chip, and remember it. This
+	 * also checks if there is a device at the address at all.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	data = smbus_data.byte;
+
+	/*
+	 * Receive a byte again; very likely to be the same for
+	 * 1-register-only devices.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, 0,
+			     I2C_SMBUS_BYTE, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a register offset equal to
+	 * the read byte; for 1-register-only device this should read
+	 * the same byte value in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != data)
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Then try a standard byte read, with a slightly different register
+	 * offset; this should again read the register offset in return.
+	 */
+	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, data ^ 0x01,
+			     I2C_SMBUS_BYTE_DATA, &smbus_data);
+	if (err < 0)
+		return err;
+
+	if (smbus_data.byte != (data ^ 0x01))
+		return 0; /* Not a 1-register-only device. */
+
+	/*
+	 * Apparently this is a 1-register-only device, restore the original
+	 * register value and leave it alone.
+	 */
+	i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_WRITE, data,
+		       I2C_SMBUS_BYTE, NULL);
+	dev_warn(smo8800->dev, "I2C safety check for address 0x%02x failed, skipping\n", addr);
+	return -ENODEV;
+}
+
 /*
  * Set label to let iio-sensor-proxy know these freefall sensors are located in
  * the laptop base (not the display) and are not intended for screen rotation.
@@ -161,12 +241,19 @@  const struct software_node smo8800_accel_node = {
 
 static int smo8800_detect_accel(struct smo8800_device *smo8800,
 				struct i2c_adapter *adap, u8 addr,
-				struct i2c_board_info *info)
+				struct i2c_board_info *info, bool probe)
 {
 	union i2c_smbus_data smbus_data;
 	const char *type;
 	int err;
 
+	if (probe) {
+		dev_info(smo8800->dev, "Probing for accelerometer on address 0x%02x\n", addr);
+		err = i2c_safety_check(smo8800, adap, addr);
+		if (err < 0)
+			return err;
+	}
+
 	err = i2c_smbus_xfer(adap, addr, 0, I2C_SMBUS_READ, LIS3_WHO_AM_I,
 			     I2C_SMBUS_BYTE_DATA, &smbus_data);
 	if (err < 0) {
@@ -253,17 +340,25 @@  static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 		}
 	}
 
-	if (!addr) {
+	if (addr) {
+		/* Always detect the accel-type, this also checks the accel is actually there */
+		err = smo8800_detect_accel(smo8800, adap, addr, &info, false);
+		if (err)
+			goto put_adapter;
+	} else if (probe_i2c_addr) {
+		/* First try address 0x29 (most used) and then try 0x1d */
+		if (smo8800_detect_accel(smo8800, adap, 0x29, &info, true) != 0 &&
+		    smo8800_detect_accel(smo8800, adap, 0x1d, &info, true) != 0) {
+			dev_warn(smo8800->dev, "No accelerometer found\n");
+			goto put_adapter;
+		}
+	} else {
 		dev_warn(smo8800->dev,
 			 "Accelerometer lis3lv02d is present on SMBus but its address is unknown, skipping registration\n");
+		dev_info(smo8800->dev, "Pass dell_smo8800.probe_i2c_addr=1 on the kernel commandline to probe, this may be dangerous!\n");
 		goto put_adapter;
 	}
 
-	/* Always detect the accel-type, this also checks the accel is actually there */
-	err = smo8800_detect_accel(smo8800, adap, addr, &info);
-	if (err)
-		goto put_adapter;
-
 	/*
 	 * If requested override detected type with "lis3lv02d" i2c_client_id,
 	 * for the old drivers/misc/lis3lv02d/lis3lv02d.c driver.
@@ -281,6 +376,9 @@  static void smo8800_instantiate_i2c_client(struct smo8800_device *smo8800)
 	} else {
 		dev_info(smo8800->dev, "Registered %s accelerometer on address 0x%02x\n",
 			 info.type, info.addr);
+		if (!addr)
+			dev_info(smo8800->dev,
+				 "Please report this address upstream together with the output of 'cat /sys/class/dmi/id/product_name'\n");
 	}
 put_adapter:
 	i2c_put_adapter(adap);