Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thu, May 21, 2015 at 01:42:26PM +0200, Geert Uytterhoeven wrote: > From: Laurent Pinchart > > 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 will contain the more specific > "adi,adxl345" or "adi,adxl346" value first. This prevents the device > node from being matched with the adxl34x driver. > > Fix this by adding an OF match table with an "adi,adxl345" compatible > entry. There's no need to add the "adi,adxl346" entry as the ADXL346 is > backward-compatible with the ADXL345 with differences handled by runtime > detection of the device model. > > Signed-off-by: Laurent Pinchart > Reviewed-by: Wolfram Sang > Signed-off-by: Geert Uytterhoeven > --- > v2: > - Add Reviewed-by. Reviewed-by: Simon Horman -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/2] input: adxl34x: Add OF match support
From: Laurent Pinchart
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 will contain the more specific
"adi,adxl345" or "adi,adxl346" value first. This prevents the device
node from being matched with the adxl34x driver.
Fix this by adding an OF match table with an "adi,adxl345" compatible
entry. There's no need to add the "adi,adxl346" entry as the ADXL346 is
backward-compatible with the ADXL345 with differences handled by runtime
detection of the device model.
Signed-off-by: Laurent Pinchart
Reviewed-by: Wolfram Sang
Signed-off-by: Geert Uytterhoeven
---
v2:
- Add Reviewed-by.
drivers/input/misc/adxl34x-i2c.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/input/misc/adxl34x-i2c.c b/drivers/input/misc/adxl34x-i2c.c
index 470bfd6f08304c67..bdb5d03b296ec6db 100644
--- a/drivers/input/misc/adxl34x-i2c.c
+++ b/drivers/input/misc/adxl34x-i2c.c
@@ -10,6 +10,7 @@
#include/* BUS_I2C */
#include
#include
+#include
#include
#include
#include "adxl34x.h"
@@ -135,11 +136,31 @@ 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[] = {
+ /*
+* The ADXL346 is backward-compatible with the ADXL345. Differences are
+* handled by runtime detection of the device model, there's thus no
+* need for listing the "adi,adxl346" compatible value explicitly.
+*/
+ { .compatible = "adi,adxl345", },
+ /*
+* Deprecated, DT nodes should use one or more of the device-specific
+* compatible values "adi,adxl345" and "adi,adxl346".
+*/
+ { .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,
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
Hi Dmitry, On Thursday 15 January 2015 13:50:53 Dmitry Torokhov wrote: > On Thu, Jan 15, 2015 at 11:34:00PM +0200, Laurent Pinchart wrote: > > On Thursday 15 January 2015 13:06:32 Dmitry Torokhov wrote: > >> On Thu, Jan 15, 2015 at 10:34:29PM +0200, Laurent Pinchart wrote: > >>> On Thursday 15 January 2015 21:00:37 Geert Uytterhoeven wrote: > On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov wrote: > > I still do not understand what we are trying to fix here. Why is > > "adi,adxl34x" compatible string no good anymore? If we start using > > exact models and the physical device does not match do we abort > > probe? What is the problem that we are solving here? > > Because there's no guarantee that the driver actually supports all > "adi,adxl34" with = 0..9, some of which don't exist yet. > >> > >> So, what? When we encounter such devices and decide that we actually > >> want a different driver for them (instead of enhancing the existing one) > >> we'll give them their own compatible string. It's not like "adi,adxl348" > >> will automatically match with "adi,adxl34x", is it? > > > > Please remember that compatible strings shouldn't be decided based on a > > particular operating system implementation. > > In this case we can never have generic compatible strings of whatever > since in 10 years there might appear an OS that handles them > differently. Let's be a bit realistic here as well. I don't agree with you. The generic compatible strings should express compatibility with a hardware interface to the system, not compatibility with particular drivers on particular operating systems. We can thus have generic compatible strings without taking the OS into account. > >>> That's one of the reasons. Another one is that the adxl34x driver > >>> won't match DT nodes that list the "adi,adxl34x" compatible value in > >>> positions other than the first. > >> > >> Will it match anything else in the position other than 1st (i.e. > >> if device has compatible sting like "adi,adxl345-1", "adi,adxl345")? > > > Why "adi,adxl34x" is special? > > > > The problem on the driver side is that the automatic I2C DT compatible to > > device name matching implementation only tries to match with the first > > compatible string without looking at the other ones. The driver thus needs > > to be enhanced with an explicit OF match table to be able to match on DT > > nodes that specify "adi,adxl345" or "adi,adxl346" as the first compatible > > entry. > > Why don't we enhance I2C core instead to do proper matching? That would also be possible, but I believe that patch 1/2 is still the right thing to do, in which case patch 2/2 is required anyway. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
Hello. On 01/15/2015 11:34 PM, Laurent Pinchart wrote: I still do not understand what we are trying to fix here. Why is "adi,adxl34x" compatible string no good anymore? If we start using exact models and the physical device does not match do we abort probe? What is the problem that we are solving here? Because there's no guarantee that the driver actually supports all "adi,adxl34" with = 0..9, some of which don't exist yet. That's one of the reasons. Another one is that the adxl34x driver won't match DT nodes that list the "adi,adxl34x" compatible value in positions other than the first. Let's also not forget that wildcards in the "compatible" prop are not really allowed. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thu, Jan 15, 2015 at 11:34:00PM +0200, Laurent Pinchart wrote: > On Thursday 15 January 2015 13:06:32 Dmitry Torokhov wrote: > > On Thu, Jan 15, 2015 at 10:34:29PM +0200, Laurent Pinchart wrote: > > > On Thursday 15 January 2015 21:00:37 Geert Uytterhoeven wrote: > > > > On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov wrote: > > > > > I still do not understand what we are trying to fix here. Why is > > > > > "adi,adxl34x" compatible string no good anymore? If we start using > > > > > exact models and the physical device does not match do we abort probe? > > > > > What is the problem that we are solving here? > > > > > > > > Because there's no guarantee that the driver actually supports all > > > > "adi,adxl34" with = 0..9, some of which don't exist yet. > > > > So, what? When we encounter such devices and decide that we actually > > want a different driver for them (instead of enhancing the existing one) > > we'll give them their own compatible string. It's not like "adi,adxl348" > > will automatically match with "adi,adxl34x", is it? > > Please remember that compatible strings shouldn't be decided based on a > particular operating system implementation. In this case we can never have generic compatible strings of whatever since in 10 years there might appear an OS that handles them differently. Let's be a bit realistic here as well. > > > > That's one of the reasons. Another one is that the adxl34x driver won't > > > match DT nodes that list the "adi,adxl34x" compatible value in positions > > > other than the first. > > > > Will it match anything else in the position other than 1st (i.e. > > if device has compatible sting like "adi,adxl345-1", "adi,adxl345")? > > Why "adi,adxl34x" is special? > > The problem on the driver side is that the automatic I2C DT compatible to > device name matching implementation only tries to match with the first > compatible string without looking at the other ones. The driver thus needs to > be enhanced with an explicit OF match table to be able to match on DT nodes > that specify "adi,adxl345" or "adi,adxl346" as the first compatible entry. Why don't we enhance I2C core instead to do proper matching? -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thursday 15 January 2015 13:06:32 Dmitry Torokhov wrote: > On Thu, Jan 15, 2015 at 10:34:29PM +0200, Laurent Pinchart wrote: > > On Thursday 15 January 2015 21:00:37 Geert Uytterhoeven wrote: > > > On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov wrote: > > > > I still do not understand what we are trying to fix here. Why is > > > > "adi,adxl34x" compatible string no good anymore? If we start using > > > > exact models and the physical device does not match do we abort probe? > > > > What is the problem that we are solving here? > > > > > > Because there's no guarantee that the driver actually supports all > > > "adi,adxl34" with = 0..9, some of which don't exist yet. > > So, what? When we encounter such devices and decide that we actually > want a different driver for them (instead of enhancing the existing one) > we'll give them their own compatible string. It's not like "adi,adxl348" > will automatically match with "adi,adxl34x", is it? Please remember that compatible strings shouldn't be decided based on a particular operating system implementation. > > That's one of the reasons. Another one is that the adxl34x driver won't > > match DT nodes that list the "adi,adxl34x" compatible value in positions > > other than the first. > > Will it match anything else in the position other than 1st (i.e. > if device has compatible sting like "adi,adxl345-1", "adi,adxl345")? > Why "adi,adxl34x" is special? The problem on the driver side is that the automatic I2C DT compatible to device name matching implementation only tries to match with the first compatible string without looking at the other ones. The driver thus needs to be enhanced with an explicit OF match table to be able to match on DT nodes that specify "adi,adxl345" or "adi,adxl346" as the first compatible entry. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thu, Jan 15, 2015 at 10:34:29PM +0200, Laurent Pinchart wrote: > On Thursday 15 January 2015 21:00:37 Geert Uytterhoeven wrote: > > On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov wrote: > > > I still do not understand what we are trying to fix here. Why is > > > "adi,adxl34x" compatible string no good anymore? If we start using exact > > > models and the physical device does not match do we abort probe? What is > > > the problem that we are solving here? > > > > Because there's no guarantee that the driver actually supports all > > "adi,adxl34" with = 0..9, some of which don't exist yet. So, what? When we encounter such devices and decide that we actually want a different driver for them (instead of enhancing the existing one) we'll give them their own compatible string. It's not like "adi,adxl348" will automatically match with "adi,adxl34x", is it? > > That's one of the reasons. Another one is that the adxl34x driver won't match > DT nodes that list the "adi,adxl34x" compatible value in positions other than > the first. Will it match anything else in the position other than 1st (i.e. if device has compatible sting like "adi,adxl345-1", "adi,adxl345")? Why "adi,adxl34x" is special? Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thursday 15 January 2015 21:00:37 Geert Uytterhoeven wrote: > On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov wrote: > > I still do not understand what we are trying to fix here. Why is > > "adi,adxl34x" compatible string no good anymore? If we start using exact > > models and the physical device does not match do we abort probe? What is > > the problem that we are solving here? > > Because there's no guarantee that the driver actually supports all > "adi,adxl34" with = 0..9, some of which don't exist yet. That's one of the reasons. Another one is that the adxl34x driver won't match DT nodes that list the "adi,adxl34x" compatible value in positions other than the first. -- Regards, Laurent Pinchart -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thu, Jan 15, 2015 at 7:54 PM, Dmitry Torokhov
wrote:
> I still do not understand what we are trying to fix here. Why is
> "adi,adxl34x" compatible string no good anymore? If we start using exact
> models and the physical device does not match do we abort probe? What is
> the problem that we are solving here?
Because there's no guarantee that the driver actually supports all
"adi,adxl34" with = 0..9, some of which don't exist yet.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thu, Jan 15, 2015 at 06:45:33PM +0100, Geert Uytterhoeven wrote:
> On Thu, Jan 15, 2015 at 3:54 PM, 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 will contain the more specific
> > "adi,adxl345" or "adi,adxl346" value first. This prevents the device
> > node from being matched with the adxl34x driver.
> >
> > Fix this by adding an OF match table with an "adi,adxl345" compatible
> > entry. There's no need to add the "adi,adxl346" entry as the ADXL346 is
> > backward-compatible with the ADXL345 with differences handled by runtime
> > detection of the device model.
>
> Thanks!
>
> > Signed-off-by: Laurent Pinchart
>
> Acked-by: Geert Uytterhoeven
>
> > --- a/drivers/input/misc/adxl34x-i2c.c
> > +++ b/drivers/input/misc/adxl34x-i2c.c
>
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id adxl34x_of_id[] = {
> > + /*
> > +* The ADXL346 is backward-compatible with the ADXL345. Differences
> > are
> > +* handled by runtime detection of the device model, there's thus no
> > +* need for listing the "adi,adxl346" compatible value explicitly.
> > +*/
> > + { .compatible = "adi,adxl345", },
> > + /*
> > +* Deprecated, DT nodes should use one or more of the
> > device-specific
> > +* compatible values "adi,adxl345" and "adi,adxl346".
>
> Ideally, the two comments above are moved to a real DT binding document ;-)
>
> > +*/
> > + { .compatible = "adi,adxl34x", },
>
> I'd append "/* deprecated */" to the line above, so "git grep adxl34x"
> will show its deprecated status.
I still do not understand what we are trying to fix here. Why is
"adi,adxl34x" compatible string no good anymore? If we start using exact
models and the physical device does not match do we abort probe? What is
the problem that we are solving here?
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thu, Jan 15, 2015 at 3:54 PM, 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 will contain the more specific
> "adi,adxl345" or "adi,adxl346" value first. This prevents the device
> node from being matched with the adxl34x driver.
>
> Fix this by adding an OF match table with an "adi,adxl345" compatible
> entry. There's no need to add the "adi,adxl346" entry as the ADXL346 is
> backward-compatible with the ADXL345 with differences handled by runtime
> detection of the device model.
Thanks!
> Signed-off-by: Laurent Pinchart
Acked-by: Geert Uytterhoeven
> --- a/drivers/input/misc/adxl34x-i2c.c
> +++ b/drivers/input/misc/adxl34x-i2c.c
> +#ifdef CONFIG_OF
> +static const struct of_device_id adxl34x_of_id[] = {
> + /*
> +* The ADXL346 is backward-compatible with the ADXL345. Differences
> are
> +* handled by runtime detection of the device model, there's thus no
> +* need for listing the "adi,adxl346" compatible value explicitly.
> +*/
> + { .compatible = "adi,adxl345", },
> + /*
> +* Deprecated, DT nodes should use one or more of the device-specific
> +* compatible values "adi,adxl345" and "adi,adxl346".
Ideally, the two comments above are moved to a real DT binding document ;-)
> +*/
> + { .compatible = "adi,adxl34x", },
I'd append "/* deprecated */" to the line above, so "git grep adxl34x"
will show its deprecated status.
> + { }
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] input: adxl34x: Add OF match support
On Thu, Jan 15, 2015 at 04:54:15PM +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 will contain the more specific > "adi,adxl345" or "adi,adxl346" value first. This prevents the device > node from being matched with the adxl34x driver. > > Fix this by adding an OF match table with an "adi,adxl345" compatible > entry. There's no need to add the "adi,adxl346" entry as the ADXL346 is > backward-compatible with the ADXL345 with differences handled by runtime > detection of the device model. > > Signed-off-by: Laurent Pinchart Reviewed-by: Wolfram Sang signature.asc Description: Digital signature
[PATCH v2 2/2] input: adxl34x: Add OF match support
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 will contain the more specific
"adi,adxl345" or "adi,adxl346" value first. This prevents the device
node from being matched with the adxl34x driver.
Fix this by adding an OF match table with an "adi,adxl345" compatible
entry. There's no need to add the "adi,adxl346" entry as the ADXL346 is
backward-compatible with the ADXL345 with differences handled by runtime
detection of the device model.
Signed-off-by: Laurent Pinchart
---
drivers/input/misc/adxl34x-i2c.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/input/misc/adxl34x-i2c.c b/drivers/input/misc/adxl34x-i2c.c
index 416f47ddcc90..1e028f1f221c 100644
--- a/drivers/input/misc/adxl34x-i2c.c
+++ b/drivers/input/misc/adxl34x-i2c.c
@@ -10,6 +10,7 @@
#include/* BUS_I2C */
#include
#include
+#include
#include
#include
#include "adxl34x.h"
@@ -137,11 +138,31 @@ 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[] = {
+ /*
+* The ADXL346 is backward-compatible with the ADXL345. Differences are
+* handled by runtime detection of the device model, there's thus no
+* need for listing the "adi,adxl346" compatible value explicitly.
+*/
+ { .compatible = "adi,adxl345", },
+ /*
+* Deprecated, DT nodes should use one or more of the device-specific
+* compatible values "adi,adxl345" and "adi,adxl346".
+*/
+ { .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,
--
2.0.5
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
