| Message ID | 1418868923-13411-1-git-send-email-laurent.pinchart+renesas@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote: > 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 <laurent.pinchart+renesas@ideasonboard.com> > --- > 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).
Hi Wolfram, On Thursday 18 December 2014 09:21:51 Wolfram Sang wrote: > On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote: > > 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 > > <laurent.pinchart+renesas@ideasonboard.com> > > --- > > > > 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. 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.
On Thu, Dec 18, 2014 at 1:49 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > 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. FWIW, I'm the evil person who added the adxl entries to that file... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Thursday 18 December 2014 14:03:18 Geert Uytterhoeven wrote: > On Thu, Dec 18, 2014 at 1:49 PM, Laurent Pinchart wrote: > > 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. > > FWIW, I'm the evil person who added the adxl entries to that file... git-blame had already reported you. Do you think we should remove that compatible string ?
Hi Geert, On Thursday 18 December 2014 21:23:46 Laurent Pinchart wrote: > On Thursday 18 December 2014 14:03:18 Geert Uytterhoeven wrote: > > On Thu, Dec 18, 2014 at 1:49 PM, Laurent Pinchart wrote: > > > 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. > > > > FWIW, I'm the evil person who added the adxl entries to that file... > > git-blame had already reported you. > > Do you think we should remove that compatible string ? Ping ?
Hi Laurent, Wolfram, On Thu, Dec 18, 2014 at 8:23 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > On Thursday 18 December 2014 14:03:18 Geert Uytterhoeven wrote: >> On Thu, Dec 18, 2014 at 1:49 PM, Laurent Pinchart wrote: >> > 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. >> >> FWIW, I'm the evil person who added the adxl entries to that file... > > git-blame had already reported you. > > Do you think we should remove that compatible string ? Remove it from trivial-devices.txt, or from the .dts? Can't we just fix i2c_device_match(), to consider all compatible values of the device's node instead of just the first one when doing a legacy match? For now, I'd remove the "adi,adxl345" entry from sh73a0-kzm9g.dts, until matching is fixed. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 18, 2014 at 02:49:28PM +0200, Laurent Pinchart wrote: > Hi Wolfram, > > On Thursday 18 December 2014 09:21:51 Wolfram Sang wrote: > > On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote: > > > 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 > > > <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > > > > 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" :( > 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 2) also add "34x" as a compatible but mark it as deprecateed 3) delete "34x" from trivial devices Everyone OK with that? Thanks, Wolfram
On Thu, Jan 15, 2015 at 1:53 PM, Wolfram Sang <wsa@the-dreams.de> wrote: >> 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" :( Yep, pca953x => leds-pca953x >> 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 > 2) also add "34x" as a compatible but mark it as deprecateed > 3) delete "34x" from trivial devices > > Everyone OK with that? Sounds fine to me. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Wolfram, On Thursday 15 January 2015 13:53:22 Wolfram Sang wrote: > On Thu, Dec 18, 2014 at 02:49:28PM +0200, Laurent Pinchart wrote: > > On Thursday 18 December 2014 09:21:51 Wolfram Sang wrote: > >> On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote: > >>> 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 > >>> <laurent.pinchart+renesas@ideasonboard.com> > >>> --- > >>> > >>> 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. > > 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 ? > 2) also add "34x" as a compatible but mark it as deprecateed > 3) delete "34x" from trivial devices OK. > Everyone OK with that?
On Thursday 15 January 2015 16:19:19 Laurent Pinchart wrote: > On Thursday 15 January 2015 13:53:22 Wolfram Sang wrote: > > On Thu, Dec 18, 2014 at 02:49:28PM +0200, Laurent Pinchart wrote: > >> On Thursday 18 December 2014 09:21:51 Wolfram Sang wrote: > >>> On Thu, Dec 18, 2014 at 04:15:23AM +0200, Laurent Pinchart wrote: > >>>> 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 > >>>> <laurent.pinchart+renesas@ideasonboard.com> > >>>> --- > >>>> > >>>> 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. > > > > 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. > > 2) also add "34x" as a compatible but mark it as deprecateed > > 3) delete "34x" from trivial devices > > OK. > > > Everyone OK with that?
On Thu, Jan 15, 2015 at 3:19 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: >> > 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 ? Adxl34x is instantiated in: arch/arm/boot/dts/sh73a0-kzm9g.dts arch/arm/mach-shmobile/board-kzm9g.c arch/arm/mach-shmobile/board-mackerel.c arch/blackfin/mach-bf527/boards/tll6527m.c arch/blackfin/mach-bf537/boards/stamp.c arch/blackfin/mach-bf548/boards/ezkit.c arch/blackfin/mach-bf609/boards/ezkit.c The shmobile variants are all adxl345. The tll6527m is also an adxl345. For the remaining, it's not clear to me. I Googled a bit, but no luck. So they'll have to live with adxl345, too ;-) >> 2) also add "34x" as a compatible but mark it as deprecateed >> 3) delete "34x" from trivial devices > > OK. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > > 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. Well, I think different here, but let's skip this discussion as it is not really needed right now... > > 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. I agree. > > > 2) also add "34x" as a compatible but mark it as deprecateed > > > 3) delete "34x" from trivial devices > > > > OK. Yay :)
On Thursday 15 January 2015 15:36:37 Wolfram Sang wrote: > >>> 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. > > Well, I think different here, but let's skip this discussion as it is > not really needed right now... > > >> 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. > > I agree. > > >>> 2) also add "34x" as a compatible but mark it as deprecateed > >>> 3) delete "34x" from trivial devices > >> > >> OK. > > Yay :) I'll submit patches very soon.
diff --git a/drivers/input/misc/adxl34x-i2c.c b/drivers/input/misc/adxl34x-i2c.c index 416f47ddcc90..22687b738811 100644 --- a/drivers/input/misc/adxl34x-i2c.c +++ b/drivers/input/misc/adxl34x-i2c.c @@ -10,6 +10,7 @@ #include <linux/input.h> /* BUS_I2C */ #include <linux/i2c.h> #include <linux/module.h> +#include <linux/of.h> #include <linux/types.h> #include <linux/pm.h> #include "adxl34x.h" @@ -137,11 +138,21 @@ static const struct i2c_device_id adxl34x_id[] = { MODULE_DEVICE_TABLE(i2c, adxl34x_id); +#ifdef CONFIG_OF +static const struct of_device_id adxl34x_of_id[] = { + { .compatible = "adi,adxl34x", }, + { } +}; + +MODULE_DEVICE_TABLE(of, adxl34x_of_id); +#endif + static struct i2c_driver adxl34x_driver = { .driver = { .name = "adxl34x", .owner = THIS_MODULE, .pm = &adxl34x_i2c_pm, + .of_match_table = of_match_ptr(adxl34x_of_id), }, .probe = adxl34x_i2c_probe, .remove = adxl34x_i2c_remove,
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 <laurent.pinchart+renesas@ideasonboard.com> --- 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.