Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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 :-) -- Jean Delvare SUSE L3 Support
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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. -- Jean Delvare SUSE L3 Support
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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?) -- Jean Delvare SUSE L3 Support
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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? -- Pali Rohár pali.ro...@gmail.com signature.asc Description: PGP signature
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
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. -- Pali Rohár pali.ro...@gmail.com signature.asc Description: PGP signature
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
> 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. signature.asc Description: PGP signature
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Tue, Feb 13, 2018 at 7:01 PM, Andy Shevchenko 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Tue, Feb 13, 2018 at 6:50 PM, Pali Rohár wrote: > On Tuesday 13 February 2018 17:06:19 Andy Shevchenko wrote: >> On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár wrote: >> > On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote: >> >> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár 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). -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Tuesday 13 February 2018 17:06:19 Andy Shevchenko wrote: > On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár wrote: > > On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote: > >> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár wrote: > >> > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: > >> >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár > >> >> wrote: > >> >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: > >> >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár > >> >> >> 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. > > -- Pali Rohár pali.ro...@gmail.com signature.asc Description: PGP signature
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár wrote: > On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote: >> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár wrote: >> > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: >> >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár wrote: >> >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: >> >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár >> >> >> 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote: > On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár wrote: > > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: > >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár wrote: > >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: > >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár > >> >> 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. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár wrote: > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár wrote: >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote: > On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár wrote: > > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: > >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár 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. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Mon, Feb 12, 2018 at 12:03 AM, Michał Kępień wrote: >> Signed-off-by: Pali Rohár >> --- >> 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ń Thanks! I'm waiting for Pali to address my concern. > I did not review the contents of the patch this time, sorry. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
> Dell platform team told us that some (DMI whitelisted) Dell Latitude > machines have ST microelectronics accelerometer at i2c address 0x29. > > Presence of that ST microelectronics accelerometer is verified by existence > of SMO88xx ACPI device which represent that accelerometer. Unfortunately > ACPI device does not specify i2c address. > > This patch registers lis3lv02d device for selected Dell Latitude machines > at i2c address 0x29 after detection. And for Dell Vostro V131 machine at > i2c address 0x1d which was manually detected. > > Finally commit a7ae81952cda ("i2c: i801: Allow ACPI SystemIO OpRegion to > conflict with PCI BAR") allowed to use i2c-i801 driver on Dell machines so > lis3lv02d correctly initialize accelerometer. > > Tested on Dell Latitude E6440. > > Signed-off-by: Pali Rohár > --- > 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ń I did not review the contents of the patch this time, sorry. -- Best regards, Michał Kępień
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár wrote: > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár 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"}, ... {} }; -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote: > On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár wrote: > > On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote: > >> On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár 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. -- Pali Rohár pali.ro...@gmail.com
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár wrote: > On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote: >> On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár 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) -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote: > On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár 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; > > +} > > -- Pali Rohár pali.ro...@gmail.com signature.asc Description: PGP signature
Re: [PATCH v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár 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; > +} -- With Best Regards, Andy Shevchenko