HI,
On 12-09-16 19:20, H. Nikolaus Schaller wrote:
Hi,
Am 12.09.2016 um 16:44 schrieb Hans de Goede <hdegoede@xxxxxxxxxx>:
Hi,
On 12-09-16 16:31, H. Nikolaus Schaller wrote:
Hi,
Am 11.09.2016 um 18:43 schrieb Hans de Goede <hdegoede@xxxxxxxxxx>:
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@xxxxxxxxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
