How to deal with accelerometers where the ACPI HID indicates the location?

How to deal with accelerometers where the ACPI HID indicates the location?

[Hans de Goede]

Hi All,

So I've been debugging some touchpad issues on a Tresktor Primebook C13,
unfortunately I've been able to get the touchpad to work.

But I did notice the following in the ACPI tables, and there i2cdetect
confirms there are i2c devices at the expected addresses:

                 Device (KXJ0)
                 {
                     Name (_ADR, Zero)  // _ADR: Address
                     Name (_HID, "KIOX010A")  // _HID: Hardware ID
                     Name (_CID, "KIOX010A")  // _CID: Compatible ID
                     Name (_DDN, "Kionix KXCJ9 Accelerometer Display")  // _DDN:

...

                 Device (KXJ1)
                 {
                     Name (_ADR, Zero)  // _ADR: Address
                     Name (_HID, "KIOX020A")  // _HID: Hardware ID
                     Name (_CID, "KIOX020A")  // _CID: Compatible ID
                     Name (_DDN, "Kionix KXCJ9 Accelerometer Keyboard")  // _DDN:

...

Notice these accelerometers use KIOX010A / KIOX020A as HID rather then
the normal KIOX000A and their DDN lists a location (display vs
keyboard, this is a yoga style convertible).

I'm pretty sure from the kernel side these can be fixed by just adding:

         {"KIOX010A", KXCJ91008},
         {"KIOX020A", KXCJ91008},

Entries to the drivers/iio/accel/kxcjk-1013.c driver. I believe the
different HIDs are just there to allow Windows to determine the
location based on the HID.

But before submitting a patch to add these 2 HIDs to the driver I was
wondering do we want to do anything wrt the location on the kernel side.

I do believe there is a need for an ACCEL_LOCATION udev property on
accelerometer iio-devs for use by iio-sensor-proxy, so that it can
pick the one in the display to provide display rotation info.

But since the HID ends up in the device name we can simply add an
udev rule based on this, without the kernel needing to export any
data AFAICT.

Bastien, do you agree that we don't need the kernel to export this
and that we can use a HID match in a udev rule to set an
ACCEL_LOCATION udev property for this ?

Regards,

Hans

Re: How to deal with accelerometers where the ACPI HID indicates the location?

[Jonathan Cameron]

On Wed, 27 Dec 2017 17:42:36 +0100
Hans de Goede <hdegoede@redhat.com> wrote:

> Hi All,
> 
> So I've been debugging some touchpad issues on a Tresktor Primebook C13,
> unfortunately I've been able to get the touchpad to work.
> 
> But I did notice the following in the ACPI tables, and there i2cdetect
> confirms there are i2c devices at the expected addresses:
> 
>                  Device (KXJ0)
>                  {
>                      Name (_ADR, Zero)  // _ADR: Address
>                      Name (_HID, "KIOX010A")  // _HID: Hardware ID
>                      Name (_CID, "KIOX010A")  // _CID: Compatible ID
>                      Name (_DDN, "Kionix KXCJ9 Accelerometer Display")  // _DDN:
> 
> ...
> 
>                  Device (KXJ1)
>                  {
>                      Name (_ADR, Zero)  // _ADR: Address
>                      Name (_HID, "KIOX020A")  // _HID: Hardware ID
>                      Name (_CID, "KIOX020A")  // _CID: Compatible ID
>                      Name (_DDN, "Kionix KXCJ9 Accelerometer Keyboard")  // _DDN:
> 
> ...
> 
> Notice these accelerometers use KIOX010A / KIOX020A as HID rather then
> the normal KIOX000A and their DDN lists a location (display vs
> keyboard, this is a yoga style convertible).
> 
> I'm pretty sure from the kernel side these can be fixed by just adding:
> 
>          {"KIOX010A", KXCJ91008},
>          {"KIOX020A", KXCJ91008},
> 
> Entries to the drivers/iio/accel/kxcjk-1013.c driver. I believe the
> different HIDs are just there to allow Windows to determine the
> location based on the HID.
> 
> But before submitting a patch to add these 2 HIDs to the driver I was
> wondering do we want to do anything wrt the location on the kernel side.

Whilst it's not supported currently by this driver we do have
sysfs attributes to describe this in the ABI but so far it's only
in the device specific docs as it hasn't spread that widely.

Documentation/ABI/testing/sysfs-bus-iio-cros-ec

has location for example - we could move this up to the top level ABI doc
and use it more generally.

> 
> I do believe there is a need for an ACCEL_LOCATION udev property on
> accelerometer iio-devs for use by iio-sensor-proxy, so that it can
> pick the one in the display to provide display rotation info.
> 
> But since the HID ends up in the device name we can simply add an
> udev rule based on this, without the kernel needing to export any
> data AFAICT.
> 
> Bastien, do you agree that we don't need the kernel to export this
> and that we can use a HID match in a udev rule to set an
> ACCEL_LOCATION udev property for this ?

It is somewhat horrible that they are using different ACPI IDs for
location on the device.  I wonder if we can find out where that
came from and whether it is in any way standard?

+CC Chris on off chance still at Kionix and might be able to find
this out for us. (it's been a while ;)

Jonathan

> 
> Regards,
> 
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: How to deal with accelerometers where the ACPI HID indicates the location?

[Hans de Goede]

Hi,

On 29-12-17 13:14, Jonathan Cameron wrote:
> On Wed, 27 Dec 2017 17:42:36 +0100
> Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Hi All,
>>
>> So I've been debugging some touchpad issues on a Tresktor Primebook C13,
>> unfortunately I've been able to get the touchpad to work.
>>
>> But I did notice the following in the ACPI tables, and there i2cdetect
>> confirms there are i2c devices at the expected addresses:
>>
>>                   Device (KXJ0)
>>                   {
>>                       Name (_ADR, Zero)  // _ADR: Address
>>                       Name (_HID, "KIOX010A")  // _HID: Hardware ID
>>                       Name (_CID, "KIOX010A")  // _CID: Compatible ID
>>                       Name (_DDN, "Kionix KXCJ9 Accelerometer Display")  // _DDN:
>>
>> ...
>>
>>                   Device (KXJ1)
>>                   {
>>                       Name (_ADR, Zero)  // _ADR: Address
>>                       Name (_HID, "KIOX020A")  // _HID: Hardware ID
>>                       Name (_CID, "KIOX020A")  // _CID: Compatible ID
>>                       Name (_DDN, "Kionix KXCJ9 Accelerometer Keyboard")  // _DDN:
>>
>> ...
>>
>> Notice these accelerometers use KIOX010A / KIOX020A as HID rather then
>> the normal KIOX000A and their DDN lists a location (display vs
>> keyboard, this is a yoga style convertible).
>>
>> I'm pretty sure from the kernel side these can be fixed by just adding:
>>
>>           {"KIOX010A", KXCJ91008},
>>           {"KIOX020A", KXCJ91008},
>>
>> Entries to the drivers/iio/accel/kxcjk-1013.c driver. I believe the
>> different HIDs are just there to allow Windows to determine the
>> location based on the HID.
>>
>> But before submitting a patch to add these 2 HIDs to the driver I was
>> wondering do we want to do anything wrt the location on the kernel side.
> 
> Whilst it's not supported currently by this driver we do have
> sysfs attributes to describe this in the ABI but so far it's only
> in the device specific docs as it hasn't spread that widely.
> 
> Documentation/ABI/testing/sysfs-bus-iio-cros-ec
> 
> has location for example - we could move this up to the top level ABI doc
> and use it more generally.

Yes that would also work, although in practice I think we're going to
need some udev / hwdb stuff for this anyways since the driver will not
be able to get the location itself in a lot of cases.

I guess we could have a udev role using the location attribute to set
the same ACCEL_LOCATION udev property as we will have the udev rule
set manually on other devices, then the sensor consumer, e.g.
iio-sensor-proxy still only needs to check one place.

Anyways lets wait a bit for Bastien to get back from his holidays
and get his input before moving forward with this.

Regards,

Hans




> 
>>
>> I do believe there is a need for an ACCEL_LOCATION udev property on
>> accelerometer iio-devs for use by iio-sensor-proxy, so that it can
>> pick the one in the display to provide display rotation info.
>>
>> But since the HID ends up in the device name we can simply add an
>> udev rule based on this, without the kernel needing to export any
>> data AFAICT.
>>
>> Bastien, do you agree that we don't need the kernel to export this
>> and that we can use a HID match in a udev rule to set an
>> ACCEL_LOCATION udev property for this ?
> 
> It is somewhat horrible that they are using different ACPI IDs for
> location on the device.  I wonder if we can find out where that
> came from and whether it is in any way standard?
> 
> +CC Chris on off chance still at Kionix and might be able to find
> this out for us. (it's been a while ;)
> 
> Jonathan
> 
>>
>> Regards,
>>
>> Hans
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: How to deal with accelerometers where the ACPI HID indicates the location?

[Bastien Nocera]

----- Original Message -----
> Hi All,
> 
> So I've been debugging some touchpad issues on a Tresktor Primebook C13,
> unfortunately I've been able to get the touchpad to work.
> 
> But I did notice the following in the ACPI tables, and there i2cdetect
> confirms there are i2c devices at the expected addresses:
> 
>                  Device (KXJ0)
>                  {
>                      Name (_ADR, Zero)  // _ADR: Address
>                      Name (_HID, "KIOX010A")  // _HID: Hardware ID
>                      Name (_CID, "KIOX010A")  // _CID: Compatible ID
>                      Name (_DDN, "Kionix KXCJ9 Accelerometer Display")  //
>                      _DDN:
> 
> ...
> 
>                  Device (KXJ1)
>                  {
>                      Name (_ADR, Zero)  // _ADR: Address
>                      Name (_HID, "KIOX020A")  // _HID: Hardware ID
>                      Name (_CID, "KIOX020A")  // _CID: Compatible ID
>                      Name (_DDN, "Kionix KXCJ9 Accelerometer Keyboard")  //
>                      _DDN:
> 
> ...
> 
> Notice these accelerometers use KIOX010A / KIOX020A as HID rather then
> the normal KIOX000A and their DDN lists a location (display vs
> keyboard, this is a yoga style convertible).
> 
> I'm pretty sure from the kernel side these can be fixed by just adding:
> 
>          {"KIOX010A", KXCJ91008},
>          {"KIOX020A", KXCJ91008},
> 
> Entries to the drivers/iio/accel/kxcjk-1013.c driver. I believe the
> different HIDs are just there to allow Windows to determine the
> location based on the HID.
> 
> But before submitting a patch to add these 2 HIDs to the driver I was
> wondering do we want to do anything wrt the location on the kernel side.
> 
> I do believe there is a need for an ACCEL_LOCATION udev property on
> accelerometer iio-devs for use by iio-sensor-proxy, so that it can
> pick the one in the display to provide display rotation info.

I don't think that Windows would be differentiating them based on a textual
value, a free-form string like this one. It might be something we use, but
it's not something they'd do.

I would expect the Windows driver to contain the IDs and say "this ID
is the one in the display, this ID is the one in the keyboard".

You might want to check whether the ACPI DSDT has _PLD information though,
as that would be the programmatic way of exporting this data:
http://marc.info/?l=linux-iio&m=147981183211362&w=2

> But since the HID ends up in the device name we can simply add an
> udev rule based on this, without the kernel needing to export any
> data AFAICT.
> 
> Bastien, do you agree that we don't need the kernel to export this
> and that we can use a HID match in a udev rule to set an
> ACCEL_LOCATION udev property for this ?

See https://github.com/systemd/systemd/issues/6125
and https://github.com/hadess/iio-sensor-proxy/issues/166

iio-sensor-proxy would just ignore the keyboard accelerometer for now.

Cheers

Re: How to deal with accelerometers where the ACPI HID indicates the location?

[Hans de Goede]

Hi,

On 15-01-18 14:28, Bastien Nocera wrote:
> 
> 
> ----- Original Message -----
>> Hi All,
>>
>> So I've been debugging some touchpad issues on a Tresktor Primebook C13,
>> unfortunately I've been able to get the touchpad to work.
>>
>> But I did notice the following in the ACPI tables, and there i2cdetect
>> confirms there are i2c devices at the expected addresses:
>>
>>                   Device (KXJ0)
>>                   {
>>                       Name (_ADR, Zero)  // _ADR: Address
>>                       Name (_HID, "KIOX010A")  // _HID: Hardware ID
>>                       Name (_CID, "KIOX010A")  // _CID: Compatible ID
>>                       Name (_DDN, "Kionix KXCJ9 Accelerometer Display")  //
>>                       _DDN:
>>
>> ...
>>
>>                   Device (KXJ1)
>>                   {
>>                       Name (_ADR, Zero)  // _ADR: Address
>>                       Name (_HID, "KIOX020A")  // _HID: Hardware ID
>>                       Name (_CID, "KIOX020A")  // _CID: Compatible ID
>>                       Name (_DDN, "Kionix KXCJ9 Accelerometer Keyboard")  //
>>                       _DDN:
>>
>> ...
>>
>> Notice these accelerometers use KIOX010A / KIOX020A as HID rather then
>> the normal KIOX000A and their DDN lists a location (display vs
>> keyboard, this is a yoga style convertible).
>>
>> I'm pretty sure from the kernel side these can be fixed by just adding:
>>
>>           {"KIOX010A", KXCJ91008},
>>           {"KIOX020A", KXCJ91008},
>>
>> Entries to the drivers/iio/accel/kxcjk-1013.c driver. I believe the
>> different HIDs are just there to allow Windows to determine the
>> location based on the HID.
>>
>> But before submitting a patch to add these 2 HIDs to the driver I was
>> wondering do we want to do anything wrt the location on the kernel side.
>>
>> I do believe there is a need for an ACCEL_LOCATION udev property on
>> accelerometer iio-devs for use by iio-sensor-proxy, so that it can
>> pick the one in the display to provide display rotation info.
> 
> I don't think that Windows would be differentiating them based on a textual
> value, a free-form string like this one. It might be something we use, but
> it's not something they'd do.

Right.

> I would expect the Windows driver to contain the IDs and say "this ID
> is the one in the display, this ID is the one in the keyboard".

Right, I agree that Windows is likely triggering on the IDs and that we
should do the same (were possible). My purpose of starting this thread
was to agree on an API for exporting this information to iio-sensor-proxy
(and possibly other interested accelerometer event consumers).

The reason I'm suggesting a udev property (with agreed up on fixed
contents for accelerometer in keyboard/base and display) is because on
some hardware the information is not encoded in the ID and we need some
other way to get this info. I'm thinking using a (dmi based) hwdb entry
for this, which matches well with using a udev property and writing udev
rules to set the property based on the ID should be easy too since the
ID is part of the device-name for these devices.

> You might want to check whether the ACPI DSDT has _PLD information though,
> as that would be the programmatic way of exporting this data:
> http://marc.info/?l=linux-iio&m=147981183211362&w=2

So these 2 ids are used in the DSTDs of 3 devices in my DSTD collection:

Medion e2228t (same device as https://github.com/hadess/iio-sensor-proxy/issues/166)
T-Bao Tbook air 12.5
Trekstor Primebook C13

Neither of these 3 DSDTs have an _PLD method on either of the
accelerometer ACPI device nodes.

>> But since the HID ends up in the device name we can simply add an
>> udev rule based on this, without the kernel needing to export any
>> data AFAICT.
>>
>> Bastien, do you agree that we don't need the kernel to export this
>> and that we can use a HID match in a udev rule to set an
>> ACCEL_LOCATION udev property for this ?
> 
> See https://github.com/systemd/systemd/issues/6125
> and https://github.com/hadess/iio-sensor-proxy/issues/166

So looking at these, these too suggest an ACCEL_LOCATION udev property,
given that that makes 2 people coming up with this proposal independently
and even with the ACCEL_LOCATION name, my proposal would be to go with
that.

https://github.com/systemd/systemd/issues/6125 suggests using
"display" and "keyboard" as possible values for now, but I would prefer
to use "display" and "base" since not all devices have a keyboard,
see e.g. :

http://www.gpd.hk/products.asp?selectclassid=017001&id=1299

Which also runs Linux AFAIK, but if you prefer "keyboard" over
"base" that is fine too. As long as we agree on values for this, then
we can document all this in /lib/udev/hwdb.d/60-sensor.hwdb.

Shall I write a systemd patch adding documentation for this and
adding a first hwdb entry?

Writing udev rules for the devices where the location is indicated by
the ACPI HID for the accelerometer is a bit tricky for me to do
because -ENOHARDWARE. But I can take a shot and ask Enaut (in the Cc)
to test, he has a Trekstor Primebook C13.

> iio-sensor-proxy would just ignore the keyboard accelerometer for now.

I guess iio-sensor-proxy should ignore any accelerometer with an
ACCEL_LOCATION property which is not "display", rather then ignore
"keyboard" so that if future locations show up we also ignore those?

Otherwise ack.

Regards,

Hans

Re: How to deal with accelerometers where the ACPI HID indicates the location?

[Bastien Nocera]

----- Original Message -----
<snip>
> Right, I agree that Windows is likely triggering on the IDs and that we
> should do the same (were possible). My purpose of starting this thread
> was to agree on an API for exporting this information to iio-sensor-proxy
> (and possibly other interested accelerometer event consumers).
> 
> The reason I'm suggesting a udev property (with agreed up on fixed
> contents for accelerometer in keyboard/base and display) is because on
> some hardware the information is not encoded in the ID and we need some
> other way to get this info. I'm thinking using a (dmi based) hwdb entry
> for this, which matches well with using a udev property and writing udev
> rules to set the property based on the ID should be easy too since the
> ID is part of the device-name for these devices.
> 
> > You might want to check whether the ACPI DSDT has _PLD information though,
> > as that would be the programmatic way of exporting this data:
> > http://marc.info/?l=linux-iio&m=147981183211362&w=2
> 
> So these 2 ids are used in the DSTDs of 3 devices in my DSTD collection:
> 
> Medion e2228t (same device as
> https://github.com/hadess/iio-sensor-proxy/issues/166)
> T-Bao Tbook air 12.5
> Trekstor Primebook C13
> 
> Neither of these 3 DSDTs have an _PLD method on either of the
> accelerometer ACPI device nodes.

Shame, but not surprising.

> >> But since the HID ends up in the device name we can simply add an
> >> udev rule based on this, without the kernel needing to export any
> >> data AFAICT.
> >>
> >> Bastien, do you agree that we don't need the kernel to export this
> >> and that we can use a HID match in a udev rule to set an
> >> ACCEL_LOCATION udev property for this ?
> > 
> > See https://github.com/systemd/systemd/issues/6125
> > and https://github.com/hadess/iio-sensor-proxy/issues/166
> 
> So looking at these, these too suggest an ACCEL_LOCATION udev property,
> given that that makes 2 people coming up with this proposal independently
> and even with the ACCEL_LOCATION name, my proposal would be to go with
> that.
> 
> https://github.com/systemd/systemd/issues/6125 suggests using
> "display" and "keyboard" as possible values for now, but I would prefer
> to use "display" and "base" since not all devices have a keyboard,
> see e.g. :
> 
> http://www.gpd.hk/products.asp?selectclassid=017001&id=1299
> 
> Which also runs Linux AFAIK, but if you prefer "keyboard" over
> "base" that is fine too. As long as we agree on values for this, then
> we can document all this in /lib/udev/hwdb.d/60-sensor.hwdb.

Those names work for me.

> Shall I write a systemd patch adding documentation for this and
> adding a first hwdb entry?

It'll also need a test case to be added to systemd's test suite.

> Writing udev rules for the devices where the location is indicated by
> the ACPI HID for the accelerometer is a bit tricky for me to do
> because -ENOHARDWARE. But I can take a shot and ask Enaut (in the Cc)
> to test, he has a Trekstor Primebook C13.

As long as there's enough data to uniquely identify the accelerometer on
a specific machine, sounds fine.

> > iio-sensor-proxy would just ignore the keyboard accelerometer for now.
> 
> I guess iio-sensor-proxy should ignore any accelerometer with an
> ACCEL_LOCATION property which is not "display", rather then ignore
> "keyboard" so that if future locations show up we also ignore those?
> 
> Otherwise ack.

Sounds good.

Feel free to CC: me once you've filed a systemd merge request.

Cheers