From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751572AbcL2V23 (ORCPT ); Thu, 29 Dec 2016 16:28:29 -0500 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34286 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751276AbcL2V2W (ORCPT ); Thu, 29 Dec 2016 16:28:22 -0500 From: Pali =?utf-8?q?Roh=C3=A1r?= To: =?utf-8?q?Micha=C5=82_K=C4=99pie=C5=84?= Subject: Re: [PATCH] i2c: i801: Register optional lis3lv02d i2c device on Dell machines Date: Thu, 29 Dec 2016 22:28:18 +0100 User-Agent: KMail/1.13.7 (Linux/4.9.0-040900-generic; KDE/4.14.2; x86_64; ; ) Cc: Jean Delvare , Wolfram Sang , Steven Honeyman , Valdis.Kletnieks@vt.edu, Jochen Eisinger , Gabriele Mazzotta , Andy Lutomirski , Mario_Limonciello@dell.com, Alex Hung , Takashi Iwai , Benjamin Tissoires , linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, platform-driver-x86@vger.kernel.org References: <1482843136-12838-1-git-send-email-pali.rohar@gmail.com> <201612291517.37474@pali> <20161229210932.GA1254@kmp-mobile.hq.kempniu.pl> In-Reply-To: <20161229210932.GA1254@kmp-mobile.hq.kempniu.pl> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart41206958.7hQB71mveW"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Message-Id: <201612292228.18706@pali> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --nextPart41206958.7hQB71mveW Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On Thursday 29 December 2016 22:09:32 Micha=C5=82 K=C4=99pie=C5=84 wrote: > > On Thursday 29 December 2016 14:47:19 Micha=C5=82 K=C4=99pie=C5=84 wrot= e: > > > > On Thursday 29 December 2016 09:29:36 Micha=C5=82 K=C4=99pie=C5=84 = 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. > > > > > >=20 > > > > > > Presence of that ST microelectronics accelerometer is > > > > > > verified by existence of SMO88xx ACPI device which > > > > > > represent that accelerometer. Unfortunately without i2c > > > > > > address. > > > > >=20 > > > > > This part of the commit message sounded a bit confusing to me > > > > > at first because there is already an ACPI driver which > > > > > handles SMO88xx > > > > >=20 > > > > > devices (dell-smo8800). My understanding is that: > > > > > * the purpose of this patch is to expose a richer interface > > > > > (as > > > > > =20 > > > > > provided by lis3lv02d) to these devices on some machines, > > > > > =20 > > > > > * on whitelisted machines, dell-smo8800 and lis3lv02d can > > > > > work > > > > > =20 > > > > > simultaneously (even though dell-smo8800 effectively > > > > > duplicates the work that lis3lv02d does). > > > >=20 > > > > 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. > > > >=20 > > > > So both drivers are doing different things and both are useful. > > > >=20 > > > > 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. > > > >=20 > > > > 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... > > >=20 > > > 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. > >=20 > > 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. > >=20 > > I think such solution is much much more complicated, there are lot > > of combinations of kernel configuration and available dell > > devices... >=20 > I tried a few more things, but ultimately failed to find a nice way > to implement this. >=20 > 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=20 configure notebook without "3 axes free fall sensor". But! In BIOS SETUP it is possible to disable free fall sensor. I will=20 try to disable it there and will check what happen. My guess is that it=20 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. > > > > >=20 > > > > > > This patch registers lis3lv02d device at i2c address 0x29 > > > > > > if is detected. > > > > > >=20 > > > > > > 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. > > > > > >=20 > > > > > > Tested on Dell Latitude E6440. > > > > > >=20 > > > > > > Signed-off-by: Pali Roh=C3=A1r > > > > > > --- > > > > > >=20 > > > > > > drivers/i2c/busses/i2c-i801.c | 98 > > > > > > +++++++++++++++++++++++++++++++++++++++++ 1 file changed, > > > > > > 98 insertions(+) > > > > > >=20 > > > > > > 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) > > > > > >=20 > > > > > > } > > > > > > =20 > > > > > > } > > > > > >=20 > > > > > > +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 =3D acpi_get_object_info(obj_handle, &info); > > > > >=20 > > > > > acpi_get_object_info() allocates the returned buffer, which > > > > > the caller has to free. > > > >=20 > > > > Ok, I will fix it in next patch iteration. > > > >=20 > > > > > > + if (!ACPI_SUCCESS(status) || !(info->valid & > > > > > > ACPI_VALID_HID)) + return AE_OK; > > > > > > + > > > > > > + hid =3D info->hardware_id.string; > > > > > > + if (!hid) > > > > > > + return AE_OK; > > > > > > + > > > > > > + if (strlen(hid) < 7) > > > > > > + return AE_OK; > > > > > > + > > > > > > + if (memcmp(hid, "SMO88", 5) !=3D 0) > > > > > > + return AE_OK; > > > > > > + > > > > > > + *((bool *)return_value) =3D true; > > > > > > + return AE_CTRL_TERMINATE; > > > > > > +} > > > > > > + > > > > > > +static bool is_dell_system_with_lis3lv02d(void) > > > > > > +{ > > > > > > + bool found; > > > > > > + acpi_status status; > > > > > > + const char *vendor; > > > > > > + > > > > > > + vendor =3D dmi_get_system_info(DMI_SYS_VENDOR); > > > > > > + if (strcmp(vendor, "Dell Inc.") !=3D 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 =3D false; > > > > > > + status =3D 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[] =3D { > > > > > > + "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 =3D false; > > > > > > + product_name =3D dmi_get_system_info(DMI_PRODUCT_NAME); > > > > > > + for (i =3D 0; i < ARRAY_SIZE(dmi_dell_product_names); ++i) > > > > > > { + if (strcmp(product_name, dmi_dell_product_names[i]) > > > > > > =3D=3D 0) { > > > > > > + known_i2c_address =3D 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"); > > > > >=20 > > > > > 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. > > > >=20 > > > > Yes, but I do not know how to fix it. Splitting message into > > > > two lines generates warning. Having long line generates > > > > warning too. > > >=20 > > > Weird, checkpatch does not protest on my machine when the log > > > message is written on a single line... > >=20 > > I hope that i2c maintainers decide how to format that line. > >=20 > > > > > > + return; > > > > > > + } > > > > > > + > > > > > > + memset(&info, 0, sizeof(struct i2c_board_info)); > > > > >=20 > > > > > How about just doing "struct i2c_board_info info =3D { 0 };" > > > > > instead? > > > >=20 > > > > Ok. > > > >=20 > > > > > > + info.addr =3D 0x29; > > > > > > + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE); > > > > > > + i2c_new_device(&priv->adapter, &info); > > > > > > +} > > > > > > + > > > > > >=20 > > > > > > /* Register optional slaves */ > > > > > > static void i801_probe_optional_slaves(struct i801_priv > > > > > > *priv) { > > > > > >=20 > > > > > > @@ -1136,6 +1231,9 @@ static void > > > > > > i801_probe_optional_slaves(struct i801_priv *priv) > > > > > >=20 > > > > > > if (dmi_name_in_vendors("FUJITSU")) > > > > > > =09 > > > > > > dmi_walk(dmi_check_onboard_devices, &priv->adapter); > > > > > >=20 > > > > > > + > > > > > > + if (is_dell_system_with_lis3lv02d()) > > > > > > + register_dell_lis3lv02d_i2c_device(priv); > > > > > >=20 > > > > > > } > > > > > > #else > > > > > > static void __init input_apanel_init(void) {} > > > > >=20 > > > > > 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. > > > >=20 > > > > 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. > > >=20 > > > 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. > >=20 > > lis3lv02d exports input device, you should find its number lsinput. > > You can then test accelerometer with e.g. program input-events. > >=20 > > If it is working fine, I can add your machine to whitelist with i2c > > address 0x1d. >=20 > 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. =2D-=20 Pali Roh=C3=A1r pali.rohar@gmail.com --nextPart41206958.7hQB71mveW Content-Type: application/pgp-signature; name=signature.asc Content-Description: This is a digitally signed message part. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iEYEABECAAYFAlhlf/IACgkQi/DJPQPkQ1JdlwCfY4er2yBBdb9hbn8VqGIW9F64 /o8AniJ1cfPBfDKEf856DceMDBxiErqk =e9j+ -----END PGP SIGNATURE----- --nextPart41206958.7hQB71mveW--