[Letux-kernel] [RFC v2] iio: input-bridge: optionally bridge iio acceleometers to create a /dev/input interface

H. Nikolaus Schaller hns at goldelico.com
Mon Apr 15 23:04:15 CEST 2019


Hi Jonathan,
we seem to have opposite goals and therefore quite different ideas what the right
solution is.

I come from user-space and the input side, find that input can handle abstract "acceleration input",
and some older chip drivers even provide that as their only interface. Therefore I want
to provide "acceleration input" in these cases where only iio capable drivers exist by
using the existing in-kernel-iio infrastructure. Nothing more.

You seem to come from the iio architecture and want to extend it to other APIs as
general as possible and input is just one of several of them.

Different goals usually lead to different solution architectures.

> Am 14.04.2019 um 13:40 schrieb Jonathan Cameron <jic23 at kernel.org>:
> 
> On Mon, 8 Apr 2019 15:15:56 +0200
> H. Nikolaus Schaller <hns at goldelico.com> wrote:
> 
>> Hi Jonathan,
>> 
>> I still do not fully understand what is worrying you here.
> 
>> 
>> Do you worry about functionality, flexibility or resources or something else?
> 
> Two main things:
> 1) Lack of generality of the approach. 
>   This is a single use trick for input devices. Why does it make sense for
>   input devices?

No, it is not a trick...

Why does it make sense for input devices?

a) because there is no alternative for accelerometer input devices and there are some
(older) accelerometer chips which only present themselves as input devices as soon
as the driver is compiled and the chip is found.

b) because input events define ACCEL input devices. But no Temperature or Voltage
input or generic ADC input.So there is no generalization of input devices beyond
keyboard, mouse, joystick, accelerometer and some others.

>  There are lots of other in kernel users and potential
>   ones in the future.

This bridge addition does not exclude any (additional) in-kernel use.

>  The ability to register additional IIO consumers like
>   this is useful, lets make it useful to everyone.

But not everyone and every iio device can and should be mappable to the "input" abstraction.
This does not make any sense to me.

For example, does it make sense to map a temperature sensor to accelerometer input? Or an
accelerometer to hwmon? This seems to be possible of your generalization unless I am missing something here.
If it is, it ignores that iio sensors are already grouped by what physical property they do measure.

> 
> 2) To much generality of the specific usecase.  I don't want to put an Input
>   interface on accelerometers where it makes no sense.

I think you can just ignore the input interfaces in that case, if it was created.

>  The rule of it has
>   2-3 axis so it must make sense isn't good enough to my mind.  How
>   does userspace know which accelerometer to use (more and more devices have
>   multiple?)

In our approach user-space can make it known by udev rules based on /dev/input/event*
(not on iio but the input created for accelerometers). I think I mentioned that. This
comes for free for any device registering as input. So it is no additional code.

>  You could do something like looking at the location info from
>   DT / ACPI in your driver and pick the 'best' but that's policy. Should be
>   in userspace.  Sure you can just use the right input driver, but the moment
>   we do that, we need aware userspace, if that's the case why not make it
>   aware from the start.
> 
> Believe me I've been round this one a good few times and thought about it
> a lot.

That is fine and why we should discuss all the different aspects we have collected.

>  I'll take a lot of convincing that this isn't a problem that
> should be pushed into userspace.
> 
>> 
>> I think having them mapped always does not need much resources (except a handful of bytes
>> in memory and some µs during probing) unless the event device is opened and really used.
>> Only then it starts e.g. I2C traffic to read the sensors.
> 
> The bytes don't really mater.

Ok, good to know.

> The userspace ABI additions do.

There are only new /dev/input/event devices with well defined ABI. This approach does
not invent anything new here, hence there are no ABI additions which we can break.

> 
>> 
>> So it is just some unused file sitting around in /dev. Or 2 or could be even 100.
>> For devices which have no iio accelerometers configured, there will be no /dev/input
>> file. So we are discussing the rare case of devices with more than one or two accelerometers.
> 
> Well they aren't exactly rare in IIO using systems ;)

This is another thing where our experiences differ. What specific devices are you thinking
of? I am focussed on handhelds where the accelerometer (or two) is a way to do GUI input
depending on device orientation in space.

> 
>> 
>> Now, on every system there are many interfaces and files that are not used because it makes
>> no sense to look at them. If I check on one of my systems, I find for example a lot of
>> /dev/tty and only a very small portion is used and generic distros have no issue with it.
>> 
>> There is even /dev/iio:device0 to /dev/iio:device5 representing the raw iio devices.
>> Not all of them are actively used, but they are simply there and can be scanned for.
> 
> Agreed, in the ideal case we wouldn't have had that either, but we are
> stuck with it.  The long term plan is to allow use of IIO backends without the
> front end being there at all. Lots of SoC ADC users would prefer this. We are
> stuck with the legacy intertwining fo the front end and back end of IIO so
> this isn't as easy to do as I would like.

Ah, ok. I think it is a similar discussion of hiding the serdev /dev/tty* if it is
used for accessing an embedded GPS or Bluetooth chip, for example.

But is this needed? I think it is not a problem if there are multiple consumers for
the same iio channel. Some in-kernel, some through /dev/iio:device* and maybe some
through /dev/input (which boils down to in-kernel).

> 
>> 
>> So I do not see a resource problem if every accelerometer /dev/iio:device* gets
>> some companion /dev/input/event* for being used on demand - but only if this bridge
>> is configured at all.
> 
> That argument does not apply. If we add a config option, distros will enable it.
> So the vast majority of systems will ship with this turned on.  You cannot
> use a config variable to control policy and expect it to be change by anyone
> but a very very small subset of users.  So please drop the 'you can just not
> build it argument'.

This is not my point here. I mention this under the (now known to be wrong) assumption
that resources do care. I just want to state that kernels built for platforms where every
byte counts can be stripped down by disabling it. Others where resources are no concern
simply can map them all, even if not used.

> Userspace configuration changing is a lot easier if people actually care.
> Sure, many distros will ship the same script to everyone.
> 
>> 
>>> I think we need some deliberate userspace interaction to instantiate
>>> one of these rather than 'always doing it'.  
>> 
>> My gut feeling is that this additional user-space interaction needs more resources and
>> adds a lot of complexity, independently of how it is done.
> 
> Trivial resources and actually fairly trivial complexity.  Key thing is
> it puts the burden on the users of this functionality to configure what they
> want.

Hm. No. My proposal does not need configuration which accelerometers should go where.

I assumethat input accelerometer users do not want to configure anything, like neither
a mouse or keyboard is to be configured to be useable (yes there are keymaps but that
is impossible to automate).

They just want to be able to read device orientation in a device-independent scale.
Therefore my approach already takes the mount-matrix into account to hide sensor position
differences.

> 
>> 
>> And I think is even less flexible than "always doing it". Let me explain this claim.
>> 
>> For me, the kernel should present everything the hardware supports to user-space
>> in better digestable device files or APIs (without making any assumptions about the
>> user-space code).
> 
> Agreed, we just have a different view on how this should be done. I want
> it to be dynamic and extremely flexible, you want the easy way of just
> putting a fixed set out all the time.
> 
>> 
>> Then, every user-space that will be installed can find out what the hardware supports
>> by looking at standard places.
>> 
>> E.g. it can scan for all mice and keyboards. And for all input accelerometers.
> 
> Or, you an have the correct 'fairly trivial' userspace setup to scan for all
> registered accelerometers and 'on demand' create the bindings to bring them up as
> Input accelerometers if that is what makes sense for your platform.

Why not scan for input accelerometers and leave it as an implementation detail that
the kernel does serve the physical chips through the iio infrastructure?

IMHO some user-spaces may already be scanning all */input/event* and check for
the device property INPUT_PROP_ACCELEROMETER.

This is a discussion mainly about proper encapsulation of lower level differences.

> 
>> 
>> If the kernel is hiding some chips and needs some initial user-space action before
>> presenting them all, this requires that the user-space has some a-priori knowledge
>> about which specific devices it should ask for.
> 
> No more that it needs to know which accelerometer to use?

> 
>> So it does not really need to scan
>> for them. Because it must already know. Obviously in some mapping table stored at
>> a well known location inside the rootfs image.
> 
> No. Let me give some more details of how this would work.  It's really just
> a more flexible version of what you have.
> 
> A distro, or individual user decides to put the relevant script in place for the
> following:
> 
> 1. Userspace detects a new accelerometer driver, via the standard methods (uevent)
> 2. Userspace looks to see if it has the required properties. Now this includes things
> like detecting that it is the accelerometer in the lid of a laptop - if so do not
> register it as an input device.  If it's in the keyboard then do register it.
> 3. Userspace script then creates the files in configfs
> /sys/kernel/config/iio/maps/
> (this interface needs appropriate definition)
> Maybe...
> /sys/kernel/config/iio/maps/iio_input/iio_device:X/accel_x, accel_y, etc
> When done it writes to the bind file
> /sys/kernel/config/iio/maps/iio_input/iio_device:X/bind
> which instantiates the input driver.
> 
> This moves all of the policy decision into userspace, where it belongs.  If
> we want to enable a particular accelerometer on a particular board because it
> actually works better than the one the default policy says to use, then we can
> do so.
> 
> The resulting infrastructure is much more general, because it lets us do the
> same for any IIO consumer.  This input bridge is not a special case. It works
> equally well for the existing hwmon bridge any would even let us do things
> like provide the information from userspace that we have an analog accelerometer
> wired up to an ADC on some hacker board.

Ok, understood.

My approach triggers input uevents:

1. kernel detects a new iio accelerometer (looks like an analog accelerometer should be
   the DTS child of an iio adc and then iio should create an accelerometer and not a voltage
   channel)
2. iio-bridge registers as input event
3. this triggers an uevent
4  an udev-rule can detect the properties and map it to some "speaking" name like
   /dev/input/main-accelerometer, /dev/input/lid-accelerometer etc. Or if the
   accelerometer is to be ignored, it does not get a "speaking" name at all.

The required udev rules are stored in user space and are of course user-space and application
specific. But this does not require to invent some new configfs stuff and special scripts
in user-space. Just install some udev rule at a well established location in file-system.

Yes, this does not cover arbitrary mappings. But what are arbitrary mappings good
for? Your scheme seems to be able to map a light sensor to accelerometer input.
Does this "full matrix of everything is possible" really make sense?

I can't decide because I have no need for it. Others may have.

But another thought: does it interfere with this input-bridge? Probably no. You can
still add your configfs approach for general iio devices to e.g. hwmon mappings. Even
as an alternate method of creating input devices (enabled only if my input-bridge is
disabled).

> 
> 
>> 
>> This seems to make it impossible to develop a generic distro rootfs image - without
>> asking the user for manual configuration. And that where the kernel already knows
>> this (which iio accelerometers do exist for a specific piece of hardware).
>> 
>> This is why I believe a mechanism to instantiate only on demand isn't adding but
>> removing flexibility because it prevents copying a rootfs from one device to another.
> 
> I disagree, see above.
> 
>> 
>>> 
>>> As I mentioned in V1, look at the possibility of a configfs based method
>>> to build the map.  It's easy for userspace to work out what makes sense to
>>> map in principle.  There may be some missing info that we also need to
>>> look to expose.  
>> 
>> With a "may be missing" it is impossible to write code for it...
>> Can you please name which information is missing on the input accelerometer
>> API?
> 
> See above. It's not the input accelerometer ABI, it's the missing ability
> to instantiate IIO maps from user space.
> 
>> 
>>> 
>>> In general, userspace created channel maps would be very useful for
>>> other things such as maker type boards where they can plug all sorts
>>> of odd things into ADC channels for example.  
>> 
>> Ok, I understand, but this is a different problem where this iio-input-bridge is not
>> intended to be a solution. Generic ADCs are not input devices. Like SD cards are not
>> keyboards.
>> 
>> So we should not try to mix the idea of general mapping with this input-bridge for
>> input accelerometers.
> Yes we should. You are proposing a solution that is a subset of the larger
> problem set.

Yes, of course. Because I did not see or know about the general problem set.
And I still don't see a need for user-space controlled mapping for input-accelerometers.

>  Why introduce a stop gap like this when we can do it correctly
> and provide something useful for all those other use cases.
> 
> The only difference here is the uevent triggered script that creates those maps
> for your particular usecase.

Well, I am a friend of solving one problem after the other in smaller steps than
immediately aiming at a very general solution, which has side-effects of inventing
new stuff for things that would work without.

> 
> 
>> 
>> BTW, there is a way to define additional mapping using udev rules which symlink the
>> /dev/input/event* paths to stable names like /dev/input/accelerometer.
>> 
>> This comes without additional code and is already provided by udev and the input system.
>> 
>> So in summary, I have not yet seen a convincing scenario where being able to dynamically
>> map iio channels to input devices seems beneficial.
> 
> That is true for the narrow case you are talking about. I don't want to see that
> narrow case solved in a fashion that effectively breaks solving it properly.

How does it break your approach if added later? The more I think about it they are
not incompatible. It is just useless to apply both in parallel.

> If we add this, we have to export all accelerometers for ever under all circumstances
> to userspace, because to remove it will break existing userspace.
> 
> If we stand back and work out if we can do the general solution now, we avoid
> this problem.

We get a different problem that we break existing user-space that simply wants to see
an /dev/input/accelerometer without doing more than an existing udev rule.

> 
>> 
>>> 
>>>> 
>>>> This driver simply collects the first 3 accelerometer channels as X, Y and Z.
>>>> If only 1 or 2 channels are available, they are used for X and Y only. Additional
>>>> channels are ignored.
>>>> 
>>>> Scaling is done automatically so that 1g is represented by value 256 and
>>>> range is assumed to be -511 .. +511 which gives a reasonable precision as an
>>>> input device.  
>>> 
>>> Why do we do this, rather than letting input deal with it?  Input is used
>>> to widely differing scales IIRC  
>> 
>> Well, it can't be done differently... And what I call scale here is nothing more than
>> defining ABSMIN_ACC_VAL and ABSMAX_ACC_VAL.
>> 
>> We need to apply some scale since iio reports in (fractional) units of 1g, i.e. values
>> of magnitude 1.
> 
> m/s^2 not g, but doesn't matter for the point of view of this discussion.

My fault. The driver takes care of this in the scaling formula so that "input" reports
MAX/2 for 1g.

> 
>> These are not adaequate for input events which use integers. So we must
>> define some factor for iio_convert_raw_to_processed() to scale from raw value range
>> to int value range. We could report raw values but this would be an improper abstraction
>> from chip specific differences.
> 
> Hmm. I can see we perhaps need some mapping, but is there a concept of standard scale
> for existing input accelerometers?  How is this done to give for other input devices
> such as touch screens?  I'd expect to see a separation between scale, and range.
> 
> 
>> 
>> BTW: the range (and therefore the factor) is reported through the evdev driver to user-space
>> (evtest reports Min and Max as you can see in the example).
>> 
>> The most important thing is that this is a hardware independent definition. Every accelerometer
>> chip will report this range. So you can easily upgrade hardware or switch accelerometers
>> without touching user-space calibration. Like you can replace ethernet controller chips but
>> networking works the same with all of them.
> 
> Agreed, it needs to be hardware independent by the time it hits userspace, but I would
> have thought that scaling would be done in input, rather than IIO. It's hardly
> a problem unique to our usecase!
> 
> Perhaps Dmitry can give some advice on this.

Yes, that would be helpful.

> 
>> 
>> 
>> Hm. Is there an alternative to attach such private data to an struct iio_dev
>> allocated by someone else? I have not found one yet.
>> 
>> Or can I add some void *input_mapping; to struct iio_dev? Depending on
>> #if defined(CONFIG_IIO_INPUT_BRIDGE)?
> 
> Yes, add a new element.

Ok, works fine.

I already have found one case of iio accelerometer driver where it did make a problem
not using a special element.

>>> 
>>> iio_input_find_accel_channel(indio_dev, chan, &numchans);
>>> iio_input_register_device(indio_dev, chan, numchans);  
>> 
>> Well, that looks like it needs some temporary storage of dynamic size
>> and loop twice over channels for no functional benefit.
> 
> Use fixed size. The worst that happens is we end up with it being
> an entry larger that it needs to be.
> 
>> And handle the
>> special case of numchans == 0 (the proposed code simply does not call
>> iio_input_register_accel_channel and does not register anything).
>> 
>> So I'd prefer to follow the "KISS" principle and register single channels
>> instead of a set of channels.
> 
> Well we disagree on this.  A singleton approach like used here
> is to my mind not KISS.  I would rather see what is there then
> act as two simple steps, rather than interleave two different
> actions with a totally different path for the first channel found.
> If there is only one channel you just built a load of infrastructure
> that makes no sense.  If you scan first then you can know that
> before building anything.

Ok, this is more a matter of taste and resource requirements can probably
be neglected. I'll update the driver.

So in summary, I'll post a v3 that fixes some bugs of v2 (because we need
them fixed for our production systems as well).

Then it is up to you if you want to take this approach or want to write
a full version following your concept. Or if it is possible as I assume, we
can have both.

BR and thanks,
Nikolaus



More information about the Letux-kernel mailing list