Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1547586 > unrolled thread

[PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

Back to article view | Back to linux.kernel

Contents

  [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2016-12-27 14:00 +0100
    Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Wolfram Sang <wsa@the-dreams.de> - 2016-12-27 15:00 +0100
      Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Pali Rohár <pali.rohar@gmail.com> - 2016-12-27 15:00 +0100
        Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-27 23:20 +0100
          Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Valdis.Kletnieks@vt.edu - 2016-12-27 23:50 +0100
            Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Andy Shevchenko <andy.shevchenko@gmail.com> - 2016-12-28 09:00 +0100
              Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2016-12-28 10:10 +0100
                Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Wolfram Sang <wsa@the-dreams.de> - 2016-12-28 15:10 +0100
                  Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Valdis.Kletnieks@vt.edu - 2016-12-29 05:40 +0100
          Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2016-12-28 09:40 +0100
        Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Wolfram Sang <wsa@the-dreams.de> - 2016-12-28 15:10 +0100
    Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Michał Kępień <kernel@kempniu.pl> - 2016-12-29 09:30 +0100
      Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2016-12-29 10:10 +0100
        Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Michał Kępień <kernel@kempniu.pl> - 2016-12-29 14:50 +0100
          Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2016-12-29 15:20 +0100
            Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Michał Kępień <kernel@kempniu.pl> - 2016-12-29 22:10 +0100
              Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Pali Rohár <pali.rohar@gmail.com> - 2016-12-29 22:30 +0100
                Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Benjamin Tissoires <benjamin.tissoires@redhat.com> - 2017-01-03 10:10 +0100
                  Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on  Dell machines Pali Rohár <pali.rohar@gmail.com> - 2017-01-03 10:30 +0100

#1547586 — [PATCH] 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. That
i2c address is not specified in DMI or ACPI, so runtime detection without
whitelist which is below is not possible.

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

This patch registers lis3lv02d device at i2c address 0x29 if is 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 <pali.rohar@gmail.com>
---
 drivers/i2c/busses/i2c-i801.c |   98 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index eb3627f..188cfd4 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1118,6 +1118,101 @@ 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 if ACPI device SMO88xx exists and if is enabled. That ACPI
+	 * device represent our ST microelectronics lis3lv02d accelerometer but
+	 * unfortunately without any other additional information.
+	 */
+	found = false;
+	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
+				  (void **)&found);
+	if (!ACPI_SUCCESS(status) || !found)
+		return false;
+
+	return true;
+}
+
+/*
+ * Dell platform team told us that these Latitude devices have
+ * ST microelectronics accelerometer at i2c address 0x29.
+ * That i2c address is not specified in DMI or ACPI, so runtime
+ * detection without whitelist which is below is not possible.
+ */
+static const char * const dmi_dell_product_names[] = {
+	"Latitude E5250",
+	"Latitude E5450",
+	"Latitude E5550",
+	"Latitude E6440",
+	"Latitude E6440 ATG",
+	"Latitude E6540",
+};
+
+static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
+{
+	struct i2c_board_info info;
+	const char *product_name;
+	bool known_i2c_address;
+	int i;
+
+	known_i2c_address = false;
+	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+	for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
+		if (strcmp(product_name, dmi_dell_product_names[i]) == 0) {
+			known_i2c_address = true;
+			break;
+		}
+	}
+
+	if (!known_i2c_address) {
+		dev_warn(&priv->pci_dev->dev,
+			 "Accelerometer lis3lv02d i2c device is present "
+			 "but its i2c address is unknown, skipping ...\n");
+		return;
+	}
+
+	memset(&info, 0, sizeof(struct i2c_board_info));
+	info.addr = 0x29;
+	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)
 {
@@ -1136,6 +1231,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) {}
-- 
1.7.9.5

[toc] | [next] | [standalone]

#1547594 — Re: [PATCH] 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. 
> That
> i2c address is not specified in DMI or ACPI, so runtime detection 
> without
> whitelist which is below is not possible.

I'd think this should rather live somewhere in
drivers/platform/x86/dell*.c?

[toc] | [prev] | [next] | [standalone]

#1547595 — Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

[Multipart message — attachments visible in raw view] — view raw

On Tuesday 27 December 2016 14:43:49 Wolfram Sang wrote:
> > Dell platform team told us that some (DMI whitelisted) Dell
> > Latitude machines have ST microelectronics accelerometer at i2c
> > address 0x29. That
> > i2c address is not specified in DMI or ACPI, so runtime detection
> > without
> > whitelist which is below is not possible.
> 
> I'd think this should rather live somewhere in
> drivers/platform/x86/dell*.c?

i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called 
after initializing i2c-i801 bus driver.

I have no idea how to do it (properly) outside of i2c-i801.c file.

Same thing is done for Fujitsu machines, see function 
i801_probe_optional_slaves() in i2c-i801.c file. So I did similar 
approach for Dell machines.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1547768

On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Tuesday 27 December 2016 14:43:49 Wolfram Sang wrote:
>> > Dell platform team told us that some (DMI whitelisted) Dell
>> > Latitude machines have ST microelectronics accelerometer at i2c
>> > address 0x29. That
>> > i2c address is not specified in DMI or ACPI, so runtime detection
>> > without
>> > whitelist which is below is not possible.
>>
>> I'd think this should rather live somewhere in
>> drivers/platform/x86/dell*.c?
>
> i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called
> after initializing i2c-i801 bus driver.
>
> I have no idea how to do it (properly) outside of i2c-i801.c file.

I doubt we need a single line of code for this. See [1] and perhaps
create an EFI variable with necessary upgrade device node.

> Same thing is done for Fujitsu machines, see function
> i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
> approach for Dell machines.

Perhaps, this also needs to be converted to use EFI variable.

[1] https://lwn.net/Articles/693212/

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1547783

[Multipart message — attachments visible in raw view] — view raw

On Wed, 28 Dec 2016 00:15:30 +0200, Andy Shevchenko said:
> On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com> wrote:

> > I have no idea how to do it (properly) outside of i2c-i801.c file.
>
> I doubt we need a single line of code for this. See [1] and perhaps
> create an EFI variable with necessary upgrade device node.
>
> > Same thing is done for Fujitsu machines, see function
> > i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
> > approach for Dell machines.
>
> Perhaps, this also needs to be converted to use EFI variable.
>
> [1] https://lwn.net/Articles/693212/

There's no guarantee that the laptops in question are booted with UEFI,
as Dell still supports legacy boot. So assuming the presence of EFI variables
is somewhat problematic.

In addition, it requires the user (or something in userspace) to set the UEFI
variable or configfs tweak, rather than Just Working Out Of The Box.

[toc] | [prev] | [next] | [standalone]

#1547862

On Wed, Dec 28, 2016 at 12:41 AM,  <Valdis.Kletnieks@vt.edu> wrote:
> On Wed, 28 Dec 2016 00:15:30 +0200, Andy Shevchenko said:
>> On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
>
>> > I have no idea how to do it (properly) outside of i2c-i801.c file.
>>
>> I doubt we need a single line of code for this. See [1] and perhaps
>> create an EFI variable with necessary upgrade device node.
>>
>> > Same thing is done for Fujitsu machines, see function
>> > i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
>> > approach for Dell machines.
>>
>> Perhaps, this also needs to be converted to use EFI variable.
>>
>> [1] https://lwn.net/Articles/693212/
>
> There's no guarantee that the laptops in question are booted with UEFI,
> as Dell still supports legacy boot. So assuming the presence of EFI variables
> is somewhat problematic.


> In addition, it requires the user (or something in userspace) to set the UEFI
> variable or configfs tweak, rather than Just Working Out Of The Box.

I have no strong opinion, though I don't support the idea to put all
hacks in the world to the kernel. For example, we have user space tool
to switch USB modem from storage to actual communication device and
that is just working out of the box.

Mika, Darren, what are your opinions?

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1547889

[Multipart message — attachments visible in raw view] — view raw

On Wednesday 28 December 2016 08:55:18 Andy Shevchenko wrote:
> On Wed, Dec 28, 2016 at 12:41 AM,  <Valdis.Kletnieks@vt.edu> wrote:
> > On Wed, 28 Dec 2016 00:15:30 +0200, Andy Shevchenko said:
> >> On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> >> > I have no idea how to do it (properly) outside of i2c-i801.c file.
> >>
> >> I doubt we need a single line of code for this. See [1] and perhaps
> >> create an EFI variable with necessary upgrade device node.
> >>
> >> > Same thing is done for Fujitsu machines, see function
> >> > i801_probe_optional_slaves() in i2c-i801.c file. So I did similar
> >> > approach for Dell machines.
> >>
> >> Perhaps, this also needs to be converted to use EFI variable.
> >>
> >> [1] https://lwn.net/Articles/693212/
> >
> > There's no guarantee that the laptops in question are booted with UEFI,
> > as Dell still supports legacy boot. So assuming the presence of EFI variables
> > is somewhat problematic.
> 
> 
> > In addition, it requires the user (or something in userspace) to set the UEFI
> > variable or configfs tweak, rather than Just Working Out Of The Box.
> 
> I have no strong opinion, though I don't support the idea to put all
> hacks in the world to the kernel. For example, we have user space tool
> to switch USB modem from storage to actual communication device and
> that is just working out of the box.
> 
> Mika, Darren, what are your opinions?

I have absolutely no idea how to you want to achieve calling that i2c_new_device() registration 
without kernel patches.

So before starting discussion which option to use (EFI, kernel patch, userspace script, etc...) 
please describe how would you implement such logic with different options.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1547963 — Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

> I have absolutely no idea how to you want to achieve calling that 
> i2c_new_device() registration
> without kernel patches.

Documentation/i2c/instantiating-devices lists all supported methods.
Method 4 is userspace instantiation.

[toc] | [prev] | [next] | [standalone]

#1548167

[Multipart message — attachments visible in raw view] — view raw

On Wed, 28 Dec 2016 15:03:02 +0100, Wolfram Sang said:
> > I have absolutely no idea how to you want to achieve calling that
> > i2c_new_device() registration
> > without kernel patches.
>
> Documentation/i2c/instantiating-devices lists all supported methods.
> Method 4 is userspace instantiation.

I'd be totally OK with userspace doing it, except for the question "How good
will distros be about shipping it"?  I don't have any sense of how good Fedora
and Ubuntu and so on will be about making sure the userspace part is already
done for the user.

Anybody got evidence one way or another?

[toc] | [prev] | [next] | [standalone]

#1547875

[Multipart message — attachments visible in raw view] — view raw

On Tuesday 27 December 2016 23:15:30 Andy Shevchenko wrote:
> On Tue, Dec 27, 2016 at 3:51 PM, Pali Rohár <pali.rohar@gmail.com>
> wrote:
> > On Tuesday 27 December 2016 14:43:49 Wolfram Sang wrote:
> >> > Dell platform team told us that some (DMI whitelisted) Dell
> >> > Latitude machines have ST microelectronics accelerometer at i2c
> >> > address 0x29. That
> >> > i2c address is not specified in DMI or ACPI, so runtime
> >> > detection without
> >> > whitelist which is below is not possible.
> >> 
> >> I'd think this should rather live somewhere in
> >> drivers/platform/x86/dell*.c?
> > 
> > i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called
> > after initializing i2c-i801 bus driver.
> > 
> > I have no idea how to do it (properly) outside of i2c-i801.c file.
> 
> I doubt we need a single line of code for this. See [1] and perhaps
> create an EFI variable with necessary upgrade device node.

Sorry, but EFI variable is not accessible from BIOS booted kernel. So 
such thing will not work.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1547962 — Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

On Tue, Dec 27, 2016 at 02:51:01PM +0100, Pali Rohár wrote:
> On Tuesday 27 December 2016 14:43:49 Wolfram Sang wrote:
> > > Dell platform team told us that some (DMI whitelisted) Dell
> > > Latitude machines have ST microelectronics accelerometer at i2c
> > > address 0x29. That
> > > i2c address is not specified in DMI or ACPI, so runtime detection
> > > without
> > > whitelist which is below is not possible.
> >
> > I'd think this should rather live somewhere in
> > drivers/platform/x86/dell*.c?
>
> i2c_new_device() with lis3lv02d for i801 i2c bus needs to be called
> after initializing i2c-i801 bus driver.
>
> I have no idea how to do it (properly) outside of i2c-i801.c file.

I once used bus_notifiers to achieve something similar. You could check
arch/arm/mach-shmobile/regulator-quirk-rcar-gen2.c to see an action
triggered once a client device got added, but you could act on another
action like BUS_NOTIFY_BOUND_DRIVER. I used exactly that, too, somewhen
somewhere.  Haven't checked if that helps here, too. And since we have 
a
precedence (Fujitsu case), I'll leave it to Jean who is the maintainer
of this driver.

Thanks,

    Wolfram

[toc] | [prev] | [next] | [standalone]

#1548236 — Re: [PATCH] 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. That
> i2c address is not specified in DMI or ACPI, so runtime detection without
> whitelist which is below is not possible.
> 
> Presence of that ST microelectronics accelerometer is verified by existence
> of SMO88xx ACPI device which represent that accelerometer. Unfortunately
> without i2c address.

This part of the commit message sounded a bit confusing to me at first
because there is already an ACPI driver which handles SMO88xx devices
(dell-smo8800).  My understanding is that:

  * the purpose of this patch is to expose a richer interface (as
    provided by lis3lv02d) to these devices on some machines,

  * on whitelisted machines, dell-smo8800 and lis3lv02d can work
    simultaneously (even though dell-smo8800 effectively duplicates the
    work that lis3lv02d does).

If I got something wrong, please correct me.  If I got it right, it
might make sense to rephrase the commit message a bit so that the first
bullet point above is immediately clear to the reader.

> 
> This patch registers lis3lv02d device at i2c address 0x29 if is 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 <pali.rohar@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c |   98 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index eb3627f..188cfd4 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1118,6 +1118,101 @@ 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);

acpi_get_object_info() allocates the returned buffer, which the caller
has to free.

> +	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 if ACPI device SMO88xx exists and if is enabled. That ACPI
> +	 * device represent our ST microelectronics lis3lv02d accelerometer but
> +	 * unfortunately without any other additional information.
> +	 */
> +	found = false;
> +	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> +				  (void **)&found);
> +	if (!ACPI_SUCCESS(status) || !found)
> +		return false;
> +
> +	return true;
> +}
> +
> +/*
> + * Dell platform team told us that these Latitude devices have
> + * ST microelectronics accelerometer at i2c address 0x29.
> + * That i2c address is not specified in DMI or ACPI, so runtime
> + * detection without whitelist which is below is not possible.
> + */
> +static const char * const dmi_dell_product_names[] = {
> +	"Latitude E5250",
> +	"Latitude E5450",
> +	"Latitude E5550",
> +	"Latitude E6440",
> +	"Latitude E6440 ATG",
> +	"Latitude E6540",
> +};
> +
> +static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv)
> +{
> +	struct i2c_board_info info;
> +	const char *product_name;
> +	bool known_i2c_address;
> +	int i;
> +
> +	known_i2c_address = false;
> +	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> +	for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> +		if (strcmp(product_name, dmi_dell_product_names[i]) == 0) {
> +			known_i2c_address = true;
> +			break;
> +		}
> +	}
> +
> +	if (!known_i2c_address) {
> +		dev_warn(&priv->pci_dev->dev,
> +			 "Accelerometer lis3lv02d i2c device is present "
> +			 "but its i2c address is unknown, skipping ...\n");

You are probably well aware of this, but checkpatch prefers keeping long
log messages in one line.  I am pointing it out just in case.

> +		return;
> +	}
> +
> +	memset(&info, 0, sizeof(struct i2c_board_info));

How about just doing "struct i2c_board_info info = { 0 };" instead?

> +	info.addr = 0x29;
> +	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)
>  {
> @@ -1136,6 +1231,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) {}
> -- 
> 1.7.9.5
> 

I tested this patch on a Vostro V131, which is not on the whitelist, so
all I got was the warning message, but to this extent, it works for me.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1548254

[Multipart message — attachments visible in raw view] — view raw

On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > Dell platform team told us that some (DMI whitelisted) Dell
> > Latitude machines have ST microelectronics accelerometer at i2c
> > address 0x29. That i2c address is not specified in DMI or ACPI, so
> > runtime detection without whitelist which is below is not
> > possible.
> > 
> > Presence of that ST microelectronics accelerometer is verified by
> > existence of SMO88xx ACPI device which represent that
> > accelerometer. Unfortunately without i2c address.
> 
> This part of the commit message sounded a bit confusing to me at
> first because there is already an ACPI driver which handles SMO88xx
> devices (dell-smo8800).  My understanding is that:
> 
>   * the purpose of this patch is to expose a richer interface (as
>     provided by lis3lv02d) to these devices on some machines,
> 
>   * on whitelisted machines, dell-smo8800 and lis3lv02d can work
>     simultaneously (even though dell-smo8800 effectively duplicates
>     the work that lis3lv02d does).

No. dell-smo8800 reads from ACPI irq number and exports /dev/freefall 
device which notify userspace about falls. lis3lv02d is i2c driver which 
exports axes of accelerometer. Additionaly lis3lv02d can export also 
/dev/freefall if registerer of i2c device provides irq number -- which 
is not case of this patch.

So both drivers are doing different things and both are useful.

IIRC both dell-smo8800 and lis3lv02d represent one HW device (that ST 
microelectronics accelerometer) but due to complicated HW abstraction 
and layers on Dell laptops it is handled by two drivers, one ACPI and 
one i2c.

Yes, in ideal world irq number should be passed to lis3lv02d driver and 
that would export whole device (with /dev/freefall too), but due to HW 
abstraction it is too much complicated...

> If I got something wrong, please correct me.  If I got it right, it
> might make sense to rephrase the commit message a bit so that the
> first bullet point above is immediately clear to the reader.
> 
> > This patch registers lis3lv02d device at i2c address 0x29 if is
> > 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 <pali.rohar@gmail.com>
> > ---
> > 
> >  drivers/i2c/busses/i2c-i801.c |   98
> >  +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> >  insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-i801.c
> > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1118,6 +1118,101 @@ 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);
> 
> acpi_get_object_info() allocates the returned buffer, which the
> caller has to free.

Ok, I will fix it in next patch iteration.

> > +	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 if ACPI device SMO88xx exists and if is enabled. That
> > ACPI +	 * device represent our ST microelectronics lis3lv02d
> > accelerometer but +	 * unfortunately without any other additional
> > information. +	 */
> > +	found = false;
> > +	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > +				  (void **)&found);
> > +	if (!ACPI_SUCCESS(status) || !found)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * Dell platform team told us that these Latitude devices have
> > + * ST microelectronics accelerometer at i2c address 0x29.
> > + * That i2c address is not specified in DMI or ACPI, so runtime
> > + * detection without whitelist which is below is not possible.
> > + */
> > +static const char * const dmi_dell_product_names[] = {
> > +	"Latitude E5250",
> > +	"Latitude E5450",
> > +	"Latitude E5550",
> > +	"Latitude E6440",
> > +	"Latitude E6440 ATG",
> > +	"Latitude E6540",
> > +};
> > +
> > +static void register_dell_lis3lv02d_i2c_device(struct i801_priv
> > *priv) +{
> > +	struct i2c_board_info info;
> > +	const char *product_name;
> > +	bool known_i2c_address;
> > +	int i;
> > +
> > +	known_i2c_address = false;
> > +	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > +	for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > +		if (strcmp(product_name, dmi_dell_product_names[i]) == 0) {
> > +			known_i2c_address = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!known_i2c_address) {
> > +		dev_warn(&priv->pci_dev->dev,
> > +			 "Accelerometer lis3lv02d i2c device is present "
> > +			 "but its i2c address is unknown, skipping ...\n");
> 
> You are probably well aware of this, but checkpatch prefers keeping
> long log messages in one line.  I am pointing it out just in case.

Yes, but I do not know how to fix it. Splitting message into two lines 
generates warning. Having long line generates warning too.

> > +		return;
> > +	}
> > +
> > +	memset(&info, 0, sizeof(struct i2c_board_info));
> 
> How about just doing "struct i2c_board_info info = { 0 };" instead?

Ok.

> > +	info.addr = 0x29;
> > +	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)
> >  {
> > 
> > @@ -1136,6 +1231,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) {}
> 
> I tested this patch on a Vostro V131, which is not on the whitelist,
> so all I got was the warning message, but to this extent, it works
> for me.

Hm... That means your notebook has ST microelectronics accelerometer 
too. You could try to find it on i2c-i801 bus with userspace i2cdetect 
program (part of i2c-tools) and get i2c address. If it will work we can 
extend DMI name --> i2c address mapping and include your notebook too.

I have that list of confirmed Latitude devices since April 2015 but due 
to problem with ACPI resource conflicts it was not possible to load i2c-
i801.ko bus driver. It was fixed only this year by commit a7ae81952cda. 
So list of devices is probably not up-to-date and new appeared.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1548310 — Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

> On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > Dell platform team told us that some (DMI whitelisted) Dell
> > > Latitude machines have ST microelectronics accelerometer at i2c
> > > address 0x29. That i2c address is not specified in DMI or ACPI, so
> > > runtime detection without whitelist which is below is not
> > > possible.
> > > 
> > > Presence of that ST microelectronics accelerometer is verified by
> > > existence of SMO88xx ACPI device which represent that
> > > accelerometer. Unfortunately without i2c address.
> > 
> > This part of the commit message sounded a bit confusing to me at
> > first because there is already an ACPI driver which handles SMO88xx
> > devices (dell-smo8800).  My understanding is that:
> > 
> >   * the purpose of this patch is to expose a richer interface (as
> >     provided by lis3lv02d) to these devices on some machines,
> > 
> >   * on whitelisted machines, dell-smo8800 and lis3lv02d can work
> >     simultaneously (even though dell-smo8800 effectively duplicates
> >     the work that lis3lv02d does).
> 
> No. dell-smo8800 reads from ACPI irq number and exports /dev/freefall 
> device which notify userspace about falls. lis3lv02d is i2c driver which 
> exports axes of accelerometer. Additionaly lis3lv02d can export also 
> /dev/freefall if registerer of i2c device provides irq number -- which 
> is not case of this patch.
> 
> So both drivers are doing different things and both are useful.
> 
> IIRC both dell-smo8800 and lis3lv02d represent one HW device (that ST 
> microelectronics accelerometer) but due to complicated HW abstraction 
> and layers on Dell laptops it is handled by two drivers, one ACPI and 
> one i2c.
> 
> Yes, in ideal world irq number should be passed to lis3lv02d driver and 
> that would export whole device (with /dev/freefall too), but due to HW 
> abstraction it is too much complicated...

Why?  AFAICT, all that is required to pass that IRQ number all the way
down to lis3lv02d is to set the irq field of the struct i2c_board_info
you are passing to i2c_new_device().  And you can extract that IRQ
number e.g. in check_acpi_smo88xx_device().  However, you would then
need to make sure dell-smo8800 does not attempt to request the same IRQ
on whitelisted machines.  This got me thinking about a way to somehow
incorporate your changes into dell-smo8800 using Wolfram's bus_notifier
suggestion, but I do not have a working solution for now.  What is
tempting about this approach is that you would not have to scan the ACPI
namespace in search of SMO88xx devices, because smo8800_add() is
automatically called for them.  However, I fear that the resulting
solution may be more complicated than the one you submitted.

> 
> > If I got something wrong, please correct me.  If I got it right, it
> > might make sense to rephrase the commit message a bit so that the
> > first bullet point above is immediately clear to the reader.
> > 
> > > This patch registers lis3lv02d device at i2c address 0x29 if is
> > > 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 <pali.rohar@gmail.com>
> > > ---
> > > 
> > >  drivers/i2c/busses/i2c-i801.c |   98
> > >  +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> > >  insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > > --- a/drivers/i2c/busses/i2c-i801.c
> > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > @@ -1118,6 +1118,101 @@ 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);
> > 
> > acpi_get_object_info() allocates the returned buffer, which the
> > caller has to free.
> 
> Ok, I will fix it in next patch iteration.
> 
> > > +	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 if ACPI device SMO88xx exists and if is enabled. That
> > > ACPI +	 * device represent our ST microelectronics lis3lv02d
> > > accelerometer but +	 * unfortunately without any other additional
> > > information. +	 */
> > > +	found = false;
> > > +	status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > > +				  (void **)&found);
> > > +	if (!ACPI_SUCCESS(status) || !found)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +/*
> > > + * Dell platform team told us that these Latitude devices have
> > > + * ST microelectronics accelerometer at i2c address 0x29.
> > > + * That i2c address is not specified in DMI or ACPI, so runtime
> > > + * detection without whitelist which is below is not possible.
> > > + */
> > > +static const char * const dmi_dell_product_names[] = {
> > > +	"Latitude E5250",
> > > +	"Latitude E5450",
> > > +	"Latitude E5550",
> > > +	"Latitude E6440",
> > > +	"Latitude E6440 ATG",
> > > +	"Latitude E6540",
> > > +};
> > > +
> > > +static void register_dell_lis3lv02d_i2c_device(struct i801_priv
> > > *priv) +{
> > > +	struct i2c_board_info info;
> > > +	const char *product_name;
> > > +	bool known_i2c_address;
> > > +	int i;
> > > +
> > > +	known_i2c_address = false;
> > > +	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > +	for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > > +		if (strcmp(product_name, dmi_dell_product_names[i]) == 0) {
> > > +			known_i2c_address = true;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (!known_i2c_address) {
> > > +		dev_warn(&priv->pci_dev->dev,
> > > +			 "Accelerometer lis3lv02d i2c device is present "
> > > +			 "but its i2c address is unknown, skipping ...\n");
> > 
> > You are probably well aware of this, but checkpatch prefers keeping
> > long log messages in one line.  I am pointing it out just in case.
> 
> Yes, but I do not know how to fix it. Splitting message into two lines 
> generates warning. Having long line generates warning too.

Weird, checkpatch does not protest on my machine when the log message is
written on a single line...

> 
> > > +		return;
> > > +	}
> > > +
> > > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > 
> > How about just doing "struct i2c_board_info info = { 0 };" instead?
> 
> Ok.
> 
> > > +	info.addr = 0x29;
> > > +	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)
> > >  {
> > > 
> > > @@ -1136,6 +1231,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) {}
> > 
> > I tested this patch on a Vostro V131, which is not on the whitelist,
> > so all I got was the warning message, but to this extent, it works
> > for me.
> 
> Hm... That means your notebook has ST microelectronics accelerometer 
> too. You could try to find it on i2c-i801 bus with userspace i2cdetect 
> program (part of i2c-tools) and get i2c address.

Bingo, it is at 0x1d.  I modified your patch to set the i2c address to
0x1d and at least free fall detection seems to be working correctly.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1548317

[Multipart message — attachments visible in raw view] — view raw

On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > Latitude machines have ST microelectronics accelerometer at i2c
> > > > address 0x29. That i2c address is not specified in DMI or ACPI,
> > > > so runtime detection without whitelist which is below is not
> > > > possible.
> > > > 
> > > > Presence of that ST microelectronics accelerometer is verified
> > > > by existence of SMO88xx ACPI device which represent that
> > > > accelerometer. Unfortunately without i2c address.
> > > 
> > > This part of the commit message sounded a bit confusing to me at
> > > first because there is already an ACPI driver which handles
> > > SMO88xx
> > > 
> > > devices (dell-smo8800).  My understanding is that:
> > >   * the purpose of this patch is to expose a richer interface (as
> > >   
> > >     provided by lis3lv02d) to these devices on some machines,
> > >   
> > >   * on whitelisted machines, dell-smo8800 and lis3lv02d can work
> > >   
> > >     simultaneously (even though dell-smo8800 effectively
> > >     duplicates the work that lis3lv02d does).
> > 
> > No. dell-smo8800 reads from ACPI irq number and exports
> > /dev/freefall device which notify userspace about falls. lis3lv02d
> > is i2c driver which exports axes of accelerometer. Additionaly
> > lis3lv02d can export also /dev/freefall if registerer of i2c
> > device provides irq number -- which is not case of this patch.
> > 
> > So both drivers are doing different things and both are useful.
> > 
> > IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> > ST microelectronics accelerometer) but due to complicated HW
> > abstraction and layers on Dell laptops it is handled by two
> > drivers, one ACPI and one i2c.
> > 
> > Yes, in ideal world irq number should be passed to lis3lv02d driver
> > and that would export whole device (with /dev/freefall too), but
> > due to HW abstraction it is too much complicated...
> 
> Why?  AFAICT, all that is required to pass that IRQ number all the
> way down to lis3lv02d is to set the irq field of the struct
> i2c_board_info you are passing to i2c_new_device().  And you can
> extract that IRQ number e.g. in check_acpi_smo88xx_device(). 
> However, you would then need to make sure dell-smo8800 does not
> attempt to request the same IRQ on whitelisted machines.  This got
> me thinking about a way to somehow incorporate your changes into
> dell-smo8800 using Wolfram's bus_notifier suggestion, but I do not
> have a working solution for now.  What is tempting about this
> approach is that you would not have to scan the ACPI namespace in
> search of SMO88xx devices, because smo8800_add() is automatically
> called for them.  However, I fear that the resulting solution may be
> more complicated than the one you submitted.

Then we need to deal with lot of problems. Order of loading .ko modules 
is undefined. Binding devices to drivers registered by .ko module is 
also in "random" order. At any time any of those .ko module can be 
unloaded or at least device unbind (via sysfs) from driver... And there 
can be some pathological situation (thanks to adding ACPI layer as Andy 
pointed) that there will be more SMO88xx devices in ACPI. Plus you can 
compile kernel with and without those modules and also you can blacklist 
loading them (so compile time check is not enough). And still some 
correct message notifier must be used.

I think such solution is much much more complicated, there are lot of 
combinations of kernel configuration and available dell devices...

> > > If I got something wrong, please correct me.  If I got it right,
> > > it might make sense to rephrase the commit message a bit so that
> > > the first bullet point above is immediately clear to the reader.
> > > 
> > > > This patch registers lis3lv02d device at i2c address 0x29 if is
> > > > 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 <pali.rohar@gmail.com>
> > > > ---
> > > > 
> > > >  drivers/i2c/busses/i2c-i801.c |   98
> > > >  +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> > > >  insertions(+)
> > > > 
> > > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > > > --- a/drivers/i2c/busses/i2c-i801.c
> > > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > > @@ -1118,6 +1118,101 @@ 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);
> > > 
> > > acpi_get_object_info() allocates the returned buffer, which the
> > > caller has to free.
> > 
> > Ok, I will fix it in next patch iteration.
> > 
> > > > +	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 if ACPI device SMO88xx exists and if is enabled.
> > > > That ACPI +	 * device represent our ST microelectronics
> > > > lis3lv02d accelerometer but +	 * unfortunately without any
> > > > other additional information. +	 */
> > > > +	found = false;
> > > > +	status = acpi_get_devices(NULL, check_acpi_smo88xx_device,
> > > > NULL, +				  (void **)&found);
> > > > +	if (!ACPI_SUCCESS(status) || !found)
> > > > +		return false;
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Dell platform team told us that these Latitude devices have
> > > > + * ST microelectronics accelerometer at i2c address 0x29.
> > > > + * That i2c address is not specified in DMI or ACPI, so
> > > > runtime + * detection without whitelist which is below is not
> > > > possible. + */
> > > > +static const char * const dmi_dell_product_names[] = {
> > > > +	"Latitude E5250",
> > > > +	"Latitude E5450",
> > > > +	"Latitude E5550",
> > > > +	"Latitude E6440",
> > > > +	"Latitude E6440 ATG",
> > > > +	"Latitude E6540",
> > > > +};
> > > > +
> > > > +static void register_dell_lis3lv02d_i2c_device(struct
> > > > i801_priv *priv) +{
> > > > +	struct i2c_board_info info;
> > > > +	const char *product_name;
> > > > +	bool known_i2c_address;
> > > > +	int i;
> > > > +
> > > > +	known_i2c_address = false;
> > > > +	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > > +	for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > > > +		if (strcmp(product_name, dmi_dell_product_names[i]) == 0)
> > > > {
> > > > +			known_i2c_address = true;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (!known_i2c_address) {
> > > > +		dev_warn(&priv->pci_dev->dev,
> > > > +			 "Accelerometer lis3lv02d i2c device is present "
> > > > +			 "but its i2c address is unknown, skipping ...\n");
> > > 
> > > You are probably well aware of this, but checkpatch prefers
> > > keeping long log messages in one line.  I am pointing it out
> > > just in case.
> > 
> > Yes, but I do not know how to fix it. Splitting message into two
> > lines generates warning. Having long line generates warning too.
> 
> Weird, checkpatch does not protest on my machine when the log message
> is written on a single line...

I hope that i2c maintainers decide how to format that line.

> > > > +		return;
> > > > +	}
> > > > +
> > > > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > > 
> > > How about just doing "struct i2c_board_info info = { 0 };"
> > > instead?
> > 
> > Ok.
> > 
> > > > +	info.addr = 0x29;
> > > > +	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)
> > > >  {
> > > > 
> > > > @@ -1136,6 +1231,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) {}
> > > 
> > > I tested this patch on a Vostro V131, which is not on the
> > > whitelist, so all I got was the warning message, but to this
> > > extent, it works for me.
> > 
> > Hm... That means your notebook has ST microelectronics
> > accelerometer too. You could try to find it on i2c-i801 bus with
> > userspace i2cdetect program (part of i2c-tools) and get i2c
> > address.
> 
> Bingo, it is at 0x1d.  I modified your patch to set the i2c address
> to 0x1d and at least free fall detection seems to be working
> correctly.

lis3lv02d exports input device, you should find its number lsinput. You 
can then test accelerometer with e.g. program input-events.

If it is working fine, I can add your machine to whitelist with i2c 
address 0x1d.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1548367 — Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

> On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > > Latitude machines have ST microelectronics accelerometer at i2c
> > > > > address 0x29. That i2c address is not specified in DMI or ACPI,
> > > > > so runtime detection without whitelist which is below is not
> > > > > possible.
> > > > > 
> > > > > Presence of that ST microelectronics accelerometer is verified
> > > > > by existence of SMO88xx ACPI device which represent that
> > > > > accelerometer. Unfortunately without i2c address.
> > > > 
> > > > This part of the commit message sounded a bit confusing to me at
> > > > first because there is already an ACPI driver which handles
> > > > SMO88xx
> > > > 
> > > > devices (dell-smo8800).  My understanding is that:
> > > >   * the purpose of this patch is to expose a richer interface (as
> > > >   
> > > >     provided by lis3lv02d) to these devices on some machines,
> > > >   
> > > >   * on whitelisted machines, dell-smo8800 and lis3lv02d can work
> > > >   
> > > >     simultaneously (even though dell-smo8800 effectively
> > > >     duplicates the work that lis3lv02d does).
> > > 
> > > No. dell-smo8800 reads from ACPI irq number and exports
> > > /dev/freefall device which notify userspace about falls. lis3lv02d
> > > is i2c driver which exports axes of accelerometer. Additionaly
> > > lis3lv02d can export also /dev/freefall if registerer of i2c
> > > device provides irq number -- which is not case of this patch.
> > > 
> > > So both drivers are doing different things and both are useful.
> > > 
> > > IIRC both dell-smo8800 and lis3lv02d represent one HW device (that
> > > ST microelectronics accelerometer) but due to complicated HW
> > > abstraction and layers on Dell laptops it is handled by two
> > > drivers, one ACPI and one i2c.
> > > 
> > > Yes, in ideal world irq number should be passed to lis3lv02d driver
> > > and that would export whole device (with /dev/freefall too), but
> > > due to HW abstraction it is too much complicated...
> > 
> > Why?  AFAICT, all that is required to pass that IRQ number all the
> > way down to lis3lv02d is to set the irq field of the struct
> > i2c_board_info you are passing to i2c_new_device().  And you can
> > extract that IRQ number e.g. in check_acpi_smo88xx_device(). 
> > However, you would then need to make sure dell-smo8800 does not
> > attempt to request the same IRQ on whitelisted machines.  This got
> > me thinking about a way to somehow incorporate your changes into
> > dell-smo8800 using Wolfram's bus_notifier suggestion, but I do not
> > have a working solution for now.  What is tempting about this
> > approach is that you would not have to scan the ACPI namespace in
> > search of SMO88xx devices, because smo8800_add() is automatically
> > called for them.  However, I fear that the resulting solution may be
> > more complicated than the one you submitted.
> 
> Then we need to deal with lot of problems. Order of loading .ko modules 
> is undefined. Binding devices to drivers registered by .ko module is 
> also in "random" order. At any time any of those .ko module can be 
> unloaded or at least device unbind (via sysfs) from driver... And there 
> can be some pathological situation (thanks to adding ACPI layer as Andy 
> pointed) that there will be more SMO88xx devices in ACPI. Plus you can 
> compile kernel with and without those modules and also you can blacklist 
> loading them (so compile time check is not enough). And still some 
> correct message notifier must be used.
> 
> I think such solution is much much more complicated, there are lot of 
> combinations of kernel configuration and available dell devices...

I tried a few more things, but ultimately failed to find a nice way to
implement this.

Another issue popped up, though.  Linus' master branch contains a recent
commit by Benjamin Tissoires (CC'ed), 4d5538f5882a ("i2c: use an IRQ to
report Host Notify events, not alert") which breaks your patch.  The
reason for that is that lis3lv02d relies on the i2c client's IRQ being 0
to detect that it should not create /dev/freefall.  Benjamin's patch
causes the Host Notify IRQ to be assigned to the i2c client your patch
creates, thus causing lis3lv02d to create /dev/freefall, which in turn
conflicts with dell-smo8800 which is trying to create /dev/freefall
itself.
        
Also, just to make sure we do not overthink this, I understand that not
every unit of the models from the whitelist has an accelerometer,
correct?  In other words, could we perhaps skip the part where we are
making sure the SMO88xx ACPI device is there?

> 
> > > > If I got something wrong, please correct me.  If I got it right,
> > > > it might make sense to rephrase the commit message a bit so that
> > > > the first bullet point above is immediately clear to the reader.
> > > > 
> > > > > This patch registers lis3lv02d device at i2c address 0x29 if is
> > > > > 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 <pali.rohar@gmail.com>
> > > > > ---
> > > > > 
> > > > >  drivers/i2c/busses/i2c-i801.c |   98
> > > > >  +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 98
> > > > >  insertions(+)
> > > > > 
> > > > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4 100644
> > > > > --- a/drivers/i2c/busses/i2c-i801.c
> > > > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > > > @@ -1118,6 +1118,101 @@ 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);
> > > > 
> > > > acpi_get_object_info() allocates the returned buffer, which the
> > > > caller has to free.
> > > 
> > > Ok, I will fix it in next patch iteration.
> > > 
> > > > > +	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 if ACPI device SMO88xx exists and if is enabled.
> > > > > That ACPI +	 * device represent our ST microelectronics
> > > > > lis3lv02d accelerometer but +	 * unfortunately without any
> > > > > other additional information. +	 */
> > > > > +	found = false;
> > > > > +	status = acpi_get_devices(NULL, check_acpi_smo88xx_device,
> > > > > NULL, +				  (void **)&found);
> > > > > +	if (!ACPI_SUCCESS(status) || !found)
> > > > > +		return false;
> > > > > +
> > > > > +	return true;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * Dell platform team told us that these Latitude devices have
> > > > > + * ST microelectronics accelerometer at i2c address 0x29.
> > > > > + * That i2c address is not specified in DMI or ACPI, so
> > > > > runtime + * detection without whitelist which is below is not
> > > > > possible. + */
> > > > > +static const char * const dmi_dell_product_names[] = {
> > > > > +	"Latitude E5250",
> > > > > +	"Latitude E5450",
> > > > > +	"Latitude E5550",
> > > > > +	"Latitude E6440",
> > > > > +	"Latitude E6440 ATG",
> > > > > +	"Latitude E6540",
> > > > > +};
> > > > > +
> > > > > +static void register_dell_lis3lv02d_i2c_device(struct
> > > > > i801_priv *priv) +{
> > > > > +	struct i2c_board_info info;
> > > > > +	const char *product_name;
> > > > > +	bool known_i2c_address;
> > > > > +	int i;
> > > > > +
> > > > > +	known_i2c_address = false;
> > > > > +	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > > > +	for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) {
> > > > > +		if (strcmp(product_name, dmi_dell_product_names[i]) == 0)
> > > > > {
> > > > > +			known_i2c_address = true;
> > > > > +			break;
> > > > > +		}
> > > > > +	}
> > > > > +
> > > > > +	if (!known_i2c_address) {
> > > > > +		dev_warn(&priv->pci_dev->dev,
> > > > > +			 "Accelerometer lis3lv02d i2c device is present "
> > > > > +			 "but its i2c address is unknown, skipping ...\n");
> > > > 
> > > > You are probably well aware of this, but checkpatch prefers
> > > > keeping long log messages in one line.  I am pointing it out
> > > > just in case.
> > > 
> > > Yes, but I do not know how to fix it. Splitting message into two
> > > lines generates warning. Having long line generates warning too.
> > 
> > Weird, checkpatch does not protest on my machine when the log message
> > is written on a single line...
> 
> I hope that i2c maintainers decide how to format that line.
> 
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > > > 
> > > > How about just doing "struct i2c_board_info info = { 0 };"
> > > > instead?
> > > 
> > > Ok.
> > > 
> > > > > +	info.addr = 0x29;
> > > > > +	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)
> > > > >  {
> > > > > 
> > > > > @@ -1136,6 +1231,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) {}
> > > > 
> > > > I tested this patch on a Vostro V131, which is not on the
> > > > whitelist, so all I got was the warning message, but to this
> > > > extent, it works for me.
> > > 
> > > Hm... That means your notebook has ST microelectronics
> > > accelerometer too. You could try to find it on i2c-i801 bus with
> > > userspace i2cdetect program (part of i2c-tools) and get i2c
> > > address.
> > 
> > Bingo, it is at 0x1d.  I modified your patch to set the i2c address
> > to 0x1d and at least free fall detection seems to be working
> > correctly.
> 
> lis3lv02d exports input device, you should find its number lsinput. You 
> can then test accelerometer with e.g. program input-events.
> 
> If it is working fine, I can add your machine to whitelist with i2c 
> address 0x1d.

I did some tests with evtest and it seems that axis values are
consistent with laptop's movements, so I think it is safe to whitelist
Vostro V131 with i2c address 0x1d.

-- 
Best regards,
Michał Kępień

[toc] | [prev] | [next] | [standalone]

#1548371

[Multipart message — attachments visible in raw view] — view raw

On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > > > Latitude machines have ST microelectronics accelerometer at
> > > > > > i2c address 0x29. That i2c address is not specified in DMI
> > > > > > or ACPI, so runtime detection without whitelist which is
> > > > > > below is not possible.
> > > > > > 
> > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > represent that accelerometer. Unfortunately without i2c
> > > > > > address.
> > > > > 
> > > > > This part of the commit message sounded a bit confusing to me
> > > > > at first because there is already an ACPI driver which
> > > > > handles SMO88xx
> > > > > 
> > > > > devices (dell-smo8800).  My understanding is that:
> > > > >   * the purpose of this patch is to expose a richer interface
> > > > >   (as
> > > > >   
> > > > >     provided by lis3lv02d) to these devices on some machines,
> > > > >   
> > > > >   * on whitelisted machines, dell-smo8800 and lis3lv02d can
> > > > >   work
> > > > >   
> > > > >     simultaneously (even though dell-smo8800 effectively
> > > > >     duplicates the work that lis3lv02d does).
> > > > 
> > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > /dev/freefall device which notify userspace about falls.
> > > > lis3lv02d is i2c driver which exports axes of accelerometer.
> > > > Additionaly lis3lv02d can export also /dev/freefall if
> > > > registerer of i2c device provides irq number -- which is not
> > > > case of this patch.
> > > > 
> > > > So both drivers are doing different things and both are useful.
> > > > 
> > > > IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > (that ST microelectronics accelerometer) but due to
> > > > complicated HW abstraction and layers on Dell laptops it is
> > > > handled by two drivers, one ACPI and one i2c.
> > > > 
> > > > Yes, in ideal world irq number should be passed to lis3lv02d
> > > > driver and that would export whole device (with /dev/freefall
> > > > too), but due to HW abstraction it is too much complicated...
> > > 
> > > Why?  AFAICT, all that is required to pass that IRQ number all
> > > the way down to lis3lv02d is to set the irq field of the struct
> > > i2c_board_info you are passing to i2c_new_device().  And you can
> > > extract that IRQ number e.g. in check_acpi_smo88xx_device().
> > > However, you would then need to make sure dell-smo8800 does not
> > > attempt to request the same IRQ on whitelisted machines.  This
> > > got me thinking about a way to somehow incorporate your changes
> > > into dell-smo8800 using Wolfram's bus_notifier suggestion, but I
> > > do not have a working solution for now.  What is tempting about
> > > this approach is that you would not have to scan the ACPI
> > > namespace in search of SMO88xx devices, because smo8800_add() is
> > > automatically called for them.  However, I fear that the
> > > resulting solution may be more complicated than the one you
> > > submitted.
> > 
> > Then we need to deal with lot of problems. Order of loading .ko
> > modules is undefined. Binding devices to drivers registered by .ko
> > module is also in "random" order. At any time any of those .ko
> > module can be unloaded or at least device unbind (via sysfs) from
> > driver... And there can be some pathological situation (thanks to
> > adding ACPI layer as Andy pointed) that there will be more SMO88xx
> > devices in ACPI. Plus you can compile kernel with and without
> > those modules and also you can blacklist loading them (so compile
> > time check is not enough). And still some correct message notifier
> > must be used.
> > 
> > I think such solution is much much more complicated, there are lot
> > of combinations of kernel configuration and available dell
> > devices...
> 
> I tried a few more things, but ultimately failed to find a nice way
> to implement this.
> 
> Another issue popped up, though.  Linus' master branch contains a
> recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a ("i2c: use
> an IRQ to report Host Notify events, not alert") which breaks your
> patch.  The reason for that is that lis3lv02d relies on the i2c
> client's IRQ being 0 to detect that it should not create
> /dev/freefall.  Benjamin's patch causes the Host Notify IRQ to be
> assigned to the i2c client your patch creates, thus causing
> lis3lv02d to create /dev/freefall, which in turn conflicts with
> dell-smo8800 which is trying to create /dev/freefall itself.

So 4d5538f5882a is breaking lis3lv02d driver...

> Also, just to make sure we do not overthink this, I understand that
> not every unit of the models from the whitelist has an
> accelerometer, correct?  In other words, could we perhaps skip the
> part where we are making sure the SMO88xx ACPI device is there?

Good question... At least for E6440 I'm did not thing it was possible to 
configure notebook without "3 axes free fall sensor".

But! In BIOS SETUP it is possible to disable free fall sensor. I will 
try to disable it there and will check what happen. My guess is that it 
will be disabled in ACPI.

> > > > > If I got something wrong, please correct me.  If I got it
> > > > > right, it might make sense to rephrase the commit message a
> > > > > bit so that the first bullet point above is immediately
> > > > > clear to the reader.
> > > > > 
> > > > > > This patch registers lis3lv02d device at i2c address 0x29
> > > > > > if is 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 <pali.rohar@gmail.com>
> > > > > > ---
> > > > > > 
> > > > > >  drivers/i2c/busses/i2c-i801.c |   98
> > > > > >  +++++++++++++++++++++++++++++++++++++++++ 1 file changed,
> > > > > >  98 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/i2c/busses/i2c-i801.c
> > > > > > b/drivers/i2c/busses/i2c-i801.c index eb3627f..188cfd4
> > > > > > 100644 --- a/drivers/i2c/busses/i2c-i801.c
> > > > > > +++ b/drivers/i2c/busses/i2c-i801.c
> > > > > > @@ -1118,6 +1118,101 @@ 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);
> > > > > 
> > > > > acpi_get_object_info() allocates the returned buffer, which
> > > > > the caller has to free.
> > > > 
> > > > Ok, I will fix it in next patch iteration.
> > > > 
> > > > > > +	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 if ACPI device SMO88xx exists and if is enabled.
> > > > > > That ACPI +	 * device represent our ST microelectronics
> > > > > > lis3lv02d accelerometer but +	 * unfortunately without any
> > > > > > other additional information. +	 */
> > > > > > +	found = false;
> > > > > > +	status = acpi_get_devices(NULL,
> > > > > > check_acpi_smo88xx_device, NULL, +				  (void
> > > > > > **)&found);
> > > > > > +	if (!ACPI_SUCCESS(status) || !found)
> > > > > > +		return false;
> > > > > > +
> > > > > > +	return true;
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * Dell platform team told us that these Latitude devices
> > > > > > have + * ST microelectronics accelerometer at i2c address
> > > > > > 0x29. + * That i2c address is not specified in DMI or
> > > > > > ACPI, so runtime + * detection without whitelist which is
> > > > > > below is not possible. + */
> > > > > > +static const char * const dmi_dell_product_names[] = {
> > > > > > +	"Latitude E5250",
> > > > > > +	"Latitude E5450",
> > > > > > +	"Latitude E5550",
> > > > > > +	"Latitude E6440",
> > > > > > +	"Latitude E6440 ATG",
> > > > > > +	"Latitude E6540",
> > > > > > +};
> > > > > > +
> > > > > > +static void register_dell_lis3lv02d_i2c_device(struct
> > > > > > i801_priv *priv) +{
> > > > > > +	struct i2c_board_info info;
> > > > > > +	const char *product_name;
> > > > > > +	bool known_i2c_address;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	known_i2c_address = false;
> > > > > > +	product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
> > > > > > +	for (i = 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i)
> > > > > > { +		if (strcmp(product_name, dmi_dell_product_names[i])
> > > > > > == 0) {
> > > > > > +			known_i2c_address = true;
> > > > > > +			break;
> > > > > > +		}
> > > > > > +	}
> > > > > > +
> > > > > > +	if (!known_i2c_address) {
> > > > > > +		dev_warn(&priv->pci_dev->dev,
> > > > > > +			 "Accelerometer lis3lv02d i2c device is present "
> > > > > > +			 "but its i2c address is unknown, skipping ...
> > > > > > \n");
> > > > > 
> > > > > You are probably well aware of this, but checkpatch prefers
> > > > > keeping long log messages in one line.  I am pointing it out
> > > > > just in case.
> > > > 
> > > > Yes, but I do not know how to fix it. Splitting message into
> > > > two lines generates warning. Having long line generates
> > > > warning too.
> > > 
> > > Weird, checkpatch does not protest on my machine when the log
> > > message is written on a single line...
> > 
> > I hope that i2c maintainers decide how to format that line.
> > 
> > > > > > +		return;
> > > > > > +	}
> > > > > > +
> > > > > > +	memset(&info, 0, sizeof(struct i2c_board_info));
> > > > > 
> > > > > How about just doing "struct i2c_board_info info = { 0 };"
> > > > > instead?
> > > > 
> > > > Ok.
> > > > 
> > > > > > +	info.addr = 0x29;
> > > > > > +	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) {
> > > > > > 
> > > > > > @@ -1136,6 +1231,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) {}
> > > > > 
> > > > > I tested this patch on a Vostro V131, which is not on the
> > > > > whitelist, so all I got was the warning message, but to this
> > > > > extent, it works for me.
> > > > 
> > > > Hm... That means your notebook has ST microelectronics
> > > > accelerometer too. You could try to find it on i2c-i801 bus
> > > > with userspace i2cdetect program (part of i2c-tools) and get
> > > > i2c address.
> > > 
> > > Bingo, it is at 0x1d.  I modified your patch to set the i2c
> > > address to 0x1d and at least free fall detection seems to be
> > > working correctly.
> > 
> > lis3lv02d exports input device, you should find its number lsinput.
> > You can then test accelerometer with e.g. program input-events.
> > 
> > If it is working fine, I can add your machine to whitelist with i2c
> > address 0x1d.
> 
> I did some tests with evtest and it seems that axis values are
> consistent with laptop's movements, so I think it is safe to
> whitelist Vostro V131 with i2c address 0x1d.

Ok.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [next] | [standalone]

#1549628 — Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > On Thursday 29 December 2016 14:47:19 Michał Kępień wrote:
> > > > > On Thursday 29 December 2016 09:29:36 Michał Kępień wrote:
> > > > > > > Dell platform team told us that some (DMI whitelisted) Dell
> > > > > > > Latitude machines have ST microelectronics accelerometer at
> > > > > > > i2c address 0x29. That i2c address is not specified in DMI
> > > > > > > or ACPI, so runtime detection without whitelist which is
> > > > > > > below is not possible.
> > > > > > > 
> > > > > > > Presence of that ST microelectronics accelerometer is
> > > > > > > verified by existence of SMO88xx ACPI device which
> > > > > > > represent that accelerometer. Unfortunately without i2c
> > > > > > > address.
> > > > > > 
> > > > > > This part of the commit message sounded a bit confusing to me
> > > > > > at first because there is already an ACPI driver which
> > > > > > handles SMO88xx
> > > > > > 
> > > > > > devices (dell-smo8800).  My understanding is that:
> > > > > >   * the purpose of this patch is to expose a richer interface
> > > > > >   (as
> > > > > >   
> > > > > >     provided by lis3lv02d) to these devices on some machines,
> > > > > >   
> > > > > >   * on whitelisted machines, dell-smo8800 and lis3lv02d can
> > > > > >   work
> > > > > >   
> > > > > >     simultaneously (even though dell-smo8800 effectively
> > > > > >     duplicates the work that lis3lv02d does).
> > > > > 
> > > > > No. dell-smo8800 reads from ACPI irq number and exports
> > > > > /dev/freefall device which notify userspace about falls.
> > > > > lis3lv02d is i2c driver which exports axes of accelerometer.
> > > > > Additionaly lis3lv02d can export also /dev/freefall if
> > > > > registerer of i2c device provides irq number -- which is not
> > > > > case of this patch.
> > > > > 
> > > > > So both drivers are doing different things and both are useful.
> > > > > 
> > > > > IIRC both dell-smo8800 and lis3lv02d represent one HW device
> > > > > (that ST microelectronics accelerometer) but due to
> > > > > complicated HW abstraction and layers on Dell laptops it is
> > > > > handled by two drivers, one ACPI and one i2c.
> > > > > 
> > > > > Yes, in ideal world irq number should be passed to lis3lv02d
> > > > > driver and that would export whole device (with /dev/freefall
> > > > > too), but due to HW abstraction it is too much complicated...
> > > > 
> > > > Why?  AFAICT, all that is required to pass that IRQ number all
> > > > the way down to lis3lv02d is to set the irq field of the struct
> > > > i2c_board_info you are passing to i2c_new_device().  And you can
> > > > extract that IRQ number e.g. in check_acpi_smo88xx_device().
> > > > However, you would then need to make sure dell-smo8800 does not
> > > > attempt to request the same IRQ on whitelisted machines.  This
> > > > got me thinking about a way to somehow incorporate your changes
> > > > into dell-smo8800 using Wolfram's bus_notifier suggestion, but I
> > > > do not have a working solution for now.  What is tempting about
> > > > this approach is that you would not have to scan the ACPI
> > > > namespace in search of SMO88xx devices, because smo8800_add() is
> > > > automatically called for them.  However, I fear that the
> > > > resulting solution may be more complicated than the one you
> > > > submitted.
> > > 
> > > Then we need to deal with lot of problems. Order of loading .ko
> > > modules is undefined. Binding devices to drivers registered by .ko
> > > module is also in "random" order. At any time any of those .ko
> > > module can be unloaded or at least device unbind (via sysfs) from
> > > driver... And there can be some pathological situation (thanks to
> > > adding ACPI layer as Andy pointed) that there will be more SMO88xx
> > > devices in ACPI. Plus you can compile kernel with and without
> > > those modules and also you can blacklist loading them (so compile
> > > time check is not enough). And still some correct message notifier
> > > must be used.
> > > 
> > > I think such solution is much much more complicated, there are lot
> > > of combinations of kernel configuration and available dell
> > > devices...
> > 
> > I tried a few more things, but ultimately failed to find a nice way
> > to implement this.
> > 
> > Another issue popped up, though.  Linus' master branch contains a
> > recent commit by Benjamin Tissoires (CC'ed), 4d5538f5882a ("i2c: use
> > an IRQ to report Host Notify events, not alert") which breaks your
> > patch.  The reason for that is that lis3lv02d relies on the i2c
> > client's IRQ being 0 to detect that it should not create
> > /dev/freefall.  Benjamin's patch causes the Host Notify IRQ to be
> > assigned to the i2c client your patch creates, thus causing
> > lis3lv02d to create /dev/freefall, which in turn conflicts with
> > dell-smo8800 which is trying to create /dev/freefall itself.
> 
> So 4d5538f5882a is breaking lis3lv02d driver...

Apologies for that.

I could easily fix this by adding a kernel API to know whether the
provided irq is from Host Notify or if it was coming from an actual
declaration. However, I have no idea how many other drivers would
require this (hopefully only this one).

One other solution would be to reserve the Host Notify IRQ and let the
actual drivers that need it to set it, but this was not the best
solution according to Dmitri. On my side, I am not entirely against this
given that it's a chip feature, so the driver should be able to know
that it's available.

Dmitri, Wolfram, Jean, any preferences?

> 
> > Also, just to make sure we do not overthink this, I understand that
> > not every unit of the models from the whitelist has an
> > accelerometer, correct?  In other words, could we perhaps skip the
> > part where we are making sure the SMO88xx ACPI device is there?
> 
> Good question... At least for E6440 I'm did not thing it was possible to 
> configure notebook without "3 axes free fall sensor".
> 
> But! In BIOS SETUP it is possible to disable free fall sensor. I will 
> try to disable it there and will check what happen. My guess is that it 
> will be disabled in ACPI.

Just adding my 2 cents regarding the whitelist and interaction between
those 2 drivers. I find this very fragile to have only one available
/dev/freefall node and to rely on the fairness of each driver to not bind
one. It would have been much simpler to have /dev/freefallXX and a
proper misc class device for it. This way, we don't even need to
mutually exclude the drivers. But this is already 8 years old code, so I
guess userspace expects this... (why isn't that using the input subsystem
at all?).

Cheers,
Benjamin.

[toc] | [prev] | [next] | [standalone]

#1549644 — Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines

On Tuesday 03 January 2017 10:06:41 Benjamin Tissoires wrote:
> On Dec 29 2016 or thereabouts, Pali Rohár wrote:
> > On Thursday 29 December 2016 22:09:32 Michał Kępień wrote:
> > > Also, just to make sure we do not overthink this, I understand that
> > > not every unit of the models from the whitelist has an
> > > accelerometer, correct?  In other words, could we perhaps skip the
> > > part where we are making sure the SMO88xx ACPI device is there?
> > 
> > Good question... At least for E6440 I'm did not thing it was possible to 
> > configure notebook without "3 axes free fall sensor".
> > 
> > But! In BIOS SETUP it is possible to disable free fall sensor. I will 
> > try to disable it there and will check what happen. My guess is that it 
> > will be disabled in ACPI.
> 
> Just adding my 2 cents regarding the whitelist and interaction between
> those 2 drivers. I find this very fragile to have only one available
> /dev/freefall node and to rely on the fairness of each driver to not bind
> one. It would have been much simpler to have /dev/freefallXX and a
> proper misc class device for it. This way, we don't even need to
> mutually exclude the drivers. But this is already 8 years old code, so I
> guess userspace expects this... (why isn't that using the input subsystem
> at all?).
> 
> Cheers,
> Benjamin.
> 

I think there is no problem with more /dev/freefall devices. With these
Dell drivers it should not happen as only one driver can request IRQ
which is associated with /dev/freefall. And /dev/freefal is registered
after acquiring IRQ.

But... there are other problems with it as wrote in previous emails.

-- 
Pali Rohár
pali.rohar@gmail.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web