On Thursday 15 January 2015 16:19:19 Laurent Pinchart wrote:

On Thursday 15 January 2015 13:53:22 Wolfram Sang wrote:

quoted

On Thu, Dec 18, 2014 at 02:49:28PM +0200, Laurent Pinchart wrote:

quoted

On Thursday 18 December 2014 09:21:51 Wolfram Sang wrote:

quoted

On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote:

quoted

The I2C subsystem can match devices without explicit OF support based
on the part of their compatible property after the comma. However,
this mechanism uses the first compatible value only. For adxl34x OF
device nodes the compatible property should list the more specific
"adi,adxl345" or "adi,adxl346" value first and the "adi,adxl34x"
fallback value second. This prevents the device node from being
matched with the adxl34x driver.

Fix this by adding an OF match table with an "adi,adxl34x" compatible
entry.

Signed-off-by: Laurent Pinchart
[off-list ref]
---

 drivers/input/misc/adxl34x-i2c.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Another option would have been to add "adxl325" and "adxl326" entries
to the adxl34x_id I2C match table, but it would have had the drawback
of requiring a driver update for every new device.

AFAIK this is even required for compatible entries, to be as specific
as possible. I think this makes sense. With platform_ids, we already
had the problem that pca954x was too generic and was used for both GPIO
extenders and I2C muxers (IIRC).

There are three compatible strings defined for the ADXL345 and ADXL346
in Documentation/devicetree/bindings/i2c/trivial-devices.txt:
"adi,adxl345", "adi,adxl346", "adi,adxl34x". Given that the last one is
a fallback for the first two I don't see a need to add the specific
compatible strings to the driver for now. If a new totally incompatible
chip named ADXL347 comes out we will need a new driver which won't be
allowed to use the "adi,adxl34x" compatible string.

Been there, got bitten. We only found out too late, because one driver
was in i2c and the other in GPIO (or LED even?), both using "953x" :(

That seems like a development, review and/or merge process failure to me, I
wouldn't avoid generic compatible strings for that reason only.

quoted

quoted

An option would be to remove "adi,adxl34x" from
Documentation/devicetree/bindings/i2c/trivial-devices.txt, in which case
the driver should match explicitly on "adi,adxl345" and "adi,adxl346".
That might clash with the DT ABI stability requirements though.

I do prefer this:

1) add specific compatible values to the driver. We do those updates for
new devices all the time

Do you mean OF compatible values, or I2C match table entries ? I assume OF
compatible values.

As the ADXL346 is backward-compatible with the ADXL345, and as the driver
doesn't support the ADXL346-specific features, how about adding only the
adxl345 for now, and using compatible = "adi,adxl346", "adi,adxl345"; for
the ADXL346 ?

I spoke too fast. The driver supports ADXL346-specific features, but does so 
by detecting the device model at runtime.

I still believe it would make sense to list both the 346 and 345 models in DT 
for 346 devices, as they're compatible with the 345.

quoted

2) also add "34x" as a compatible but mark it as deprecateed
3) delete "34x" from trivial devices

OK.

quoted

Everyone OK with that?

-- 
Regards,

Laurent Pinchart