[PATCH] Revert "Input: bma150 - extend chip detection for bma180"

[Hans de Goede]

HI,

On 12-09-16 19:20, H. Nikolaus Schaller wrote:
> Hi,
>
>> Am 12.09.2016 um 16:44 schrieb Hans de Goede <hdegoede at redhat.com>:
>>
>> Hi,
>>
>> On 12-09-16 16:31, H. Nikolaus Schaller wrote:
>>> Hi,
>>>
>>>> Am 11.09.2016 um 18:43 schrieb Hans de Goede <hdegoede at redhat.com>:
>>>>
>>>> This reverts commit ef3714fdbc8d ("Input: bma150 - extend chip
>>>> detection for bma180").
>>>>
>>>> The bma180 is not compatible with the bma150 at all, it has 14 bits
>>>> resolution instead of 10, and it has quite different control registers.
>>>>
>>>> Treating the bma180 as a bma150 wrt its data registers will just result
>>>> in throwing away the lowest 4 bits, which is not too bad. But the ctrl
>>>> registers are a different story.
>>>>
>>>> It may be that things happen to just work (I don't have a bma180 to
>>>> test with) but that certainly does not make this right.
>>>
>>> Yes, looks as if your observation is right. Thanks for pointing this out!
>>>
>>> This did need me some research to find an answer...
>>>
>>> If I remember correctly, the original patch was based on a recommendation
>>> from someone I don't remember, who said that the chips are the same in a
>>> different package. So we added the chip_id and it worked immediately as
>>> expected. It looks as if we did not check the data sheets. So we did not
>>> question this recommendation.
>>>
>>>>
>>>> Removing the bma180 id also removes overlap wrt the ids in the iio
>>>> bma180 driver which does treat the bma180 properly.
>>>
>>> Nack.
>>>
>>> The problem we get is that an iio driver is not an input driver and can't
>>> easily replace it.
>>
>> Actually almost all accelerometer drivers in the Linux kernel are
>> iio drivers,
>
> Yes, that is a good move indeed to make them more general.
>
>> the bma150 driver is the odd duck out,
>
> Indeed it seems to be in /misc for some reason.
>
>> that is why
>> we've iio-sensor-proxy for apps which want the accelerometer
>> to behave as an input device:
>>
>> https://github.com/hadess/iio-sensor-proxy
>
> Does this provide an input device /dev/input/event?
> From a first look it seems to provide a D-Bus abstraction which is
> not the same as an /dev/input/event.

It also provides a /dev/input/event node through uinput:

https://github.com/hadess/iio-sensor-proxy/blob/master/src/fake-input-accelerometer.c


> What about latency?
> A kernel driver can react on a sensor interrupt, queue results
> and sleep.
>
> Isn't a user-space process more resource hungry than a kernel thread?
>
>>
>>> An input driver can be used for gesture applications (e.g. detecting
>>> the device has been turned upside down) and can report X/Y/Z coordinates
>>> to e.g. X11 for games similar to a mouse or joystick (which the iio driver
>>> doesn't).
>>
>> See above.
>>
>>> So it should remain configurable which of both driver options is loaded,
>>> to match user space API needs.
>>>
>>> BTW: id overlap is only a problem if both drivers are configured in parallel.
>>
>> Right, so it is "only" a problem to any generic distro which tries to
>> support both bma150 and bma250 accelerometers, as both the input
>> bma150 as well as the iio bma250 driver claim to be bma180 compatible.
>
> Well, it is only a problem if such a generic distro is really run on a device
> which has a bma180 defined in DT. There do not seem to exist many of these.

True, still the bma180 compatible is simply just wrong here, and as such
it really should be removed (it really should never have been added).

>>> Next, I have tried to find out which devices really use the bma150 and bma180.
>>>
>>> It appears that no in-tree device uses the bma150 while the GTA04 uses
>>> the bma180 (in DT), but not in any defconfig (the GTA04 specific config
>>> is not upstreamable since omap2plus_defconfig exists).
>>>
>>> In user-space the GTA04 requires and actively uses this input driver for Replicant.
>>>
>>> So I would conclude that this revert does not improve/fix any device using a
>>> bma150 (if it exists at all), but breaks an existing device.
>>>
>>> What options do we have?
>>>
>>> a) add proper register number constants and choose conditionally (where they differ)
>>> b) drop bma150 support completely and change registers for bma180
>>> c) clone the bma150 input driver into an bma180 input driver and fix registers
>>> d) extend the bma180 iio driver to optionally provide an input device
>>> e) write a generic input/iio-accel wrapper (which should work with any iio accelerometer)
>>>
>>> I would favour approach e)
>>
>> Good, because that solution already exists :) See:
>> https://github.com/hadess/iio-sensor-proxy
>
> No, it is yet another option. I would formulate it as:
>
> f) break user-space compatibility for existing systems and write / require
>    a user-space workaround.
>
> This is something which is the least favorite in my view.

I'm pretty sure that no-one, really no-one wants to do
evdev emulation for iio devices in the kernel; and there
is nothing wrong with doing it in userspace.

> Since we have no control over the user space people have installed and IMHO
> a kernel shouldn't break APIs too often or only for really good reasons
> (e.g. completely new functions).
>
> And we have no means to force user-space developers to integrate such daemons :(
>
> So we break a device by upgrading to 4.9-kernel but can't give a solution.

The only user of this seems to be the gta04, and I believe gta04 users
will not just jump to 4.9 without getting that through some sort
of central distribution point which can make sure userland gets
fixed first.

>>> but it has an issue I have no solution for:
>>>
>>> 	how to define in DT (or CONFIG?) which iio accelerometer(s) should
>>> 	be wrapped and presented as input device(s).
>>
>> I believe currently iio-sensor-proxy simply wraps all accelerometers
>> it can find, which seems the right thing to do. Usually we're running
>> a generic desktop-ish OS / distro which wants these devices
>> to be available as input devices;
>> for special cases like actual
>> robots and stuff running Linux, iio-sensor-proxy can simply be
>> disabled; or not installed at all.
>
> Well, the GTA04 we talk about falls into a third category: a mobile device.
>
> Which is not running a desktop-ish OS / distro. It is running Replicant,

For replicant there is:

https://github.com/01org/android-iio-sensors-hal

> QtMoko, SHR, FSO or other user spaces which partially rely on /dev/input/event
> without starting a daemon.

So either these have a gta04 specific version, which can then
be fixed in parallel with the kernel, like one would do
with normal distros in a case like this. Or these are generic
in which case having them work with iio accelerometers is
a good thing in general, and they may even already work.

> At least in current releases and we have little
> influence to have them use a new daemon.
>
> So I would strongly prefer if we can solve this in the kernel and keep the
> user-space API for the bma180 input driver stable.
>
>>
>> So all in all I believe that this is a solved problem, since
>> solution e. from above is already implemented.
>
> Well it is also solved if we do not revert the patch at all, because it
> works in practice (even if not exactly correct) and is compatible to
> existing user-space.
>
> In summary I don't like option f) because it creates more problems than
> it solves.
>
> If e) is ruled out, IMHO the second best option seems to be d) to add a
> CONFIG_BMA180_INPUT_DEVICE and make it present some additional /dev/input/event
> in parallel to the iio interface.

Ugh, no we don't need more hacks here. I would greatly prefer to
just keep the "bma180" is in the bma150 input driver over adding
evdev event generation to iio drivers (either at the driver level
or the core).

I think that the best solution might be to do do something like this
in drivers/input/misc/bma150.c

static const struct i2c_device_id bma150_id[] = {
         { "bma150", 0 },
#ifndef CONFIG_BMA180 /* Avoid conflict with iio bma180 driver */
         { "bma180", 0 },
#endif
         { "smb380", 0 },
         { "bma023", 0 },
         { }
};

Then people relying on this can keep using it, and iio user
do not have to worry about it.

Regards,

Hans



> I already have a similar approach in my to-be-upstreamed queue for the tsc2007
> touch screen driver to provide additional iio channels for raw values, chip
> temperature and the auxiliary general purpose ADC (which is used as ambient
> light sensor in the GTA04).
>
> So let's develop some patch for drivers/iio/accel/bma180.c before we revert
> this bma150 patch and have nothing.
>
> BR,
> Nikolaus Schaller
>
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>>
>>> Well, it could be as simple as defining a virtual "input-iio-accel" wrapper
>>> driver with no real hardware behind and provide a reference to the iio DT node.
>>> But I think such virtual devices are against DT style.
>>>
>>> So I don't know how to implement it in an acceptable way.
>>>
>>> Ideas?
>>>
>>> BR and thanks,
>>> Nikolaus Schaller
>>>
>>>>
>>>> Cc: Dr. H. Nikolaus Schaller <hns at goldelico.com>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>> drivers/input/misc/bma150.c | 4 +---
>>>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/input/misc/bma150.c b/drivers/input/misc/bma150.c
>>>> index b0d4453..cae4832 100644
>>>> --- a/drivers/input/misc/bma150.c
>>>> +++ b/drivers/input/misc/bma150.c
>>>> @@ -70,7 +70,6 @@
>>>> #define BMA150_CFG_5_REG	0x11
>>>>
>>>> #define BMA150_CHIP_ID		2
>>>> -#define BMA180_CHIP_ID		3
>>>> #define BMA150_CHIP_ID_REG	BMA150_DATA_0_REG
>>>>
>>>> #define BMA150_ACC_X_LSB_REG	BMA150_DATA_2_REG
>>>> @@ -539,7 +538,7 @@ static int bma150_probe(struct i2c_client *client,
>>>> 	}
>>>>
>>>> 	chip_id = i2c_smbus_read_byte_data(client, BMA150_CHIP_ID_REG);
>>>> -	if (chip_id != BMA150_CHIP_ID && chip_id != BMA180_CHIP_ID) {
>>>> +	if (chip_id != BMA150_CHIP_ID) {
>>>> 		dev_err(&client->dev, "BMA150 chip id error: %d\n", chip_id);
>>>> 		return -EINVAL;
>>>> 	}
>>>> @@ -643,7 +642,6 @@ static UNIVERSAL_DEV_PM_OPS(bma150_pm, bma150_suspend, bma150_resume, NULL);
>>>>
>>>> static const struct i2c_device_id bma150_id[] = {
>>>> 	{ "bma150", 0 },
>>>> -	{ "bma180", 0 },
>>>> 	{ "smb380", 0 },
>>>> 	{ "bma023", 0 },
>>>> 	{ }
>>>> --
>>>> 2.9.3
>>>>
>>>
>