lkml.org 
[lkml]   [2019]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC 0/4] iio-input-bridge so that accelerometers which only have an iio driver can still present evdev input events
From
Date
Hi all,
I'll send a single v2 patch in some minutes.

Changes compared to this v1:
* add linux-input list and maintainers for discussion
* use input-polldev
* allocate one mapping record for each iio device instead of trying to manage that in some array

> Am 28.03.2019 um 18:42 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>
> Hi Jonathan,
>
>> Am 24.03.2019 um 19:29 schrieb Jonathan Cameron <jic23@kernel.org>:
>>
>> On Mon, 18 Mar 2019 21:39:30 +0100
>> "H. Nikolaus Schaller" <hns@goldelico.com> wrote:
>>
>>
>>> Some user spaces (e.g. some Android) use /dev/input/event* for handling the 3D
>>> position of the device with respect to the center of gravity (earth). This can
>>> be used for gaming input, rotation of screens etc.
>>>
>>> This should be the standard because this interface is an abstraction of how
>>> this data is acquired from sensor chips. Sensor chips may be connected through
>>> different interfaces and in different positions. They may also have different
>>> parameters. And, if a chip is replaced by a different one, the values reported
>>> by the device position interface should remain the same.
>>>
>>> But nowadays, new accelerometer chips usually just get iio drivers and rarely
>>> an evdev input driver.
>>>
>>> Therefore we need something like a protocol stack: input device vs. raw data.
>>> It can be seen as a similar layering like TCP/IP vs. bare Ethernet. Or keyboard
>>> input events vs. raw gpio or raw USB access.
>>>
>>> This patch set bridges the gap between raw iio data and the input device abstraction
>>> so that accelerometer measurements can also be presented as X/Y/Z accelerometer
>>> channels (INPUT_PROP_ACCELEROMETER) through /dev/input/event*.
>>>
>> Hi,
>>
>> I've kind of run out of time today and want to give this a detailed look.
>
> No problem, please take the time it needs.
>
>>
>> In the meantime a few initial comments.
>>
>> 1. Resend the whole series cc'ing the linux-input list and maintainer.
>
> Ok!

Done.

>
>> 2. In the vast majority of devices the interrupt pin is connected for an
>> accelerometer. and they support data ready signals. My gut feeling is
>> that should be the preferred mode.
>
> Mine too, and the commit message 1/4 describes this by
>
> "If it runs, the driver polls the device(s) once every 100 ms. A mode where the
> iio device defines the update rate is not implemented and for further study."
>
> But I didn't manage to get this correctly set up for in-kernel iio on demand
> (start/stop when a process does an open/close - it was simpler to start/stop
> polling).
>
> Here I am lacking some understanding of details of the subsystems and their
> capabilities.
>
> Therefore I focussed on the polling case to leave the interrupt driven case
> for later improvements. Also, since 100ms doesn't seem to be a big resource
> burden.
>
> I am also not sure if we really want to process every fidget detected by the
> raw sensor to the input subsystem. This could require more resources than
> polling with 100ms distance.

Because this is a quite fundamental design decision (do we call input_register_device
or input_register_polled_device?) and the above expressed concern about noise
triggering more input events than user-space may want to see, I have stayed
with low-speed polling for the time being.

>
>> It was for that use case that we originally
>> put all the demux and multiple buffer code in IIO. The original series for
>> that actually had an input bridge.
>> https://lore.kernel.org/linux-iio/1351679431-7963-5-git-send-email-jic23@kernel.org/
>
> Oh, nice! I wasn't aware of that. If I had known, we might have based our
> code on it.
>
> It seems to be a good blueprint to understand how to set up the callbacks.
>
> What I do not understand is where the "iio map" comes from in your approach.
>
> From device tree? That would IMHO be against the rule that DTS should describe
> hardware but bridging data to a different user-space interface is not related
> to hardware description.
>
> So our approach does not need a specific mapping.
>
>>
>> 3. It's going to be a hard sell to have it 'always' map an accelerometer in IIO
>> to an input device when configured. There are lots of other accelerometer use
>> cases where this would be crazy. In the repost, give some comentary perhaps on
>> the following options:
>> a) Explicit binding - like the iio-hwmon bridge.
>> b) A userspace bridge (I even wrote one of those years ago using uevent)
>> c) Some sort of userspace triggered way of creating these on demand.
>
> Basically, if you have one of these many other use cases, one can just keep this
> bridge unconfigured. Then, it does not eat up memory or code space or processor
> cycles.
>
> Its only use case is to map iio accelerometers to input devices which are
> really used as input devices, i.e. gaming, device orientation etc. Other use
> cases (e.g. industrial sensor grid data aggregator) should not consider this as
> an alternative interface and use e.g. libiio of course.
>
> Those (handheld) devices in mind usually have only a single accelerometer chip, but
> there are exceptions like the GTA04 and the Pyra which can have two of them (with
> different precision, mounting orientation and functions). Maybe there are some
> devices with more than two. So these iio devices should be reported separately
> but the data (X,Y,Z) should mainly agree on input device level.
>
> In the two-sensor case, our proposed driver simply creates two distinct /dev/input/event
> files which can be given fixed file names by udev instead of inventing a new trigger
> to create these on demand. IMHO it should not be handled differently from plugging in
> multiple mice or joysticks or other gaming devices to USB.
>
> And, this driver converts "raw" accelerometer data into a higher abstraction level
> input event in a way that user-space becomes independent of the specific chip set
> and its orientation. This is like hiding different flash hardware interfaces
> by MTD or file systems. All the hardware details are hidden and shielded. Or it
> is like mapping gpios or alternatively an i2c scanner chio into standardized key codes.
>
> Finally, there is no polling (in this polling setup) if no process opens the input device.
> So the only waste of 'always' mapping 'all' accelerometers but not using them are some
> data structure allocations in the kernel and duration of probe().
>
>> 4. Input has polled modes. Don't reinvent them.
>
> Haven't heard of, but that is what a RFC is good for.
>
> Anyways, if we use iio interrupts we don't need it, because the information flow
> should go from the chip to the iio driver to the input bridge to the input subsystem
> and only then notify user space (wake up the process running a select on the event
> device file).
>
> Now, there is something I have learned from studying your comment and struct input_dev.
>
> Although I did not find polled modes, I can simplify our approach significantly
> since we do not have to count open/close ourselves and define a mutex for that.
>
> This is already done by the input_dev. So I should improve this part of our patches
> before resending to input list. Thanks for inpsiring this.

I finally found it. It is an input-polldev wrapper.

I have reworked the driver in polling mode to use that now. It has simplified the
code a lot.

Thanks again!

Nikolaus

>
>> 5. The patch break up is very very random. Just have one patch :)
>
> Well, it tries to have small pieces in a specific order that you can bisect and
> do not run into compile problems (e.g. adding Makefile before code).
>
> But I can squash it. Seems to make sense since it is not really useful to know
> which of these pieces breaks other things. It can only be the driver
> and bisect will still pin-point it.
>
>>
>> Anyhow, I'll take a look in detail but may be a little while.
>
> No reason to hurry. We can wait.

>
>>
>> Thanks,
>>
>> Jonathan
>
> Thanks and BR,
> Nikolaus
>
>>
>>
>>>
>>> H. Nikolaus Schaller (4):
>>> iio: input-bridge: optionally bridge iio acceleometers to create a
>>> /dev/input
>>> iio: input-bridge: add iio-input-bridge to Makefile
>>> iio: input-bridge: add IIO_INPUT_BRIDGE kernel config option
>>> iio: input-bridge: make the iio-input-bridge driver called by iio-core
>>>
>>> drivers/iio/Kconfig | 7 +
>>> drivers/iio/Makefile | 1 +
>>> drivers/iio/industrialio-core.c | 12 +
>>> drivers/iio/industrialio-inputbridge.c | 420 +++++++++++++++++++++++++
>>> drivers/iio/industrialio-inputbridge.h | 28 ++
>>> 5 files changed, 468 insertions(+)
>>> create mode 100644 drivers/iio/industrialio-inputbridge.c
>>> create mode 100644 drivers/iio/industrialio-inputbridge.h
>>>
>>
>

\
 
 \ /
  Last update: 2019-03-31 12:11    [W:0.209 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site