| Message ID | 20180127133209.28995-1-pali.rohar@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines | expand |
On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > Dell platform team told us that some (DMI whitelisted) Dell Latitude > machines have ST microelectronics accelerometer at i2c address 0x29. > > Presence of that ST microelectronics accelerometer is verified by existence > of SMO88xx ACPI device which represent that accelerometer. Unfortunately > ACPI device does not specify i2c address. > > This patch registers lis3lv02d device for selected Dell Latitude machines > at i2c address 0x29 after detection. And for Dell Vostro V131 machine at > i2c address 0x1d which was manually detected. > > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to > conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so > lis3lv02d correctly initialize accelerometer. > > Tested on Dell Latitude E6440. > +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) > +{ > + /* > + * Check that ACPI device SMO88xx exists and is enabled. That ACPI > + * device represent our ST microelectronics lis3lv02d accelerometer but > + * unfortunately without any other information (like i2c address). > + */ Isn't it simple return acpi_dev_present("SMO88", NULL, -1); call? > + found = false; > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, > + (void **)&found); > + if (!ACPI_SUCCESS(status) || !found) > + return false; > + > + return true; > +}
On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote: > On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > +static bool is_dell_system_with_lis3lv02d(void) > > +{ > > > + /* > > + * Check that ACPI device SMO88xx exists and is enabled. That ACPI > > + * device represent our ST microelectronics lis3lv02d accelerometer but > > + * unfortunately without any other information (like i2c address). > > + */ > > Isn't it simple > > return acpi_dev_present("SMO88", NULL, -1); > > call? ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present function match only prefix and not exact string? > > + found = false; > > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, > > + (void **)&found); > > + if (!ACPI_SUCCESS(status) || !found) > > + return false; > > + > > + return true; > > +} > >
On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote: >> On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> > +static bool is_dell_system_with_lis3lv02d(void) >> > +{ >> >> > + /* >> > + * Check that ACPI device SMO88xx exists and is enabled. That ACPI >> > + * device represent our ST microelectronics lis3lv02d accelerometer but >> > + * unfortunately without any other information (like i2c address). >> > + */ >> >> Isn't it simple >> >> return acpi_dev_present("SMO88", NULL, -1); >> >> call? > > ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present > function match only prefix and not exact string? OK, fair enough. Do we have more users of such pattern? If so, it might make sense to introduce a generic helper for that which takes a list of HIDs on input. (Yes, I do not like matching pattern like "XYZhh*", I prefer explicit list of HIDs. Rationale to do so: a) any new potential collision is excluded, b) we can easily grep kernel for a users per HID)
On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: > On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote: > >> On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > >> > +static bool is_dell_system_with_lis3lv02d(void) > >> > +{ > >> > >> > + /* > >> > + * Check that ACPI device SMO88xx exists and is enabled. That ACPI > >> > + * device represent our ST microelectronics lis3lv02d accelerometer but > >> > + * unfortunately without any other information (like i2c address). > >> > + */ > >> > >> Isn't it simple > >> > >> return acpi_dev_present("SMO88", NULL, -1); > >> > >> call? > > > > ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present > > function match only prefix and not exact string? > > OK, fair enough. > > Do we have more users of such pattern? I have not seen this ACPI pattern yet, so probably not.
On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> > ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present >> > function match only prefix and not exact string? >> >> OK, fair enough. >> >> Do we have more users of such pattern? > > I have not seen this ACPI pattern yet, so probably not. I see. So, my one concern is the implicit names of the devices. I would like rather to see ... acpi_device_id ... []= { {"SMO8800"}, {"SMO8810"}, ... {} };
> Dell platform team told us that some (DMI whitelisted) Dell Latitude > machines have ST microelectronics accelerometer at i2c address 0x29. > > Presence of that ST microelectronics accelerometer is verified by existence > of SMO88xx ACPI device which represent that accelerometer. Unfortunately > ACPI device does not specify i2c address. > > This patch registers lis3lv02d device for selected Dell Latitude machines > at i2c address 0x29 after detection. And for Dell Vostro V131 machine at > i2c address 0x1d which was manually detected. > > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to > conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so > lis3lv02d correctly initialize accelerometer. > > Tested on Dell Latitude E6440. > > Signed-off-by: Pali Rohár <pali.rohar@gmail.com> > --- > Changes since v1: > * Added Dell Vostro V131 based on Michał Kępień testing I tested this patch on a Vostro V131 and it seems to work as expected: "lis3lv02d: 8 bits sensor found" is logged during boot, an input device gets registered and its axis values seem to be consistent with laptop's movements. Tested-by: Michał Kępień <kernel@kempniu.pl> I did not review the contents of the patch this time, sorry.
On Mon, Feb 12, 2018 at 12:03 AM, Michał Kępień <kernel@kempniu.pl> wrote: >> Signed-off-by: Pali Rohár <pali.rohar@gmail.com> >> --- >> Changes since v1: >> * Added Dell Vostro V131 based on Michał Kępień testing > > I tested this patch on a Vostro V131 and it seems to work as expected: > "lis3lv02d: 8 bits sensor found" is logged during boot, an input device > gets registered and its axis values seem to be consistent with laptop's > movements. > > Tested-by: Michał Kępień <kernel@kempniu.pl> Thanks! I'm waiting for Pali to address my concern. > I did not review the contents of the patch this time, sorry.
On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: > On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: > >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > >> > ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present > >> > function match only prefix and not exact string? > >> > >> OK, fair enough. > >> > >> Do we have more users of such pattern? > > > > I have not seen this ACPI pattern yet, so probably not. > > I see. So, my one concern is the implicit names of the devices. I > would like rather to see > > ... acpi_device_id ... []= { > {"SMO8800"}, > {"SMO8810"}, > ... > {} > }; Following table already exists in dell-smo8800.c file: static const struct acpi_device_id smo8800_ids[] = { { "SMO8800", 0 }, { "SMO8801", 0 }, { "SMO8810", 0 }, { "SMO8811", 0 }, { "SMO8820", 0 }, { "SMO8821", 0 }, { "SMO8830", 0 }, { "SMO8831", 0 }, { "", 0 }, }; MODULE_DEVICE_TABLE(acpi, smo8800_ids); Can we reuse it? Maybe moving array smo8800_ids[] into some header file (which one?) and statically inline it? Or having it only in dell-smo8800.c file and exporting its symbol? Or is there better idea? For sure I do not want to copy paste this table into another module and maintaining two copies of this list.
On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> >> >> > ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present >> >> > function match only prefix and not exact string? >> >> >> >> OK, fair enough. >> >> >> >> Do we have more users of such pattern? >> > >> > I have not seen this ACPI pattern yet, so probably not. >> >> I see. So, my one concern is the implicit names of the devices. I >> would like rather to see >> >> ... acpi_device_id ... []= { >> {"SMO8800"}, >> {"SMO8810"}, >> ... >> {} >> }; > > Following table already exists in dell-smo8800.c file: > > static const struct acpi_device_id smo8800_ids[] = { > { "SMO8800", 0 }, > { "SMO8801", 0 }, > { "SMO8810", 0 }, > { "SMO8811", 0 }, > { "SMO8820", 0 }, > { "SMO8821", 0 }, > { "SMO8830", 0 }, > { "SMO8831", 0 }, > { "", 0 }, > }; > > MODULE_DEVICE_TABLE(acpi, smo8800_ids); > > Can we reuse it? > Maybe moving array smo8800_ids[] into some header file > (which one?) and statically inline it? Bad idea. > Or having it only in > dell-smo8800.c file and exporting its symbol? Even worse. > Or is there better idea? > > For sure I do not want to copy paste this table into another module and > maintaining two copies of this list. The copy is fine. Can you guarantee that those two lists would be always the same? I'm not. And besides that explicitly over implicitly is a really good thing. I would not like to grep for an ID followed by grepping include line and check each files to check if it uses it or not.
On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote: > On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: > >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: > >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > >> > >> >> > ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present > >> >> > function match only prefix and not exact string? > >> >> > >> >> OK, fair enough. > >> >> > >> >> Do we have more users of such pattern? > >> > > >> > I have not seen this ACPI pattern yet, so probably not. > >> > >> I see. So, my one concern is the implicit names of the devices. I > >> would like rather to see > >> > >> ... acpi_device_id ... []= { > >> {"SMO8800"}, > >> {"SMO8810"}, > >> ... > >> {} > >> }; > > > > Following table already exists in dell-smo8800.c file: > > > > static const struct acpi_device_id smo8800_ids[] = { > > { "SMO8800", 0 }, > > { "SMO8801", 0 }, > > { "SMO8810", 0 }, > > { "SMO8811", 0 }, > > { "SMO8820", 0 }, > > { "SMO8821", 0 }, > > { "SMO8830", 0 }, > > { "SMO8831", 0 }, > > { "", 0 }, > > }; > > > > MODULE_DEVICE_TABLE(acpi, smo8800_ids); > > > > Can we reuse it? > > > Maybe moving array smo8800_ids[] into some header file > > (which one?) and statically inline it? > > Bad idea. > > > Or having it only in > > dell-smo8800.c file and exporting its symbol? > > Even worse. > > > Or is there better idea? > > > > For sure I do not want to copy paste this table into another module and > > maintaining two copies of this list. > > The copy is fine. Can you guarantee that those two lists would be > always the same? I'm not. Me neither. > And besides that explicitly over implicitly is a really good thing. I > would not like to grep for an ID followed by grepping include line and > check each files to check if it uses it or not. So what do you suggest now? Having one file where it would be defined is a bad idea for you. And maintaining copy of same array in two different files in two different subsystems is something which I cannot guarantee. Therefore the current patch is the best approach. No shared file with shared array/table and also no copy of that array in two different subsystems.
On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote: >> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: >> >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: >> >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> >> >> >> >> > ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present >> >> >> > function match only prefix and not exact string? >> >> >> >> >> >> OK, fair enough. >> >> >> >> >> >> Do we have more users of such pattern? >> >> > >> >> > I have not seen this ACPI pattern yet, so probably not. >> >> >> >> I see. So, my one concern is the implicit names of the devices. I >> >> would like rather to see >> >> >> >> ... acpi_device_id ... []= { >> >> {"SMO8800"}, >> >> {"SMO8810"}, >> >> ... >> >> {} >> >> }; >> > >> > Following table already exists in dell-smo8800.c file: >> > >> > static const struct acpi_device_id smo8800_ids[] = { >> > { "SMO8800", 0 }, >> > { "SMO8801", 0 }, >> > { "SMO8810", 0 }, >> > { "SMO8811", 0 }, >> > { "SMO8820", 0 }, >> > { "SMO8821", 0 }, >> > { "SMO8830", 0 }, >> > { "SMO8831", 0 }, >> > { "", 0 }, >> > }; >> > >> > MODULE_DEVICE_TABLE(acpi, smo8800_ids); >> > >> > Can we reuse it? >> >> > Maybe moving array smo8800_ids[] into some header file >> > (which one?) and statically inline it? >> >> Bad idea. >> >> > Or having it only in >> > dell-smo8800.c file and exporting its symbol? >> >> Even worse. >> >> > Or is there better idea? >> > >> > For sure I do not want to copy paste this table into another module and >> > maintaining two copies of this list. >> >> The copy is fine. Can you guarantee that those two lists would be >> always the same? I'm not. > > Me neither. > >> And besides that explicitly over implicitly is a really good thing. I >> would not like to grep for an ID followed by grepping include line and >> check each files to check if it uses it or not. > > So what do you suggest now? Copy'n'paste and maintain two lists. Yes, it's not the ideal, but working solution. You may put a comment before each list to explain what the second does and tell a contributor to look at it and update if needed. > Having one file where it would be defined is a bad idea for you. Not just "one file", but "one *header* file". Or "exporting a symbol" which is basically not supposed to be exported. ID tables are part of the actual drivers, neither headers, nor libraries. > And maintaining copy of same array in two different files in two > different subsystems is something which I cannot guarantee. > > Therefore the current patch is the best approach. I don't like it. I'll not take it, sorry. > No shared file with > shared array/table and also no copy of that array in two different > subsystems.
On Tuesday 13 February 2018 17:06:19 Andy Shevchenko wrote: > On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > > On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote: > >> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > >> > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: > >> >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > >> >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: > >> >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > >> >> > >> >> >> > ACPI device name is SMO8800, SMO8810, ... Will that acpi_dev_present > >> >> >> > function match only prefix and not exact string? > >> >> >> > >> >> >> OK, fair enough. > >> >> >> > >> >> >> Do we have more users of such pattern? > >> >> > > >> >> > I have not seen this ACPI pattern yet, so probably not. > >> >> > >> >> I see. So, my one concern is the implicit names of the devices. I > >> >> would like rather to see > >> >> > >> >> ... acpi_device_id ... []= { > >> >> {"SMO8800"}, > >> >> {"SMO8810"}, > >> >> ... > >> >> {} > >> >> }; > >> > > >> > Following table already exists in dell-smo8800.c file: > >> > > >> > static const struct acpi_device_id smo8800_ids[] = { > >> > { "SMO8800", 0 }, > >> > { "SMO8801", 0 }, > >> > { "SMO8810", 0 }, > >> > { "SMO8811", 0 }, > >> > { "SMO8820", 0 }, > >> > { "SMO8821", 0 }, > >> > { "SMO8830", 0 }, > >> > { "SMO8831", 0 }, > >> > { "", 0 }, > >> > }; > >> > > >> > MODULE_DEVICE_TABLE(acpi, smo8800_ids); > >> > > >> > Can we reuse it? > >> > >> > Maybe moving array smo8800_ids[] into some header file > >> > (which one?) and statically inline it? > >> > >> Bad idea. > >> > >> > Or having it only in > >> > dell-smo8800.c file and exporting its symbol? > >> > >> Even worse. > >> > >> > Or is there better idea? > >> > > >> > For sure I do not want to copy paste this table into another module and > >> > maintaining two copies of this list. > >> > >> The copy is fine. Can you guarantee that those two lists would be > >> always the same? I'm not. > > > > Me neither. > > > >> And besides that explicitly over implicitly is a really good thing. I > >> would not like to grep for an ID followed by grepping include line and > >> check each files to check if it uses it or not. > > > > So what do you suggest now? > > Copy'n'paste and maintain two lists. > Yes, it's not the ideal, but working solution. > > You may put a comment before each list to explain what the second does > and tell a contributor to look at it and update if needed. I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. Therefore instructing future contributors would be up to them. Jean, it is OK? > > Having one file where it would be defined is a bad idea for you. > > Not just "one file", but "one *header* file". Or "exporting a symbol" > which is basically not supposed to be exported. > ID tables are part of the actual drivers, neither headers, nor libraries. But this is exactly what is needed. This ACPI ID table contains ACPI names which says if accelerometer is present or not. > > And maintaining copy of same array in two different files in two > > different subsystems is something which I cannot guarantee. > > > > Therefore the current patch is the best approach. > > I don't like it. I'll not take it, sorry. > > > No shared file with > > shared array/table and also no copy of that array in two different > > subsystems. > >
On Tue, Feb 13, 2018 at 6:50 PM, Pali Rohár <pali.rohar@gmail.com> wrote: > On Tuesday 13 February 2018 17:06:19 Andy Shevchenko wrote: >> On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> > On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote: >> >> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <pali.rohar@gmail.com> wrote: >> >> > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: >> >> > Following table already exists in dell-smo8800.c file: >> >> > Can we reuse it? >> >> > Maybe moving array smo8800_ids[] into some header file >> >> > (which one?) and statically inline it? >> >> >> >> Bad idea. >> >> >> >> > Or having it only in >> >> > dell-smo8800.c file and exporting its symbol? >> >> >> >> Even worse. >> >> >> >> > Or is there better idea? >> >> > >> >> > For sure I do not want to copy paste this table into another module and >> >> > maintaining two copies of this list. >> >> >> >> The copy is fine. Can you guarantee that those two lists would be >> >> always the same? I'm not. >> > >> > Me neither. >> > >> >> And besides that explicitly over implicitly is a really good thing. I >> >> would not like to grep for an ID followed by grepping include line and >> >> check each files to check if it uses it or not. >> > >> > So what do you suggest now? >> >> Copy'n'paste and maintain two lists. >> Yes, it's not the ideal, but working solution. >> >> You may put a comment before each list to explain what the second does >> and tell a contributor to look at it and update if needed. > > I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. > Therefore instructing future contributors would be up to them. Right. But from ACPI prospective the proposed change is not okay by my opinion. How can I see what drivers are binding / relying on certain ACPI ID? More precisely, % git grep -n -w SMO8800 would be useless until one gets an idea to match against partial strings. > Jean, it is OK? >> > Having one file where it would be defined is a bad idea for you. >> >> Not just "one file", but "one *header* file". Or "exporting a symbol" >> which is basically not supposed to be exported. >> ID tables are part of the actual drivers, neither headers, nor libraries. > > But this is exactly what is needed. This ACPI ID table contains ACPI > names which says if accelerometer is present or not. In case of dell-smo8800.c the driver actually *binds* to these IDs. Your change just provide another table to match and answer to the question if XYZ is present on a such platform or not. Similar approach is used in spi-pxa2xx.c (check pxa2xx_spi_pci_compound_match).
On Tue, Feb 13, 2018 at 7:01 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> Similar approach is used in spi-pxa2xx.c (check pxa2xx_spi_pci_compound_match).
Another approach might be a registration of I2C board info from
dell-smo8800.c. Not sure if it better, because theoretically user may
disable that one, but leave i2c-i801 enabled.
> I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. > Therefore instructing future contributors would be up to them. This is really Jean's realm.
On Monday 26 February 2018 21:32:55 Wolfram Sang wrote: > > > I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. > > Therefore instructing future contributors would be up to them. > > This is really Jean's realm. > Jean, ping.
On Thursday 08 March 2018 23:53:30 Pali Rohár wrote: > On Monday 26 February 2018 21:32:55 Wolfram Sang wrote: > > > > > I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. > > > Therefore instructing future contributors would be up to them. > > > > This is really Jean's realm. > > > > Jean, ping. Jean, gently ping again. Waiting for your maintainer response/decision on this problem.
On Wednesday 18 April 2018 13:46:10 Pali Rohár wrote: > On Thursday 08 March 2018 23:53:30 Pali Rohár wrote: > > On Monday 26 February 2018 21:32:55 Wolfram Sang wrote: > > > > > > > I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. > > > > Therefore instructing future contributors would be up to them. > > > > > > This is really Jean's realm. > > > > > > > Jean, ping. > > Jean, gently ping again. Waiting for your maintainer response/decision > on this problem. Jean, one year passed without any answer. May you response to this thread about this patch?
On Mon, 26 Feb 2018 21:32:55 +0100, Wolfram Sang wrote: > > I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. > > Therefore instructing future contributors would be up to them. > > This is really Jean's realm. Sorry for the delay. As a general rule I'm all in favor of instantiating I2C devices from i2c-i801 when we can, as it makes the user's life easier. However I agree with Andy that: 1* We want to have an explicit list of supported ACPI device IDs, not a just a prefix. 2* We don't want to over-engineer it with a common header file or an exported symbol. I see no problem with duplicating the lists if 2 drivers happen to be needed on the same set of devices. This is easily managed by adding a comment before each list that the other list may need to be kept in sync. It also gives us the flexibility to *not* keep them in sync if needed. Instantiating the I2C device from dell-smo8800 doesn't seem practical because that driver has no idea about the i2c subsystem in the first place. What worries me is that we seem to have 2 drivers binding to the same device (the accelerometer), one natively (lis3lv02d), and one through an ACPI layer (dell-smo8800). I don't really understand why this is needed (don't they serve the same purpose?) nor how it can be safe (what guarantees that both drivers won't attempt to access the hardware at the same time?)
Hi! Thank you finally for response! On Tuesday 28 May 2019 11:19:53 Jean Delvare wrote: > On Mon, 26 Feb 2018 21:32:55 +0100, Wolfram Sang wrote: > > > I'm not maintainer of i2c-i801.ko, Jean Delvare & Wolfram Sang are. > > > Therefore instructing future contributors would be up to them. > > > > This is really Jean's realm. > > Sorry for the delay. As a general rule I'm all in favor of > instantiating I2C devices from i2c-i801 when we can, as it makes the > user's life easier. However I agree with Andy that: > 1* We want to have an explicit list of supported ACPI device IDs, not a > just a prefix. > 2* We don't want to over-engineer it with a common header file or an > exported symbol. I see no problem with duplicating the lists if 2 > drivers happen to be needed on the same set of devices. This is > easily managed by adding a comment before each list that the other > list may need to be kept in sync. It also gives us the flexibility > to *not* keep them in sync if needed. Ok, I will then make list of supported ACPI devices and put them into two places with comment about syncing. > Instantiating the I2C device from dell-smo8800 doesn't seem practical > because that driver has no idea about the i2c subsystem in the first > place. Yes. And in same way instantiating ACPI driver dell-smo8800 from i2c-i801 is not practical too. > What worries me is that we seem to have 2 drivers binding to the same > device (the accelerometer), one natively (lis3lv02d), and one through > an ACPI layer (dell-smo8800). I don't really understand why this is > needed (don't they serve the same purpose?) nor how it can be safe > (what guarantees that both drivers won't attempt to access the hardware > at the same time?) This is not a problem. lis3lv02d provides two things: 1) 3 axes accelerometer 2) optional interrupt and signal it to userspace via misc device dell-smo8800 does not call any parts of lis2lv02d module. It just provides for userspace same misc device API as lis3lv02d. As lis3lv02d has misc device optional, registered i2c device from i2c-i801 does not enable it. So technically it is one device, but their functionality divided into two modules. One which reports accelerometer axes and one which signals disk fall interrupt. These two modules and functionalities does not have to interact, so it is safe to have them separated.
On Tue, 2019-05-28 at 11:41 +0200, Pali Rohár wrote: > This is not a problem. lis3lv02d provides two things: > > 1) 3 axes accelerometer > 2) optional interrupt and signal it to userspace via misc device > > dell-smo8800 does not call any parts of lis2lv02d module. It just > provides for userspace same misc device API as lis3lv02d. > > As lis3lv02d has misc device optional, registered i2c device from > i2c-i801 does not enable it. > > So technically it is one device, but their functionality divided into > two modules. One which reports accelerometer axes and one which signals > disk fall interrupt. These two modules and functionalities does not have > to interact, so it is safe to have them separated. OK, thanks for the explanation. But assuming that we now instantiate the lis2lv02d device from i2c-i801 for exactly all the same machines, can't we just *enable* the freefall misc device feature of lis2lv02d and kill the dell-smo8800 driver completely? Seems more simple to maintain going forward.
On Tuesday 28 May 2019 11:50:15 Jean Delvare wrote: > On Tue, 2019-05-28 at 11:41 +0200, Pali Rohár wrote: > > This is not a problem. lis3lv02d provides two things: > > > > 1) 3 axes accelerometer > > 2) optional interrupt and signal it to userspace via misc device > > > > dell-smo8800 does not call any parts of lis2lv02d module. It just > > provides for userspace same misc device API as lis3lv02d. > > > > As lis3lv02d has misc device optional, registered i2c device from > > i2c-i801 does not enable it. > > > > So technically it is one device, but their functionality divided into > > two modules. One which reports accelerometer axes and one which signals > > disk fall interrupt. These two modules and functionalities does not have > > to interact, so it is safe to have them separated. > > OK, thanks for the explanation. But assuming that we now instantiate > the lis2lv02d device from i2c-i801 for exactly all the same machines, > can't we just *enable* the freefall misc device feature of lis2lv02d > and kill the dell-smo8800 driver completely? Seems more simple to > maintain going forward. I though about it and I already wrote that is it not practical. For ACPI drivers there is easy way to get that interrupt number from ACPI tables. From i2c-i801 PCI driver it is hard to get interrupt number for particular ACPI device... That is way I preferred simple solution: ACPI driver for ACPI device and i2c driver for i2c device.
On Tue, 28 May 2019 11:54:02 +0200, Pali Rohár wrote: > On Tuesday 28 May 2019 11:50:15 Jean Delvare wrote: > > OK, thanks for the explanation. But assuming that we now instantiate > > the lis2lv02d device from i2c-i801 for exactly all the same machines, > > can't we just *enable* the freefall misc device feature of lis2lv02d > > and kill the dell-smo8800 driver completely? Seems more simple to > > maintain going forward. > > I though about it and I already wrote that is it not practical. For ACPI > drivers there is easy way to get that interrupt number from ACPI tables. > From i2c-i801 PCI driver it is hard to get interrupt number for > particular ACPI device... > > That is way I preferred simple solution: ACPI driver for ACPI device and > i2c driver for i2c device. OK, fine with me then :-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 8eac00efadc1..502678e8b8c0 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -1136,6 +1136,107 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap) } } +static acpi_status check_acpi_smo88xx_device(acpi_handle obj_handle, + u32 nesting_level, + void *context, + void **return_value) +{ + struct acpi_device_info *info; + acpi_status status; + char *hid; + + status = acpi_get_object_info(obj_handle, &info); + if (!ACPI_SUCCESS(status) || !(info->valid & ACPI_VALID_HID)) + return AE_OK; + + hid = info->hardware_id.string; + if (!hid) + return AE_OK; + + if (strlen(hid) < 7) + return AE_OK; + + if (memcmp(hid, "SMO88", 5) != 0) + return AE_OK; + + *((bool *)return_value) = true; + return AE_CTRL_TERMINATE; +} + +static bool is_dell_system_with_lis3lv02d(void) +{ + bool found; + acpi_status status; + const char *vendor; + + vendor = dmi_get_system_info(DMI_SYS_VENDOR); + if (strcmp(vendor, "Dell Inc.") != 0) + return false; + + /* + * Check that ACPI device SMO88xx exists and is enabled. That ACPI + * device represent our ST microelectronics lis3lv02d accelerometer but + * unfortunately without any other information (like i2c address). + */ + found = false; + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL, + (void **)&found); + if (!ACPI_SUCCESS(status) || !found) + return false; + + return true; +} + +/* + * Accelerometer's i2c address is not specified in DMI nor ACPI, + * so it is needed to define mapping table based on DMI product names. + */ +static struct { + const char *dmi_product_name; + unsigned short i2c_addr; +} dell_lis3lv02d_devices[] = { + /* + * Dell platform team told us that these Latitude devices have + * ST microelectronics accelerometer at i2c address 0x29. + */ + { "Latitude E5250", 0x29 }, + { "Latitude E5450", 0x29 }, + { "Latitude E5550", 0x29 }, + { "Latitude E6440", 0x29 }, + { "Latitude E6440 ATG", 0x29 }, + { "Latitude E6540", 0x29 }, + /* + * Additional individual entries were added after verification. + */ + { "Vostro V131", 0x1d }, +}; + +static void register_dell_lis3lv02d_i2c_device(struct i801_priv *priv) +{ + struct i2c_board_info info; + const char *dmi_product_name; + int i; + + dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME); + for (i = 0; i < ARRAY_SIZE(dell_lis3lv02d_devices); ++i) { + if (strcmp(dmi_product_name, + dell_lis3lv02d_devices[i].dmi_product_name) == 0) + break; + } + + if (i == ARRAY_SIZE(dell_lis3lv02d_devices)) { + dev_warn(&priv->pci_dev->dev, + "Accelerometer lis3lv02d is present on i2c bus but its" + " i2c address is unknown, skipping registration...\n"); + return; + } + + memset(&info, 0, sizeof(struct i2c_board_info)); + info.addr = dell_lis3lv02d_devices[i].i2c_addr; + strlcpy(info.type, "lis3lv02d", I2C_NAME_SIZE); + i2c_new_device(&priv->adapter, &info); +} + /* Register optional slaves */ static void i801_probe_optional_slaves(struct i801_priv *priv) { @@ -1154,6 +1255,9 @@ static void i801_probe_optional_slaves(struct i801_priv *priv) if (dmi_name_in_vendors("FUJITSU")) dmi_walk(dmi_check_onboard_devices, &priv->adapter); + + if (is_dell_system_with_lis3lv02d()) + register_dell_lis3lv02d_i2c_device(priv); } #else static void __init input_apanel_init(void) {}
Dell platform team told us that some (DMI whitelisted) Dell Latitude machines have ST microelectronics accelerometer at i2c address 0x29. Presence of that ST microelectronics accelerometer is verified by existence of SMO88xx ACPI device which represent that accelerometer. Unfortunately ACPI device does not specify i2c address. This patch registers lis3lv02d device for selected Dell Latitude machines at i2c address 0x29 after detection. And for Dell Vostro V131 machine at i2c address 0x1d which was manually detected. Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so lis3lv02d correctly initialize accelerometer. Tested on Dell Latitude E6440. Signed-off-by: Pali Rohár <pali.rohar@gmail.com> --- Changes since v1: * Added Dell Vostro V131 based on Michał Kępień testing * Changed DMI product structure to include also i2c address I re-tested this patch against Debian's 4.9 kernel on E6440. --- drivers/i2c/busses/i2c-i801.c | 104 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 104 insertions(+)