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

Message ID 20180127133209.28995-1-pali.rohar@gmail.com
State Needs Review / ACK
Headers show
Series
  • [v2] i2c: i801: Register optional lis3lv02d i2c device on Dell machines
Related show

Commit Message

Pali Rohár Jan. 27, 2018, 1:32 p.m.
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(+)

Comments

Andy Shevchenko Jan. 28, 2018, 2:39 p.m. | #1
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;
> +}
Pali Rohár Jan. 28, 2018, 2:45 p.m. | #2
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;
> > +}
> 
>
Andy Shevchenko Jan. 28, 2018, 3 p.m. | #3
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)
Pali Rohár Jan. 31, 2018, 12:03 p.m. | #4
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.
Andy Shevchenko Jan. 31, 2018, 12:27 p.m. | #5
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"},
...
{}
};
Michał Kępień Feb. 11, 2018, 10:03 p.m. | #6
> 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.
Andy Shevchenko Feb. 12, 2018, 3:25 p.m. | #7
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.
Pali Rohár Feb. 12, 2018, 3:30 p.m. | #8
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.
Andy Shevchenko Feb. 13, 2018, 2:55 p.m. | #9
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.
Pali Rohár Feb. 13, 2018, 3 p.m. | #10
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.
Andy Shevchenko Feb. 13, 2018, 3:06 p.m. | #11
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.
Pali Rohár Feb. 13, 2018, 4:50 p.m. | #12
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.
> 
>
Andy Shevchenko Feb. 13, 2018, 5:01 p.m. | #13
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).
Andy Shevchenko Feb. 13, 2018, 5:05 p.m. | #14
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.
Wolfram Sang Feb. 26, 2018, 8:32 p.m. | #15
> 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.
Pali Rohár March 8, 2018, 10:53 p.m. | #16
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 April 18, 2018, 11:46 a.m. | #17
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.

Patch

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) {}