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 <[email protected]>
---
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(+)
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) {}
--
2.11.0
On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár <[email protected]> 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
On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote:
> On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár <[email protected]> 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
[email protected]
On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <[email protected]> wrote:
> On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote:
>> On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár <[email protected]> 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
On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <[email protected]> wrote:
> > On Sunday 28 January 2018 16:39:25 Andy Shevchenko wrote:
> >> On Sat, Jan 27, 2018 at 3:32 PM, Pali Rohár <[email protected]> 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
[email protected]
On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <[email protected]> wrote:
> On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
>> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <[email protected]> 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
> 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 <[email protected]>
> ---
> 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ń <[email protected]>
I did not review the contents of the patch this time, sorry.
--
Best regards,
Michał Kępień
On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <[email protected]> wrote:
> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <[email protected]> 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
[email protected]
On Mon, Feb 12, 2018 at 12:03 AM, Michał Kępień <[email protected]> wrote:
>> Signed-off-by: Pali Rohár <[email protected]>
>> ---
>> 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ń <[email protected]>
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
On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <[email protected]> wrote:
> On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
>> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <[email protected]> wrote:
>> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
>> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <[email protected]> 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
On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote:
> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <[email protected]> wrote:
> > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
> >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <[email protected]> wrote:
> >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
> >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <[email protected]> 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
[email protected]
On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár <[email protected]> wrote:
> On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote:
>> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <[email protected]> wrote:
>> > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
>> >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <[email protected]> wrote:
>> >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
>> >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <[email protected]> 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
On Tuesday 13 February 2018 17:06:19 Andy Shevchenko wrote:
> On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár <[email protected]> wrote:
> > On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote:
> >> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <[email protected]> wrote:
> >> > On Wednesday 31 January 2018 14:27:51 Andy Shevchenko wrote:
> >> >> On Wed, Jan 31, 2018 at 2:03 PM, Pali Rohár <[email protected]> wrote:
> >> >> > On Sunday 28 January 2018 17:00:35 Andy Shevchenko wrote:
> >> >> >> On Sun, Jan 28, 2018 at 4:45 PM, Pali Rohár <[email protected]> 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
[email protected]
On Tue, Feb 13, 2018 at 6:50 PM, Pali Rohár <[email protected]> wrote:
> On Tuesday 13 February 2018 17:06:19 Andy Shevchenko wrote:
>> On Tue, Feb 13, 2018 at 5:00 PM, Pali Rohár <[email protected]> wrote:
>> > On Tuesday 13 February 2018 16:55:00 Andy Shevchenko wrote:
>> >> On Mon, Feb 12, 2018 at 5:30 PM, Pali Rohár <[email protected]> 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
On Tue, Feb 13, 2018 at 7:01 PM, Andy Shevchenko
<[email protected]> 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
> 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.
--
Pali Rohár
[email protected]
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
[email protected]
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
[email protected]
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
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
[email protected]
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
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
[email protected]
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
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 <[email protected]>
---
Changes since v2:
* Use explicit list of SMOxx ACPI devices
Changes since v1:
* Added Dell Vostro V131 based on Michał Kępień testing
* Changed DMI product structure to include also i2c address
---
drivers/i2c/busses/i2c-i801.c | 118 ++++++++++++++++++++++++++++++++++++
drivers/platform/x86/dell-smo8800.c | 1 +
2 files changed, 119 insertions(+)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index ac7f7817dc89..2ac8ff41cc24 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -1134,6 +1134,121 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
}
}
+/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */
+static const struct acpi_device_id acpi_smo8800_ids[] = {
+ { "SMO8800", 0 },
+ { "SMO8801", 0 },
+ { "SMO8810", 0 },
+ { "SMO8811", 0 },
+ { "SMO8820", 0 },
+ { "SMO8821", 0 },
+ { "SMO8830", 0 },
+ { "SMO8831", 0 },
+};
+
+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;
+ int i;
+
+ 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;
+
+ for (i = 0; i < ARRAY_SIZE(acpi_smo8800_ids); ++i) {
+ if (strcmp(hid, acpi_smo8800_ids[i].id) == 0) {
+ *((bool *)return_value) = true;
+ return AE_CTRL_TERMINATE;
+ }
+ }
+
+ return AE_OK;
+
+}
+
+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)
{
@@ -1152,6 +1267,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) {}
diff --git a/drivers/platform/x86/dell-smo8800.c b/drivers/platform/x86/dell-smo8800.c
index 5cdb09cba077..bfcc1d1b9b96 100644
--- a/drivers/platform/x86/dell-smo8800.c
+++ b/drivers/platform/x86/dell-smo8800.c
@@ -198,6 +198,7 @@ static int smo8800_remove(struct acpi_device *device)
return 0;
}
+/* NOTE: Keep this list in sync with drivers/i2c/busses/i2c-i801.c */
static const struct acpi_device_id smo8800_ids[] = {
{ "SMO8800", 0 },
{ "SMO8801", 0 },
--
2.11.0
Hi Pali,
Next time, please start a new thread for a new version of a patch.
On Sun, 2 Jun 2019 15:20:03 +0200, 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.
>
> Signed-off-by: Pali Rohár <[email protected]>
>
> ---
> Changes since v2:
> * Use explicit list of SMOxx ACPI devices
>
> Changes since v1:
> * Added Dell Vostro V131 based on Michał Kępień testing
> * Changed DMI product structure to include also i2c address
> ---
> drivers/i2c/busses/i2c-i801.c | 118 ++++++++++++++++++++++++++++++++++++
> drivers/platform/x86/dell-smo8800.c | 1 +
> 2 files changed, 119 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index ac7f7817dc89..2ac8ff41cc24 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -1134,6 +1134,121 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
> }
> }
>
> +/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */
> +static const struct acpi_device_id acpi_smo8800_ids[] = {
> + { "SMO8800", 0 },
> + { "SMO8801", 0 },
> + { "SMO8810", 0 },
> + { "SMO8811", 0 },
> + { "SMO8820", 0 },
> + { "SMO8821", 0 },
> + { "SMO8830", 0 },
> + { "SMO8831", 0 },
> +};
> +
> +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;
> + int i;
> +
> + 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;
> +
> + for (i = 0; i < ARRAY_SIZE(acpi_smo8800_ids); ++i) {
> + if (strcmp(hid, acpi_smo8800_ids[i].id) == 0) {
> + *((bool *)return_value) = true;
> + return AE_CTRL_TERMINATE;
> + }
> + }
> +
> + return AE_OK;
> +
Unneeded blank line.
> +}
> +
> +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
I see how you check that the device exists, but not that it is enabled.
> + * device represent our ST microelectronics lis3lv02d accelerometer but
> + * unfortunately without any other information (like i2c address).
I2C
> + */
> + found = false;
> + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> + (void **)&found);
> + if (!ACPI_SUCCESS(status) || !found)
> + return false;
> +
> + return true;
Looks more complex than it needs to be. You don't really care about the
status, as in the end you return the same on error as you do when no
device is found. So I think you can simply go with:
found = false;
acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
(void **)&found);
return found;
> +}
> +
> +/*
> + * Accelerometer's i2c address is not specified in DMI nor ACPI,
I2C
> + * so it is needed to define mapping table based on DMI product names.
> + */
> +static struct {
Any reason to not make it const?
> + 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.
I2C
> + */
> + { "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 bus -> SMBus
> + " i2c address is unknown, skipping registration...\n");
I2C (or just s/i2c //, as it's kind of redundant)
Suspension points not really needed IMHO.
> + return;
> + }
> + (...)
All the rest looks good to me.
Thanks,
--
Jean Delvare
SUSE L3 Support
Hi!
On Tuesday 04 June 2019 16:57:29 Jean Delvare wrote:
> Hi Pali,
>
> Next time, please start a new thread for a new version of a patch.
Ok!
> On Sun, 2 Jun 2019 15:20:03 +0200, 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.
> >
> > Signed-off-by: Pali Rohár <[email protected]>
> >
> > ---
> > Changes since v2:
> > * Use explicit list of SMOxx ACPI devices
> >
> > Changes since v1:
> > * Added Dell Vostro V131 based on Michał Kępień testing
> > * Changed DMI product structure to include also i2c address
> > ---
> > drivers/i2c/busses/i2c-i801.c | 118 ++++++++++++++++++++++++++++++++++++
> > drivers/platform/x86/dell-smo8800.c | 1 +
> > 2 files changed, 119 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> > index ac7f7817dc89..2ac8ff41cc24 100644
> > --- a/drivers/i2c/busses/i2c-i801.c
> > +++ b/drivers/i2c/busses/i2c-i801.c
> > @@ -1134,6 +1134,121 @@ static void dmi_check_onboard_devices(const struct dmi_header *dm, void *adap)
> > }
> > }
> >
> > +/* NOTE: Keep this list in sync with drivers/platform/x86/dell-smo8800.c */
> > +static const struct acpi_device_id acpi_smo8800_ids[] = {
> > + { "SMO8800", 0 },
> > + { "SMO8801", 0 },
> > + { "SMO8810", 0 },
> > + { "SMO8811", 0 },
> > + { "SMO8820", 0 },
> > + { "SMO8821", 0 },
> > + { "SMO8830", 0 },
> > + { "SMO8831", 0 },
> > +};
> > +
> > +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;
> > + int i;
> > +
> > + 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;
> > +
> > + for (i = 0; i < ARRAY_SIZE(acpi_smo8800_ids); ++i) {
> > + if (strcmp(hid, acpi_smo8800_ids[i].id) == 0) {
> > + *((bool *)return_value) = true;
> > + return AE_CTRL_TERMINATE;
> > + }
> > + }
> > +
> > + return AE_OK;
> > +
>
> Unneeded blank line.
Ok.
> > +}
> > +
> > +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
>
> I see how you check that the device exists, but not that it is enabled.
Hm.. you are right. I will add missing check.
> > + * device represent our ST microelectronics lis3lv02d accelerometer but
> > + * unfortunately without any other information (like i2c address).
>
> I2C
I will change i2c to I2C in whole patch.
> > + */
> > + found = false;
> > + status = acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> > + (void **)&found);
> > + if (!ACPI_SUCCESS(status) || !found)
> > + return false;
> > +
> > + return true;
>
> Looks more complex than it needs to be. You don't really care about the
> status, as in the end you return the same on error as you do when no
> device is found. So I think you can simply go with:
>
> found = false;
> acpi_get_devices(NULL, check_acpi_smo88xx_device, NULL,
> (void **)&found);
>
> return found;
Ok, it really simplify that check.
>
> > +}
> > +
> > +/*
> > + * Accelerometer's i2c address is not specified in DMI nor ACPI,
>
> I2C
>
> > + * so it is needed to define mapping table based on DMI product names.
> > + */
> > +static struct {
>
> Any reason to not make it const?
No, I will make it const.
> > + 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.
>
> I2C
>
> > + */
> > + { "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 bus -> SMBus
>
> > + " i2c address is unknown, skipping registration...\n");
>
> I2C (or just s/i2c //, as it's kind of redundant)
>
> Suspension points not really needed IMHO.
Ok, it will be in V4 which I sent in few minutes.
> > + return;
> > + }
> > + (...)
>
> All the rest looks good to me.
>
> Thanks,
--
Pali Rohár
[email protected]