Skip to contentSkip to conversation

i2c: i801: Register optional lis3lv02d i2c device on Dell machines

PATCH In review Pali Rohár 5 participants 27 messages Jan 27, 2018 – Jun 4, 2019

Subsystems (3)
I2C subsystem host drivers x86 platform drivers I2C/smbus controller drivers for pc
lore
Share & tools
lore

Pali Rohár

author
#0 lore raw

PATCH v2 1/1 i2c: i801: Register optional lis3lv02d i2c device on Dell machines

Dell platform team told us that some (DMI whitelisted) Dell Latitude machines have ST microelectronics accelerometer at i2c address 0x29.

Presence of that ST microelectronics accelerometer is verified by existence of SMO88xx ACPI device which represent that accelerometer. Unfortunately ACPI device does not specify i2c address.

This patch registers lis3lv02d device for selected Dell Latitude machines at i2c address 0x29 after detection. And for Dell Vostro V131 machine at i2c address 0x1d which was manually detected.

Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so lis3lv02d correctly initialize accelerometer.

Tested on Dell Latitude E6440.

Signed-off-by Pali Rohár <[email protected]>

---
Changes since v1:
* Added Dell Vostro V131 based on Michał Kępień testing
* Changed DMI product structure to include also i2c address

I re-tested this patch against Debian's 4.9 kernel on E6440.

---
 drivers/i2c/busses/i2c-i801.c | 104 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)
drivers/i2c/busses/i2c-i801.c +104 -0
index 8eac00efadc1..502678e8b8c0 100644
@@ -1136,6 +1136,107 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
 	}
 }
 
+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;
+
+	status = acpi_get_object_info(obj_handle, &info);
+	if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
+		return AE_OK;
+
+	hid = info->hardware_id.string;
+	if (!hid)
+		return AE_OK;
+
+	if (strlen(hid) < 7)
+		return AE_OK;
+
+	if (memcmp(hid, "SMO88", 5) != 0)
+		return AE_OK;
+
+	*((bool *)return_value) = true;
+	return AE_CTRL_TERMINATE;
+}
+
+static bool is_dell_system_with_lis3lv02d(void)
+{
+	bool found;
+	acpi_status status;
+	const char *vendor;
+
+	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+	if (strcmp(vendor, "Dell Inc.") != 0)
+		return false;
+
+	/*
+	 * Check that ACPI device SMO88xx exists and is enabled. That ACPI
+	 * device represent our ST microelectronics lis3lv02d accelerometer but
+	 * unfortunately without any other information (like i2c address).
+	 */
+	found = false;
+	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
+				  (void **)&found);
+	if (!ACPI_SUCCESS(status) || !found)
+		return false;
+
+	return true;
+}
+
+/*
+ * 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 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.
+	 */
+	{ "Vostro V131",        0x1d },
+};
+
+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 i2c bus but its"
+			 " i2c address is unknown, skipping registration...\n");
+		return;
+	}
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	info.addr = dell_lis3lv02d_devices[i].i2c_addr;
+	strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+	i2c_new_device(&priv->adapter, &info);
+}
+
 /* Register optional slaves */
 static void i801_probe_optional_slaves(struct i801_priv *priv)
 {

@@ -1154,6 +1255,9 @@ 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);
 }
 #else
 static void __init input_apanel_init(void) {}
Signature · 1 line
2.11.0
#1 lore raw
Quoted 42 lines · Pali Rohár

Dell platform team told us that some (DMI whitelisted) Dell Latitude machines have ST microelectronics accelerometer at i2c address 0x29.

Presence of that ST microelectronics accelerometer is verified by existence of SMO88xx ACPI device which represent that accelerometer. Unfortunately ACPI device does not specify i2c address.

This patch registers lis3lv02d device for selected Dell Latitude machines at i2c address 0x29 after detection. And for Dell Vostro V131 machine at i2c address 0x1d which was manually detected.

Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so lis3lv02d correctly initialize accelerometer.

Tested on Dell Latitude E6440.

drivers/i2c/busses/i2c-i801.c +25 -0
+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;
+
+       status = acpi_get_object_info(obj_handle, &info);
+       if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
+               return AE_OK;
+
+       hid = info->hardware_id.string;
+       if (!hid)
+               return AE_OK;
+
+       if (strlen(hid) < 7)
+               return AE_OK;
+
+       if (memcmp(hid, "SMO88", 5) != 0)
+               return AE_OK;
+
+       *((bool *)return_value) = true;
+       return AE_CTRL_TERMINATE;

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1180

Quoted 10 lines
drivers/i2c/busses/i2c-i801.c +9 -0
+}
+
+static bool is_dell_system_with_lis3lv02d(void)
+{
 
+       /*
+        * Check that ACPI device SMO88xx exists and is enabled. That ACPI
+        * device represent our ST microelectronics lis3lv02d accelerometer but
+        * unfortunately without any other information (like i2c address).
+        */

Isn't it simple

return acpi_dev_present("SMO88", NULL, -1);

call?

Quoted 8 lines
drivers/i2c/busses/i2c-i801.c +8 -0
+       found = false;
+       status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
+                                 (void **)&found);
+       if (!ACPI_SUCCESS(status) || !found)
+               return false;
+
+       return true;
+}
Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#2 lore raw
Quoted 10 lines · On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote:

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1180

Quoted 8 lines · Pali Rohár
drivers/i2c/busses/i2c-i801.c +7 -0
+static bool is_dell_system_with_lis3lv02d(void)
+{
 
+       /*
+        * Check that ACPI device SMO88xx exists and is enabled. That ACPI
+        * device represent our ST microelectronics lis3lv02d accelerometer but
+        * unfortunately without any other information (like i2c address).
+        */
Quoted 5 lines

Isn't it simple

return acpi_dev_present("SMO88", NULL, -1);

call?

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

Quoted 10 lines
Quoted 8 lines
drivers/i2c/busses/i2c-i801.c +8 -0
+       found = false;
+       status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
+                                 (void **)&found);
+       if (!ACPI_SUCCESS(status) || !found)
+               return false;
+
+       return true;
+}
Signature · 2 lines
Pali Rohár
[email protected]
#3 lore raw
Quoted 11 lines · Pali Rohár
Quoted 10 lines · On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote:
Quoted 8 lines · Pali Rohár
drivers/i2c/busses/i2c-i801.c +7 -0
+static bool is_dell_system_with_lis3lv02d(void)
+{
 
+       /*
+        * Check that ACPI device SMO88xx exists and is enabled. That ACPI
+        * device represent our ST microelectronics lis3lv02d accelerometer but
+        * unfortunately without any other information (like i2c address).
+        */
Quoted 8 lines
Quoted 5 lines

Isn't it simple

return acpi_dev_present("SMO88", NULL, -1);

call?

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

OK, fair enough.

Do we have more users of such pattern? If so, it might make sense to introduce a generic helper for that which takes a list of HIDs on input.

(Yes, I do not like matching pattern like "XYZhh*", I prefer explicit list of HIDs. Rationale to do so: a) any new potential collision is excluded, b) we can easily grep kernel for a users per HID)

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#4 lore raw
Quoted 16 lines · On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
Quoted 15 lines · Pali Rohár
Quoted 14 lines · On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote:

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1180

Quoted 8 lines · Pali Rohár
drivers/i2c/busses/i2c-i801.c +7 -0
+static bool is_dell_system_with_lis3lv02d(void)
+{
 
+       /*
+        * Check that ACPI device SMO88xx exists and is enabled. That ACPI
+        * device represent our ST microelectronics lis3lv02d accelerometer but
+        * unfortunately without any other information (like i2c address).
+        */

Isn't it simple

return acpi_dev_present("SMO88", NULL, -1);

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

call?

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

OK, fair enough.

Do we have more users of such pattern?

I have not seen this ACPI pattern yet, so probably not.

Signature · 2 lines
Pali Rohár
[email protected]
#5 lore raw
Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
Quoted 2 lines · Pali Rohár

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

OK, fair enough.

Do we have more users of such pattern?

I have not seen this ACPI pattern yet, so probably not.

I see. So, my one concern is the implicit names of the devices. I would like rather to see

... acpi_device_id ... []= {
{"SMO8800"},
{"SMO8810"},
...
{}
};

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#6 lore raw
Quoted 16 lines · On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
Quoted 2 lines · Pali Rohár

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

OK, fair enough.

Do we have more users of such pattern?

I have not seen this ACPI pattern yet, so probably not.

I see. So, my one concern is the implicit names of the devices. I would like rather to see

Quoted 6 lines

... acpi_device_id ... []= {
{"SMO8800"},
{"SMO8810"},
...
{}
};

Following table already exists in dell-smo8800.c file:

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);

Can we reuse it? Maybe moving array smo8800_ids[] into some header file (which one?) and statically inline it? Or having it only in dell-smo8800.c file and exporting its symbol? Or is there better idea?

For sure I do not want to copy paste this table into another module and maintaining two copies of this list.

Signature · 2 lines
Pali Rohár
[email protected]
#7 lore raw
Quoted 38 lines · Pali Rohár
Quoted 16 lines · On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
Quoted 2 lines · Pali Rohár

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

OK, fair enough.

Do we have more users of such pattern?

I have not seen this ACPI pattern yet, so probably not.

I see. So, my one concern is the implicit names of the devices. I would like rather to see

Quoted 6 lines

... acpi_device_id ... []= {
{"SMO8800"},
{"SMO8810"},
...
{}
};

Following table already exists in dell-smo8800.c file:

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 },
};
Quoted 6 lines

MODULE_DEVICE_TABLE(acpi, smo8800_ids);

Can we reuse it?

Maybe moving array smo8800_ids[] into some header file
(which one?) and statically inline it?

Bad idea.

Quoted 2 lines

Or having it only in
dell-smo8800.c file and exporting its symbol?

Even worse.

Quoted 4 lines

Or is there better idea?

For sure I do not want to copy paste this table into another module and maintaining two copies of this list.

The copy is fine. Can you guarantee that those two lists would be always the same? I'm not.
And besides that explicitly over implicitly is a really good thing. I would not like to grep for an ID followed by grepping include line and check each files to check if it uses it or not.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#8 lore raw
Quoted 53 lines · On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote:
Quoted 38 lines · Pali Rohár
Quoted 16 lines · On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
Quoted 2 lines · Pali Rohár

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

OK, fair enough.

Do we have more users of such pattern?

I have not seen this ACPI pattern yet, so probably not.

I see. So, my one concern is the implicit names of the devices. I would like rather to see

Quoted 6 lines

... acpi_device_id ... []= {
{"SMO8800"},
{"SMO8810"},
...
{}
};

Following table already exists in dell-smo8800.c file:

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 },
};
Quoted 6 lines

MODULE_DEVICE_TABLE(acpi, smo8800_ids);

Can we reuse it?

Maybe moving array smo8800_ids[] into some header file
(which one?) and statically inline it?

Bad idea.

Quoted 2 lines

Or having it only in
dell-smo8800.c file and exporting its symbol?

Even worse.

Quoted 7 lines
Quoted 4 lines

Or is there better idea?

For sure I do not want to copy paste this table into another module and maintaining two copies of this list.

The copy is fine. Can you guarantee that those two lists would be always the same? I'm not.

Me neither.

Quoted 3 lines

And besides that explicitly over implicitly is a really good thing. I would not like to grep for an ID followed by grepping include line and check each files to check if it uses it or not.

So what do you suggest now?

Having one file where it would be defined is a bad idea for you. And maintaining copy of same array in two different files in two different subsystems is something which I cannot guarantee.

Therefore the current patch is the best approach. No shared file with shared array/table and also no copy of that array in two different subsystems.

Signature · 2 lines
Pali Rohár
[email protected]
#9 lore raw
Quoted 62 lines · Pali Rohár
Quoted 53 lines · On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote:
Quoted 38 lines · Pali Rohár
Quoted 16 lines · On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
Quoted 2 lines · Pali Rohár

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

OK, fair enough.

Do we have more users of such pattern?

I have not seen this ACPI pattern yet, so probably not.

I see. So, my one concern is the implicit names of the devices. I would like rather to see

Quoted 6 lines

... acpi_device_id ... []= {
{"SMO8800"},
{"SMO8810"},
...
{}
};

Following table already exists in dell-smo8800.c file:

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 },
};
Quoted 6 lines

MODULE_DEVICE_TABLE(acpi, smo8800_ids);

Can we reuse it?

Maybe moving array smo8800_ids[] into some header file
(which one?) and statically inline it?

Bad idea.

Quoted 2 lines

Or having it only in
dell-smo8800.c file and exporting its symbol?

Even worse.

Quoted 7 lines
Quoted 4 lines

Or is there better idea?

For sure I do not want to copy paste this table into another module and maintaining two copies of this list.

The copy is fine. Can you guarantee that those two lists would be always the same? I'm not.

Quoted 7 lines

Me neither.

Quoted 3 lines

And besides that explicitly over implicitly is a really good thing. I would not like to grep for an ID followed by grepping include line and check each files to check if it uses it or not.

So what do you suggest now?

Copy'n'paste and maintain two lists.
Yes, it's not the ideal, but working solution.

You may put a comment before each list to explain what the second does and tell a contributor to look at it and update if needed.

Quoted 1 line

Having one file where it would be defined is a bad idea for you.

Not just "one file", but "one *header* file". Or "exporting a symbol" which is basically not supposed to be exported.
ID tables are part of the actual drivers, neither headers, nor libraries.

Quoted 4 lines

And maintaining copy of same array in two different files in two different subsystems is something which I cannot guarantee.

Therefore the current patch is the best approach.

I don't like it. I'll not take it, sorry.

Quoted 3 lines

No shared file with
shared array/table and also no copy of that array in two different subsystems.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#10 lore raw
Quoted 69 lines · On Tuesday 13 February 2018 17:06:19 Andy Shevchenko wrote:
Quoted 68 lines · Pali Rohár
Quoted 53 lines · On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote:
Quoted 38 lines · Pali Rohár
Quoted 16 lines · On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
Quoted 11 lines · Pali Rohár
Quoted 8 lines · On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
Quoted 2 lines · Pali Rohár

ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string?

OK, fair enough.

Do we have more users of such pattern?

I have not seen this ACPI pattern yet, so probably not.

I see. So, my one concern is the implicit names of the devices. I would like rather to see

Quoted 6 lines

... acpi_device_id ... []= {
{"SMO8800"},
{"SMO8810"},
...
{}
};

Following table already exists in dell-smo8800.c file:

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 },
};
Quoted 6 lines

MODULE_DEVICE_TABLE(acpi, smo8800_ids);

Can we reuse it?

Maybe moving array smo8800_ids[] into some header file
(which one?) and statically inline it?

Bad idea.

Quoted 2 lines

Or having it only in
dell-smo8800.c file and exporting its symbol?

Even worse.

Quoted 7 lines
Quoted 4 lines

Or is there better idea?

For sure I do not want to copy paste this table into another module and maintaining two copies of this list.

The copy is fine. Can you guarantee that those two lists would be always the same? I'm not.

Me neither.

Quoted 3 lines

And besides that explicitly over implicitly is a really good thing. I would not like to grep for an ID followed by grepping include line and check each files to check if it uses it or not.

Quoted 7 lines
Quoted 1 line

So what do you suggest now?

Copy'n'paste and maintain two lists.
Yes, it's not the ideal, but working solution.

You may put a comment before each list to explain what the second does and tell a contributor to look at it and update if needed.

I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them.

Jean, it is OK?

Quoted 5 lines
Quoted 1 line

Having one file where it would be defined is a bad idea for you.

Not just "one file", but "one *header* file". Or "exporting a symbol" which is basically not supposed to be exported.
ID tables are part of the actual drivers, neither headers, nor libraries.

But this is exactly what is needed. This ACPI ID table contains ACPI names which says if accelerometer is present or not.

Quoted 12 lines
Quoted 4 lines

And maintaining copy of same array in two different files in two different subsystems is something which I cannot guarantee.

Therefore the current patch is the best approach.

I don't like it. I'll not take it, sorry.

Quoted 3 lines

No shared file with
shared array/table and also no copy of that array in two different subsystems.

Signature · 2 lines
Pali Rohár
[email protected]
#11 lore raw
Quoted 35 lines · Pali Rohár
Quoted 34 lines · On Tuesday 13 February 2018 17:06:19 Andy Shevchenko wrote:
Quoted 32 lines · Pali Rohár
Quoted 16 lines · On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote:
Quoted 7 lines · Pali Rohár

On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:

Following table already exists in dell-smo8800.c file:
Can we reuse it?

Maybe moving array smo8800_ids[] into some header file
(which one?) and statically inline it?

Bad idea.

Quoted 2 lines

Or having it only in
dell-smo8800.c file and exporting its symbol?

Even worse.

Quoted 7 lines
Quoted 4 lines

Or is there better idea?

For sure I do not want to copy paste this table into another module and maintaining two copies of this list.

The copy is fine. Can you guarantee that those two lists would be always the same? I'm not.

Me neither.

Quoted 3 lines

And besides that explicitly over implicitly is a really good thing. I would not like to grep for an ID followed by grepping include line and check each files to check if it uses it or not.

So what do you suggest now?

Quoted 8 lines
Quoted 5 lines

Copy'n'paste and maintain two lists.
Yes, it's not the ideal, but working solution.

You may put a comment before each list to explain what the second does and tell a contributor to look at it and update if needed.

I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them.

Right. But from ACPI prospective the proposed change is not okay by my opinion.

How can I see what drivers are binding / relying on certain ACPI ID?

More precisely,

% git grep -n -w SMO8800

would be useless until one gets an idea to match against partial strings.

Quoted 10 lines

Jean, it is OK?

Quoted 5 lines
Quoted 1 line

Having one file where it would be defined is a bad idea for you.

Not just "one file", but "one *header* file". Or "exporting a symbol" which is basically not supposed to be exported.
ID tables are part of the actual drivers, neither headers, nor libraries.

But this is exactly what is needed. This ACPI ID table contains ACPI names which says if accelerometer is present or not.

In case of dell-smo8800.c the driver actually *binds* to these IDs. Your change just provide another table to match and answer to the question if XYZ is present on a such platform or not.

Similar approach is used in spi-pxa2xx.c (check pxa2xx_spi_pci_compound_match).

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#12 lore raw
Quoted 1 line · Andy Shevchenko

Similar approach is used in spi-pxa2xx.c (check pxa2xx_spi_pci_compound_match).

Another approach might be a registration of I2C board info from dell-smo8800.c. Not sure if it better, because theoretically user may disable that one, but leave i2c-i801 enabled.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
#13 lore raw
In reply to Pali Rohár
Quoted 3 lines

I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them.

This is really Jean's realm.

author
#14 lore raw
Quoted 6 lines · On Monday 26 February 2018 21:32:55 Wolfram Sang wrote:
Quoted 3 lines

I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them.

This is really Jean's realm.

Jean, ping.

Signature · 2 lines
Pali Rohár
[email protected]
author
#15 lore raw
Quoted 9 lines · On Thursday 08 March 2018 23:53:30 Pali Rohár wrote:
Quoted 6 lines · On Monday 26 February 2018 21:32:55 Wolfram Sang wrote:
Quoted 3 lines

I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them.

This is really Jean's realm.

Jean, ping.

Jean, gently ping again. Waiting for your maintainer response/decision on this problem.

Signature · 2 lines
Pali Rohár
[email protected]
author
#16 lore raw
Quoted 6 lines · On Wednesday 18 April 2018 13:46:10 Pali Rohár wrote:
Quoted 5 lines · On Thursday 08 March 2018 23:53:30 Pali Rohár wrote:
Quoted 4 lines · On Monday 26 February 2018 21:32:55 Wolfram Sang wrote:
Quoted 3 lines

I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

This is really Jean's realm.

Jean, ping.

Jean, gently ping again. Waiting for your maintainer response/decision on this problem.

Jean, one year passed without any answer. May you response to this thread about this patch?

Signature · 2 lines
Pali Rohár
[email protected]
#17 lore raw
In reply to Wolfram Sang
Quoted 4 lines · Wolfram Sang
Quoted 2 lines

I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them.

This is really Jean's realm.

Sorry for the delay. As a general rule I'm all in favor of instantiating I2C devices from i2c-i801 when we can, as it makes the user's life easier. However I agree with Andy that:
1* We want to have an explicit list of supported ACPI device IDs, not a
just a prefix.
2* We don't want to over-engineer it with a common header file or an
exported symbol. I see no problem with duplicating the lists if 2 drivers happen to be needed on the same set of devices. This is easily managed by adding a comment before each list that the other list may need to be kept in sync. It also gives us the flexibility to *not* keep them in sync if needed.

Instantiating the I2C device from dell-smo8800 doesn't seem practical because that driver has no idea about the i2c subsystem in the first place.

What worries me is that we seem to have 2 drivers binding to the same device (the accelerometer), one natively (lis3lv02d), and one through an ACPI layer (dell-smo8800). I don't really understand why this is needed (don't they serve the same purpose?) nor how it can be safe (what guarantees that both drivers won't attempt to access the hardware at the same time?)

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#18 lore raw

Hi! Thank you finally for response!

Quoted 6 lines · On Tuesday 28 May 2019 11:19:53 Jean Delvare wrote:
Quoted 4 lines · Wolfram Sang
Quoted 2 lines

I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them.

This is really Jean's realm.

Quoted 11 lines

Sorry for the delay. As a general rule I'm all in favor of instantiating I2C devices from i2c-i801 when we can, as it makes the user's life easier. However I agree with Andy that:
1* We want to have an explicit list of supported ACPI device IDs, not a
just a prefix.
2* We don't want to over-engineer it with a common header file or an
exported symbol. I see no problem with duplicating the lists if 2 drivers happen to be needed on the same set of devices. This is easily managed by adding a comment before each list that the other list may need to be kept in sync. It also gives us the flexibility to *not* keep them in sync if needed.

Ok, I will then make list of supported ACPI devices and put them into two places with comment about syncing.

Quoted 3 lines

Instantiating the I2C device from dell-smo8800 doesn't seem practical because that driver has no idea about the i2c subsystem in the first place.

Yes. And in same way instantiating ACPI driver dell-smo8800 from i2c-i801 is not practical too.

Quoted 6 lines

What worries me is that we seem to have 2 drivers binding to the same device (the accelerometer), one natively (lis3lv02d), and one through an ACPI layer (dell-smo8800). I don't really understand why this is needed (don't they serve the same purpose?) nor how it can be safe (what guarantees that both drivers won't attempt to access the hardware at the same time?)

This is not a problem. lis3lv02d provides two things:

1) 3 axes accelerometer
2) optional interrupt and signal it to userspace via misc device

dell-smo8800 does not call any parts of lis2lv02d module. It just provides for userspace same misc device API as lis3lv02d.

As lis3lv02d has misc device optional, registered i2c device from i2c-i801 does not enable it.

So technically it is one device, but their functionality divided into two modules. One which reports accelerometer axes and one which signals disk fall interrupt. These two modules and functionalities does not have to interact, so it is safe to have them separated.

Signature · 2 lines
Pali Rohár
[email protected]
#19 lore raw
Quoted 8 lines · Pali Rohár

This is not a problem. lis3lv02d provides two things:

1) 3 axes accelerometer
2) optional interrupt and signal it to userspace via misc device

dell-smo8800 does not call any parts of lis2lv02d module. It just provides for userspace same misc device API as lis3lv02d.

Quoted 7 lines

As lis3lv02d has misc device optional, registered i2c device from i2c-i801 does not enable it.

So technically it is one device, but their functionality divided into two modules. One which reports accelerometer axes and one which signals disk fall interrupt. These two modules and functionalities does not have to interact, so it is safe to have them separated.

OK, thanks for the explanation. But assuming that we now instantiate the lis2lv02d device from i2c-i801 for exactly all the same machines, can't we just *enable* the freefall misc device feature of lis2lv02d and kill the dell-smo8800 driver completely? Seems more simple to maintain going forward.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#20 lore raw
Quoted 17 lines · On Tuesday 28 May 2019 11:50:15 Jean Delvare wrote:
Quoted 8 lines · Pali Rohár

This is not a problem. lis3lv02d provides two things:

1) 3 axes accelerometer
2) optional interrupt and signal it to userspace via misc device

dell-smo8800 does not call any parts of lis2lv02d module. It just provides for userspace same misc device API as lis3lv02d.

Quoted 7 lines

As lis3lv02d has misc device optional, registered i2c device from i2c-i801 does not enable it.

So technically it is one device, but their functionality divided into two modules. One which reports accelerometer axes and one which signals disk fall interrupt. These two modules and functionalities does not have to interact, so it is safe to have them separated.

Quoted 5 lines

OK, thanks for the explanation. But assuming that we now instantiate the lis2lv02d device from i2c-i801 for exactly all the same machines, can't we just *enable* the freefall misc device feature of lis2lv02d and kill the dell-smo8800 driver completely? Seems more simple to maintain going forward.

I though about it and I already wrote that is it not practical. For ACPI drivers there is easy way to get that interrupt number from ACPI tables.

Quoted 1 line

From i2c-i801 PCI driver it is hard to get interrupt number for

particular ACPI device...

That is way I preferred simple solution: ACPI driver for ACPI device and i2c driver for i2c device.

Signature · 2 lines
Pali Rohár
[email protected]
#21 lore raw
Quoted 7 lines · Pali Rohár
Quoted 5 lines · On Tuesday 28 May 2019 11:50:15 Jean Delvare wrote:

OK, thanks for the explanation. But assuming that we now instantiate the lis2lv02d device from i2c-i801 for exactly all the same machines, can't we just *enable* the freefall misc device feature of lis2lv02d and kill the dell-smo8800 driver completely? Seems more simple to maintain going forward.

Quoted 7 lines

I though about it and I already wrote that is it not practical. For ACPI drivers there is easy way to get that interrupt number from ACPI tables. From i2c-i801 PCI driver it is hard to get interrupt number for particular ACPI device...

That is way I preferred simple solution: ACPI driver for ACPI device and i2c driver for i2c device.

OK, fine with me then :-)

Signature · 2 lines
Jean Delvare
SUSE L3 Support
Tested-by on v2 1/1
#22 lore raw
In reply to Pali Rohár
Quoted 15 lines

Dell platform team told us that some (DMI whitelisted) Dell Latitude machines have ST microelectronics accelerometer at i2c address 0x29.

Presence of that ST microelectronics accelerometer is verified by existence of SMO88xx ACPI device which represent that accelerometer. Unfortunately ACPI device does not specify i2c address.

This patch registers lis3lv02d device for selected Dell Latitude machines at i2c address 0x29 after detection. And for Dell Vostro V131 machine at i2c address 0x1d which was manually detected.

Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so lis3lv02d correctly initialize accelerometer.

Quoted 6 lines

Tested on Dell Latitude E6440.

Signed-off-by Pali Rohár <[email protected]>

---
Changes since v1:
* Added Dell Vostro V131 based on Michał Kępień testing

I tested this patch on a Vostro V131 and it seems to work as expected:
"lis3lv02d: 8 bits sensor found" is logged during boot, an input device gets registered and its axis values seem to be consistent with laptop's movements.

Tested-by Michał Kępień <[email protected]>

I did not review the contents of the patch this time, sorry.

Signature · 2 lines
Best regards,
Michał Kępień
#23 lore raw
Quoted 11 lines · Michał Kępień
Quoted 4 lines
Signed-off-by Pali Rohár <[email protected]>

---
Changes since v1:
* Added Dell Vostro V131 based on Michał Kępień testing

I tested this patch on a Vostro V131 and it seems to work as expected:
"lis3lv02d: 8 bits sensor found" is logged during boot, an input device gets registered and its axis values seem to be consistent with laptop's movements.

Tested-by Michał Kępień <[email protected]>

Thanks! I'm waiting for Pali to address my concern.

Quoted 1 line

I did not review the contents of the patch this time, sorry.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#24 lore raw

PATCH v3 1/1 i2c: i801: Register optional lis3lv02d i2c device on Dell machines

In reply to Pali Rohár

Dell platform team told us that some (DMI whitelisted) Dell Latitude machines have ST microelectronics accelerometer at i2c address 0x29.

Presence of that ST microelectronics accelerometer is verified by existence of SMO88xx ACPI device which represent that accelerometer. Unfortunately ACPI device does not specify i2c address.

This patch registers lis3lv02d device for selected Dell Latitude machines at i2c address 0x29 after detection. And for Dell Vostro V131 machine at i2c address 0x1d which was manually detected.

Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so lis3lv02d correctly initialize accelerometer.

Tested on Dell Latitude E6440.

Signed-off-by Pali Rohár <[email protected]>

---
Changes since v2:
* Use explicit list of SMOxx ACPI devices

Changes since v1:
* Added Dell Vostro V131 based on Michał Kępień testing
* Changed DMI product structure to include also i2c address

---
 drivers/i2c/busses/i2c-i801.c       | 118 ++++++++++++++++++++++++++++++++++++
 drivers/platform/x86/dell-smo8800.c |   1 +
 2 files changed, 119 insertions(+)
drivers/i2c/busses/i2c-i801.c +118 -0
index ac7f7817dc89..2ac8ff41cc24 100644
@@ -1134,6 +1134,121 @@ 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 struct acpi_device_id acpi_smo8800_ids[] = {
+	{ "SMO8800", 0 },
+	{ "SMO8801", 0 },
+	{ "SMO8810", 0 },
+	{ "SMO8811", 0 },
+	{ "SMO8820", 0 },
+	{ "SMO8821", 0 },
+	{ "SMO8830", 0 },
+	{ "SMO8831", 0 },
+};
+
+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_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
+		return AE_OK;
+
+	hid = info->hardware_id.string;
+	if (!hid)
+		return AE_OK;
+
+	for (i = 0; i < ARRAY_SIZE(acpi_smo8800_ids); ++i) {
+		if (strcmp(hid, acpi_smo8800_ids[i].id) == 0) {
+			*((bool *)return_value) = true;
+			return AE_CTRL_TERMINATE;
+		}
+	}
+
+	return AE_OK;
+
+}
+
+static bool is_dell_system_with_lis3lv02d(void)
+{
+	bool found;
+	acpi_status status;
+	const char *vendor;
+
+	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+	if (strcmp(vendor, "Dell Inc.") != 0)
+		return false;
+
+	/*
+	 * Check that ACPI device SMO88xx exists and is enabled. That ACPI
+	 * device represent our ST microelectronics lis3lv02d accelerometer but
+	 * unfortunately without any other information (like i2c address).
+	 */
+	found = false;
+	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
+				  (void **)&found);
+	if (!ACPI_SUCCESS(status) || !found)
+		return false;
+
+	return true;
+}
+
+/*
+ * 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 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.
+	 */
+	{ "Vostro V131",        0x1d },
+};
+
+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 i2c bus but its"
+			 " i2c address is unknown, skipping registration...\n");
+		return;
+	}
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	info.addr = dell_lis3lv02d_devices[i].i2c_addr;
+	strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE);
+	i2c_new_device(&priv->adapter, &info);
+}
+
 /* Register optional slaves */
 static void i801_probe_optional_slaves(struct i801_priv *priv)
 {

@@ -1152,6 +1267,9 @@ 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);
 }
 #else
 static void __init input_apanel_init(void) {}
drivers/platform/x86/dell-smo8800.c +1 -0
index 5cdb09cba077..bfcc1d1b9b96 100644
@@ -198,6 +198,7 @@ static int smo8800_remove(struct acpi_device *device)
 	return 0;
 }
 
+/* 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 },
Signature · 1 line
2.11.0
#25 lore raw

Hi Pali,

Next time, please start a new thread for a new version of a patch.

Quoted 68 lines · Pali Rohár

Dell platform team told us that some (DMI whitelisted) Dell Latitude machines have ST microelectronics accelerometer at i2c address 0x29.

Presence of that ST microelectronics accelerometer is verified by existence of SMO88xx ACPI device which represent that accelerometer. Unfortunately ACPI device does not specify i2c address.

This patch registers lis3lv02d device for selected Dell Latitude machines at i2c address 0x29 after detection. And for Dell Vostro V131 machine at i2c address 0x1d which was manually detected.

Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so lis3lv02d correctly initialize accelerometer.

Tested on Dell Latitude E6440.

Signed-off-by Pali Rohár <[email protected]>

---
Changes since v2:
* Use explicit list of SMOxx ACPI devices

Changes since v1:
* Added Dell Vostro V131 based on Michał Kępień testing
* Changed DMI product structure to include also i2c address

---
 drivers/i2c/busses/i2c-i801.c       | 118 ++++++++++++++++++++++++++++++++++++
 drivers/platform/x86/dell-smo8800.c |   1 +
 2 files changed, 119 insertions(+)
drivers/i2c/busses/i2c-i801.c +29 -0
index ac7f7817dc89..2ac8ff41cc24 100644
@@ -1134,6 +1134,121 @@ 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 struct acpi_device_id acpi_smo8800_ids[] = {
+	{ "SMO8800", 0 },
+	{ "SMO8801", 0 },
+	{ "SMO8810", 0 },
+	{ "SMO8811", 0 },
+	{ "SMO8820", 0 },
+	{ "SMO8821", 0 },
+	{ "SMO8830", 0 },
+	{ "SMO8831", 0 },
+};
+
+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_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
+		return AE_OK;
+
+	hid = info->hardware_id.string;
+	if (!hid)
+		return AE_OK;

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1175

Quoted 10 lines
drivers/i2c/busses/i2c-i801.c +10 -0
+
+	for (i = 0; i < ARRAY_SIZE(acpi_smo8800_ids); ++i) {
+		if (strcmp(hid, acpi_smo8800_ids[i].id) == 0) {
+			*((bool *)return_value) = true;
+			return AE_CTRL_TERMINATE;
+		}
+	}
+
+	return AE_OK;
+

Unneeded blank line.

Quoted 4 lines
drivers/i2c/busses/i2c-i801.c +4 -0
+}
+
+static bool is_dell_system_with_lis3lv02d(void)
+{

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1189

Quoted 10 lines
drivers/i2c/busses/i2c-i801.c +10 -0
+	bool found;
+	acpi_status status;
+	const char *vendor;
+
+	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+	if (strcmp(vendor, "Dell Inc.") != 0)
+		return false;
+
+	/*
+	 * Check that ACPI device SMO88xx exists and is enabled. That ACPI

I see how you check that the device exists, but not that it is enabled.

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1191

Quoted 2 lines
drivers/i2c/busses/i2c-i801.c +2 -0
+	 * device represent our ST microelectronics lis3lv02d accelerometer but
+	 * unfortunately without any other information (like i2c address).

I2C

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1199

Quoted 8 lines
drivers/i2c/busses/i2c-i801.c +8 -0
+	 */
+	found = false;
+	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
+				  (void **)&found);
+	if (!ACPI_SUCCESS(status) || !found)
+		return false;
+
+	return true;

Looks more complex than it needs to be. You don't really care about the status, as in the end you return the same on error as you do when no device is found. So I think you can simply go with:

	found = false;
	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
				  (void **)&found);

	return found;

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1203

Quoted 4 lines
drivers/i2c/busses/i2c-i801.c +4 -0
+}
+
+/*
+ * Accelerometer's i2c address is not specified in DMI nor ACPI,

I2C

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1206

Quoted 3 lines
drivers/i2c/busses/i2c-i801.c +3 -0
+ * so it is needed to define mapping table based on DMI product names.
+ */
+static struct {

Any reason to not make it const?

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1212

Quoted 6 lines
drivers/i2c/busses/i2c-i801.c +6 -0
+	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.

I2C

Quoted 19 lines
drivers/i2c/busses/i2c-i801.c +19 -0
+	 */
+	{ "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.
+	 */
+	{ "Vostro V131",        0x1d },
+};
+
+static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
+{
+	struct i2c_board_info info;
+	const char *dmi_product_name;
+	int i;
+

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1241

Quoted 10 lines
drivers/i2c/busses/i2c-i801.c +10 -0
+	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 i2c bus but its"

i2c bus -> SMBus

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1242

Quoted 1 line
drivers/i2c/busses/i2c-i801.c +1 -0
+			 " i2c address is unknown, skipping registration...\n");

I2C (or just s/i2c //, as it's kind of redundant)

Suspension points not really needed IMHO.

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)

Quoted 3 lines
drivers/i2c/busses/i2c-i801.c +3 -0
+		return;
+	}
+ (...)

All the rest looks good to me.

Thanks,

Signature · 2 lines
Jean Delvare
SUSE L3 Support
author
#26 lore raw

Hi!

Quoted 3 lines · On Tuesday 04 June 2019 16:57:29 Jean Delvare wrote:

Hi Pali,

Next time, please start a new thread for a new version of a patch.

Ok!

Quoted 73 lines · Pali Rohár
Quoted 72 lines

Dell platform team told us that some (DMI whitelisted) Dell Latitude machines have ST microelectronics accelerometer at i2c address 0x29.

Presence of that ST microelectronics accelerometer is verified by existence of SMO88xx ACPI device which represent that accelerometer. Unfortunately ACPI device does not specify i2c address.

This patch registers lis3lv02d device for selected Dell Latitude machines at i2c address 0x29 after detection. And for Dell Vostro V131 machine at i2c address 0x1d which was manually detected.

Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so lis3lv02d correctly initialize accelerometer.

Tested on Dell Latitude E6440.

Signed-off-by Pali Rohár <[email protected]>

---
Changes since v2:
* Use explicit list of SMOxx ACPI devices

Changes since v1:
* Added Dell Vostro V131 based on Michał Kępień testing
* Changed DMI product structure to include also i2c address

---
 drivers/i2c/busses/i2c-i801.c       | 118 ++++++++++++++++++++++++++++++++++++
 drivers/platform/x86/dell-smo8800.c |   1 +
 2 files changed, 119 insertions(+)
drivers/i2c/busses/i2c-i801.c +33 -0
index ac7f7817dc89..2ac8ff41cc24 100644
@@ -1134,6 +1134,121 @@ 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 struct acpi_device_id acpi_smo8800_ids[] = {
+	{ "SMO8800", 0 },
+	{ "SMO8801", 0 },
+	{ "SMO8810", 0 },
+	{ "SMO8811", 0 },
+	{ "SMO8820", 0 },
+	{ "SMO8821", 0 },
+	{ "SMO8830", 0 },
+	{ "SMO8831", 0 },
+};
+
+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_SUCCESS(status) || !(info->valid & ACPI_VALID_HID))
+		return AE_OK;
+
+	hid = info->hardware_id.string;
+	if (!hid)
+		return AE_OK;
+
+	for (i = 0; i < ARRAY_SIZE(acpi_smo8800_ids); ++i) {
+		if (strcmp(hid, acpi_smo8800_ids[i].id) == 0) {
+			*((bool *)return_value) = true;
Quoted 8 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1175

Quoted 6 lines
drivers/i2c/busses/i2c-i801.c +6 -0
+			return AE_CTRL_TERMINATE;
+		}
+	}
+
+	return AE_OK;
+

Unneeded blank line.

Ok.

Quoted 8 lines
Quoted 8 lines
drivers/i2c/busses/i2c-i801.c +8 -0
+}
+
+static bool is_dell_system_with_lis3lv02d(void)
+{
+	bool found;
+	acpi_status status;
+	const char *vendor;
+
Quoted 8 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1189

Quoted 6 lines
drivers/i2c/busses/i2c-i801.c +6 -0
+	vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+	if (strcmp(vendor, "Dell Inc.") != 0)
+		return false;
+
+	/*
+	 * Check that ACPI device SMO88xx exists and is enabled. That ACPI

I see how you check that the device exists, but not that it is enabled.

Hm.. you are right. I will add missing check.

Quoted 4 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1191

Quoted 2 lines
drivers/i2c/busses/i2c-i801.c +2 -0
+	 * device represent our ST microelectronics lis3lv02d accelerometer but
+	 * unfortunately without any other information (like i2c address).

I2C

I will change i2c to I2C in whole patch.

Quoted 13 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1199

Quoted 8 lines
drivers/i2c/busses/i2c-i801.c +8 -0
+	 */
+	found = false;
+	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
+				  (void **)&found);
+	if (!ACPI_SUCCESS(status) || !found)
+		return false;
+
+	return true;

Looks more complex than it needs to be. You don't really care about the status, as in the end you return the same on error as you do when no device is found. So I think you can simply go with:

Quoted 5 lines
	found = false;
	acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
				  (void **)&found);

	return found;

Ok, it really simplify that check.

Quoted 8 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1203

Quoted 5 lines
drivers/i2c/busses/i2c-i801.c +4 -0
+}
+
+/*
+ * Accelerometer's i2c address is not specified in DMI nor ACPI,

I2C

Quoted 5 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1206

Quoted 3 lines
drivers/i2c/busses/i2c-i801.c +3 -0
+ * so it is needed to define mapping table based on DMI product names.
+ */
+static struct {

Any reason to not make it const?

No, I will make it const.

Quoted 41 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1212

Quoted 6 lines
drivers/i2c/busses/i2c-i801.c +6 -0
+	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.

I2C

Quoted 19 lines
drivers/i2c/busses/i2c-i801.c +19 -0
+	 */
+	{ "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.
+	 */
+	{ "Vostro V131",        0x1d },
+};
+
+static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
+{
+	struct i2c_board_info info;
+	const char *dmi_product_name;
+	int i;
+

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1241

Quoted 10 lines
drivers/i2c/busses/i2c-i801.c +10 -0
+	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 i2c bus but its"

i2c bus -> SMBus

Quoted 5 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) · line 1242

Quoted 1 line
drivers/i2c/busses/i2c-i801.c +1 -0
+			 " i2c address is unknown, skipping registration...\n");

I2C (or just s/i2c //, as it's kind of redundant)

Suspension points not really needed IMHO.

Ok, it will be in V4 which I sent in few minutes.

Quoted 7 lines

drivers/i2c/busses/i2c-i801.c static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)

Quoted 3 lines
drivers/i2c/busses/i2c-i801.c +3 -0
+		return;
+	}
+ (...)

All the rest looks good to me.

Thanks,

Signature · 2 lines
Pali Rohár
[email protected]