Re: [PATCH] iio: accel: add missing sensor for some 2-in-1 based ultrabooks

[LuÃs Ferreira]

Hi Hams,

https://github.com/systemd/systemd/pull/12322
https://github.com/hadess/iio-sensor-proxy/pull/262

As all the pull requests to systemd and iio-sensor-proxy has already
been merged, I just need your review on this:
https://github.com/systemd/systemd/pull/12449

Then, I guess this patch is ready to go. Please let me know if theres any issue.

Regards,
Luis


On Wed, 17 Apr 2019 at 16:08, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 15-04-19 17:40, luis@xxxxxxxxxxxxxx wrote:
> > April 6, 2019 10:36 AM, "Hans de Goede" <hdegoede@xxxxxxxxxx> wrote:
> >
> > Hi,
> >
> >> Yes that seems the best way forward with this.
> >>
> >> Note I think "base" is better then "keyboard" for the sensor which
> >> is in the base/keyboard. But neither is perfect, so go which whatever
> >> you prefer.
> >
> > Reference to:
> > - https://github.com/hadess/iio-sensor-proxy/pull/262 > - https://github.com/systemd/systemd/pull/12322
>
> Thank you for your work on this, I see that Bastien has
> already reviewed the iio-sensor-proxy changes.
>
> I've just added one small remark to the systemd changes,
> except for that small remark the systemd changes look good to me.
>
> Regards,
>
> Hams
>
>
>
>
> >> On 06-04-19 01:01, LuÃs Ferreira wrote:
> >>
> >>> Hi,
> >>> Basically we need to come up with a convention to (optionally) indicate
> >>
> >> the sensors location with a udev attribute set by:
> >> /lib/udev/hwdb.d/60-sensor.hwdb
> >>> So should we start adding `ACCEL_LOCATION=display` and
> >>> `ACCEL_LOCATION=keyboard` attributes to that file and patch
> >>> iio-sensor-proxy to ignore the keyboard ones as a first step ?
> >>
> >> Yes that seems the best way forward with this.
> >>
> >> Note I think "base" is better then "keyboard" for the sensor which
> >> is in the base/keyboard. But neither is perfect, so go which whatever
> >> you prefer.
> >>
> >> Thanks & Regards,
> >>
> >> Hans
> >>
> >>> On Wed, 3 Apr 2019 at 10:10, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Hi,
> >>
> >> On 02-04-19 18:04, LuÃs Ferreira wrote:
> >>> Some ultrabooks, like Teclast F6 Pro, use KIOX010A sensor on display
> >>> and KIOX020A sensor on keyboard base, to detect tablet mode or screen
> >>> orientation.
> >>
> >> I deliberately left out the KIOX020A id for now, because currently
> >> userspace cannot really deal with having 2 sensors.
> >>
> >> See:
> >> https://github.com/systemd/systemd/issues/6557
> >> https://github.com/hadess/iio-sensor-proxy/issues/166
> >>
> >> Basically we need to come up with a convention to (optionally) indicate
> >> the sensors location with a udev attribute set by:
> >> /lib/udev/hwdb.d/60-sensor.hwdb
> >>
> >> And then patch iio-sensor-proxy to consume that attribute and ignore
> >> the one which has e.g. ACCEL_LOCATION=keyboard in its udev properties
> >>
> >> Ignoring would be a first step, maybe later it can do something useful
> >> with it, see e.g. : https://github.com/alesguzik/linux_detect_tablet_mode
> >>
> >> IMHO we really should minimally get code in place for iio-sensor-proxy
> >> to ignore the keyboard accelerometer before merging this patch.
> >>
> >> I realize that having the code in place will not magically get it on
> >> all users machines, but I believe this is the minimum which needs to
> >> happen before we push this out and potentially breaks people screen
> >> rotation.
> >>
> >> I've had working on this on my TODO list for a long long time now,
> >> but -ENOTIME. If you have some time to work on this then that would
> >> be great.
> >>
> >> Regards,
> >>
> >> Hans
> >>> Signed-off-by: LuÃs Ferreira <luis@xxxxxxxxxxxxxx>
> >>> ---
> >>> drivers/iio/accel/kxcjk-1013.c | 1 +
> >>> 1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
> >>> index 7096e577b23f..9a5e445facc1 100644
> >>> --- a/drivers/iio/accel/kxcjk-1013.c
> >>> +++ b/drivers/iio/accel/kxcjk-1013.c
> >>> @@ -1492,6 +1492,7 @@ static const struct acpi_device_id kx_acpi_match[] = {
> >>> {"KIOX0009", KXTJ21009},
> >>> {"KIOX000A", KXCJ91008},
> >>> {"KIOX010A", KXCJ91008}, /* KXCJ91008 inside the display of a 2-in-1 */
> >>> + {"KIOX020A", KXCJ91008},
> >>> {"KXTJ1009", KXTJ21009},
> >>> {"KXJ2109", KXTJ21009},
> >>> {"SMO8500", KXCJ91008},