* [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices
@ 2022-03-24 23:43 Laurent Pinchart
2022-03-27 14:34 ` Jonathan Cameron
2022-03-27 19:25 ` Andy Shevchenko
0 siblings, 2 replies; 7+ messages in thread
From: Laurent Pinchart @ 2022-03-24 23:43 UTC (permalink / raw)
To: linux-iio
Cc: Jonathan Cameron, Lars-Peter Clausen, Sean Nyekjaer,
Jose Cazarin, Wolfram Sang, linux-i2c, laurent.pinchart
From: Jose Cazarin <joseespiriki@gmail.com>
When matching an OF device, the match mechanism tries all components of
the compatible property. This can result with a device matched with a
compatible string that isn't the first in the compatible list. For
instance, with a compatible property set to
compatible = "ti,dac081c081", "ti,dac5571";
the driver will match the second compatible string, as the first one
isn't listed in the of_device_id table. The device will however be named
"dac081c081" by the I2C core.
This causes an issue when identifying the chip. The probe function
receives a i2c_device_id that comes from the module's I2C device ID
table. There is no entry in that table for "dac081c081", which results
in a NULL pointer passed to the probe function.
To fix this, add chip_id information in the data field of the OF device
ID table, and retrieve it with of_device_get_match_data() for OF
devices.
Signed-off-by: Jose Cazarin <joseespiriki@gmail.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Changes since v1:
- Use device_get_match_data()
---
drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++----------
1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
index 4a3b8d875518..4f50e31dffb0 100644
--- a/drivers/iio/dac/ti-dac5571.c
+++ b/drivers/iio/dac/ti-dac5571.c
@@ -19,6 +19,7 @@
#include <linux/i2c.h>
#include <linux/module.h>
#include <linux/mod_devicetable.h>
+#include <linux/property.h>
#include <linux/regulator/consumer.h>
enum chip_id {
@@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client,
const struct dac5571_spec *spec;
struct dac5571_data *data;
struct iio_dev *indio_dev;
+ enum chip_id chip_id;
int ret, i;
indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client,
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = dac5571_channels;
- spec = &dac5571_spec[id->driver_data];
+ if (dev->of_node)
+ chip_id = (uintptr_t)device_get_match_data(dev);
+ else
+ chip_id = id->driver_data;
+
+ spec = &dac5571_spec[chip_id];
+
indio_dev->num_channels = spec->num_channels;
data->spec = spec;
@@ -385,15 +393,15 @@ static int dac5571_remove(struct i2c_client *i2c)
}
static const struct of_device_id dac5571_of_id[] = {
- {.compatible = "ti,dac5571"},
- {.compatible = "ti,dac6571"},
- {.compatible = "ti,dac7571"},
- {.compatible = "ti,dac5574"},
- {.compatible = "ti,dac6574"},
- {.compatible = "ti,dac7574"},
- {.compatible = "ti,dac5573"},
- {.compatible = "ti,dac6573"},
- {.compatible = "ti,dac7573"},
+ {.compatible = "ti,dac5571", .data = (void *)single_8bit},
+ {.compatible = "ti,dac6571", .data = (void *)single_10bit},
+ {.compatible = "ti,dac7571", .data = (void *)single_12bit},
+ {.compatible = "ti,dac5574", .data = (void *)quad_8bit},
+ {.compatible = "ti,dac6574", .data = (void *)quad_10bit},
+ {.compatible = "ti,dac7574", .data = (void *)quad_12bit},
+ {.compatible = "ti,dac5573", .data = (void *)quad_8bit},
+ {.compatible = "ti,dac6573", .data = (void *)quad_10bit},
+ {.compatible = "ti,dac7573", .data = (void *)quad_12bit},
{}
};
MODULE_DEVICE_TABLE(of, dac5571_of_id);
--
Regards,
Laurent Pinchart
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices
2022-03-24 23:43 [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart
@ 2022-03-27 14:34 ` Jonathan Cameron
2022-03-27 14:37 ` Laurent Pinchart
2022-03-27 19:25 ` Andy Shevchenko
1 sibling, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2022-03-27 14:34 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-iio, Lars-Peter Clausen, Sean Nyekjaer, Jose Cazarin,
Wolfram Sang, linux-i2c
On Fri, 25 Mar 2022 01:43:40 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> From: Jose Cazarin <joseespiriki@gmail.com>
>
> When matching an OF device, the match mechanism tries all components of
> the compatible property. This can result with a device matched with a
> compatible string that isn't the first in the compatible list. For
> instance, with a compatible property set to
>
> compatible = "ti,dac081c081", "ti,dac5571";
>
> the driver will match the second compatible string, as the first one
> isn't listed in the of_device_id table. The device will however be named
> "dac081c081" by the I2C core.
>
> This causes an issue when identifying the chip. The probe function
> receives a i2c_device_id that comes from the module's I2C device ID
> table. There is no entry in that table for "dac081c081", which results
> in a NULL pointer passed to the probe function.
>
> To fix this, add chip_id information in the data field of the OF device
> ID table, and retrieve it with of_device_get_match_data() for OF
> devices.
You forgot to update the patch description for the change to device_get_match_data()
One other thing inline.
Jonathan
>
> Signed-off-by: Jose Cazarin <joseespiriki@gmail.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Changes since v1:
>
> - Use device_get_match_data()
> ---
> drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++----------
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
> index 4a3b8d875518..4f50e31dffb0 100644
> --- a/drivers/iio/dac/ti-dac5571.c
> +++ b/drivers/iio/dac/ti-dac5571.c
> @@ -19,6 +19,7 @@
> #include <linux/i2c.h>
> #include <linux/module.h>
> #include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> #include <linux/regulator/consumer.h>
>
> enum chip_id {
> @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client,
> const struct dac5571_spec *spec;
> struct dac5571_data *data;
> struct iio_dev *indio_dev;
> + enum chip_id chip_id;
> int ret, i;
>
> indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client,
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->channels = dac5571_channels;
>
> - spec = &dac5571_spec[id->driver_data];
> + if (dev->of_node)
if (dev_fwnode(dev))
> + chip_id = (uintptr_t)device_get_match_data(dev);
> + else
> + chip_id = id->driver_data;
> +
> + spec = &dac5571_spec[chip_id];
> +
> indio_dev->num_channels = spec->num_channels;
> data->spec = spec;
>
> @@ -385,15 +393,15 @@ static int dac5571_remove(struct i2c_client *i2c)
> }
>
> static const struct of_device_id dac5571_of_id[] = {
> - {.compatible = "ti,dac5571"},
> - {.compatible = "ti,dac6571"},
> - {.compatible = "ti,dac7571"},
> - {.compatible = "ti,dac5574"},
> - {.compatible = "ti,dac6574"},
> - {.compatible = "ti,dac7574"},
> - {.compatible = "ti,dac5573"},
> - {.compatible = "ti,dac6573"},
> - {.compatible = "ti,dac7573"},
> + {.compatible = "ti,dac5571", .data = (void *)single_8bit},
> + {.compatible = "ti,dac6571", .data = (void *)single_10bit},
> + {.compatible = "ti,dac7571", .data = (void *)single_12bit},
> + {.compatible = "ti,dac5574", .data = (void *)quad_8bit},
> + {.compatible = "ti,dac6574", .data = (void *)quad_10bit},
> + {.compatible = "ti,dac7574", .data = (void *)quad_12bit},
> + {.compatible = "ti,dac5573", .data = (void *)quad_8bit},
> + {.compatible = "ti,dac6573", .data = (void *)quad_10bit},
> + {.compatible = "ti,dac7573", .data = (void *)quad_12bit},
> {}
> };
> MODULE_DEVICE_TABLE(of, dac5571_of_id);
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices
2022-03-27 14:34 ` Jonathan Cameron
@ 2022-03-27 14:37 ` Laurent Pinchart
2022-03-27 15:59 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Pinchart @ 2022-03-27 14:37 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, Lars-Peter Clausen, Sean Nyekjaer, Jose Cazarin,
Wolfram Sang, linux-i2c
Hi Jonathan,
On Sun, Mar 27, 2022 at 03:34:38PM +0100, Jonathan Cameron wrote:
> On Fri, 25 Mar 2022 01:43:40 +0200 Laurent Pinchart wrote:
>
> > From: Jose Cazarin <joseespiriki@gmail.com>
> >
> > When matching an OF device, the match mechanism tries all components of
> > the compatible property. This can result with a device matched with a
> > compatible string that isn't the first in the compatible list. For
> > instance, with a compatible property set to
> >
> > compatible = "ti,dac081c081", "ti,dac5571";
> >
> > the driver will match the second compatible string, as the first one
> > isn't listed in the of_device_id table. The device will however be named
> > "dac081c081" by the I2C core.
> >
> > This causes an issue when identifying the chip. The probe function
> > receives a i2c_device_id that comes from the module's I2C device ID
> > table. There is no entry in that table for "dac081c081", which results
> > in a NULL pointer passed to the probe function.
> >
> > To fix this, add chip_id information in the data field of the OF device
> > ID table, and retrieve it with of_device_get_match_data() for OF
> > devices.
>
> You forgot to update the patch description for the change to device_get_match_data()
>
> One other thing inline.
Thank you for the comments. I agree with both. Would you like me to send
a v3, or is this something you'd be happy fixing when applying the patch
to your tree ?
> > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Use device_get_match_data()
> > ---
> > drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++----------
> > 1 file changed, 18 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
> > index 4a3b8d875518..4f50e31dffb0 100644
> > --- a/drivers/iio/dac/ti-dac5571.c
> > +++ b/drivers/iio/dac/ti-dac5571.c
> > @@ -19,6 +19,7 @@
> > #include <linux/i2c.h>
> > #include <linux/module.h>
> > #include <linux/mod_devicetable.h>
> > +#include <linux/property.h>
> > #include <linux/regulator/consumer.h>
> >
> > enum chip_id {
> > @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client,
> > const struct dac5571_spec *spec;
> > struct dac5571_data *data;
> > struct iio_dev *indio_dev;
> > + enum chip_id chip_id;
> > int ret, i;
> >
> > indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client,
> > indio_dev->modes = INDIO_DIRECT_MODE;
> > indio_dev->channels = dac5571_channels;
> >
> > - spec = &dac5571_spec[id->driver_data];
> > + if (dev->of_node)
>
> if (dev_fwnode(dev))
>
> > + chip_id = (uintptr_t)device_get_match_data(dev);
> > + else
> > + chip_id = id->driver_data;
> > +
> > + spec = &dac5571_spec[chip_id];
> > +
> > indio_dev->num_channels = spec->num_channels;
> > data->spec = spec;
> >
> > @@ -385,15 +393,15 @@ static int dac5571_remove(struct i2c_client *i2c)
> > }
> >
> > static const struct of_device_id dac5571_of_id[] = {
> > - {.compatible = "ti,dac5571"},
> > - {.compatible = "ti,dac6571"},
> > - {.compatible = "ti,dac7571"},
> > - {.compatible = "ti,dac5574"},
> > - {.compatible = "ti,dac6574"},
> > - {.compatible = "ti,dac7574"},
> > - {.compatible = "ti,dac5573"},
> > - {.compatible = "ti,dac6573"},
> > - {.compatible = "ti,dac7573"},
> > + {.compatible = "ti,dac5571", .data = (void *)single_8bit},
> > + {.compatible = "ti,dac6571", .data = (void *)single_10bit},
> > + {.compatible = "ti,dac7571", .data = (void *)single_12bit},
> > + {.compatible = "ti,dac5574", .data = (void *)quad_8bit},
> > + {.compatible = "ti,dac6574", .data = (void *)quad_10bit},
> > + {.compatible = "ti,dac7574", .data = (void *)quad_12bit},
> > + {.compatible = "ti,dac5573", .data = (void *)quad_8bit},
> > + {.compatible = "ti,dac6573", .data = (void *)quad_10bit},
> > + {.compatible = "ti,dac7573", .data = (void *)quad_12bit},
> > {}
> > };
> > MODULE_DEVICE_TABLE(of, dac5571_of_id);
>
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices
2022-03-27 16:00 ` Jonathan Cameron
@ 2022-03-27 15:57 ` Laurent Pinchart
0 siblings, 0 replies; 7+ messages in thread
From: Laurent Pinchart @ 2022-03-27 15:57 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-iio, Lars-Peter Clausen, Sean Nyekjaer, Jose Cazarin,
Wolfram Sang, linux-i2c
On Sun, Mar 27, 2022 at 05:00:05PM +0100, Jonathan Cameron wrote:
> On Sun, 27 Mar 2022 16:59:01 +0100 Jonathan Cameron wrote:
> > On Sun, 27 Mar 2022 17:37:27 +0300 Laurent Pinchart wrote:
> > > On Sun, Mar 27, 2022 at 03:34:38PM +0100, Jonathan Cameron wrote:
> > > > On Fri, 25 Mar 2022 01:43:40 +0200 Laurent Pinchart wrote:
> > > >
> > > > > From: Jose Cazarin <joseespiriki@gmail.com>
> > > > >
> > > > > When matching an OF device, the match mechanism tries all components of
> > > > > the compatible property. This can result with a device matched with a
> > > > > compatible string that isn't the first in the compatible list. For
> > > > > instance, with a compatible property set to
> > > > >
> > > > > compatible = "ti,dac081c081", "ti,dac5571";
> > > > >
> > > > > the driver will match the second compatible string, as the first one
> > > > > isn't listed in the of_device_id table. The device will however be named
> > > > > "dac081c081" by the I2C core.
> > > > >
> > > > > This causes an issue when identifying the chip. The probe function
> > > > > receives a i2c_device_id that comes from the module's I2C device ID
> > > > > table. There is no entry in that table for "dac081c081", which results
> > > > > in a NULL pointer passed to the probe function.
> > > > >
> > > > > To fix this, add chip_id information in the data field of the OF device
> > > > > ID table, and retrieve it with of_device_get_match_data() for OF
> > > > > devices.
> > > >
> > > > You forgot to update the patch description for the change to device_get_match_data()
> > > >
> > > > One other thing inline.
> > >
> > > Thank you for the comments. I agree with both. Would you like me to send
> > > a v3, or is this something you'd be happy fixing when applying the patch
> > > to your tree ?
> >
> > I've applied it how with those changes. However, it's on my fixes-togreg branch
> now.
>
> I can't type on Sundays :)
Looks like I can't update commit messages on Fridays, so we're even ;-)
> > and I don't plan to push that out until after rc1 when I can rebase it on something
> > 'stable'.
Sure, no problem. There's no urgency, it's not v5.18 material.
> > > > > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com>
> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > > ---
> > > > > Changes since v1:
> > > > >
> > > > > - Use device_get_match_data()
> > > > > ---
> > > > > drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++----------
> > > > > 1 file changed, 18 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
> > > > > index 4a3b8d875518..4f50e31dffb0 100644
> > > > > --- a/drivers/iio/dac/ti-dac5571.c
> > > > > +++ b/drivers/iio/dac/ti-dac5571.c
> > > > > @@ -19,6 +19,7 @@
> > > > > #include <linux/i2c.h>
> > > > > #include <linux/module.h>
> > > > > #include <linux/mod_devicetable.h>
> > > > > +#include <linux/property.h>
> > > > > #include <linux/regulator/consumer.h>
> > > > >
> > > > > enum chip_id {
> > > > > @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client,
> > > > > const struct dac5571_spec *spec;
> > > > > struct dac5571_data *data;
> > > > > struct iio_dev *indio_dev;
> > > > > + enum chip_id chip_id;
> > > > > int ret, i;
> > > > >
> > > > > indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > > > @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client,
> > > > > indio_dev->modes = INDIO_DIRECT_MODE;
> > > > > indio_dev->channels = dac5571_channels;
> > > > >
> > > > > - spec = &dac5571_spec[id->driver_data];
> > > > > + if (dev->of_node)
> > > >
> > > > if (dev_fwnode(dev))
> > > >
> > > > > + chip_id = (uintptr_t)device_get_match_data(dev);
> > > > > + else
> > > > > + chip_id = id->driver_data;
> > > > > +
> > > > > + spec = &dac5571_spec[chip_id];
> > > > > +
> > > > > indio_dev->num_channels = spec->num_channels;
> > > > > data->spec = spec;
> > > > >
> > > > > @@ -385,15 +393,15 @@ static int dac5571_remove(struct i2c_client *i2c)
> > > > > }
> > > > >
> > > > > static const struct of_device_id dac5571_of_id[] = {
> > > > > - {.compatible = "ti,dac5571"},
> > > > > - {.compatible = "ti,dac6571"},
> > > > > - {.compatible = "ti,dac7571"},
> > > > > - {.compatible = "ti,dac5574"},
> > > > > - {.compatible = "ti,dac6574"},
> > > > > - {.compatible = "ti,dac7574"},
> > > > > - {.compatible = "ti,dac5573"},
> > > > > - {.compatible = "ti,dac6573"},
> > > > > - {.compatible = "ti,dac7573"},
> > > > > + {.compatible = "ti,dac5571", .data = (void *)single_8bit},
> > > > > + {.compatible = "ti,dac6571", .data = (void *)single_10bit},
> > > > > + {.compatible = "ti,dac7571", .data = (void *)single_12bit},
> > > > > + {.compatible = "ti,dac5574", .data = (void *)quad_8bit},
> > > > > + {.compatible = "ti,dac6574", .data = (void *)quad_10bit},
> > > > > + {.compatible = "ti,dac7574", .data = (void *)quad_12bit},
> > > > > + {.compatible = "ti,dac5573", .data = (void *)quad_8bit},
> > > > > + {.compatible = "ti,dac6573", .data = (void *)quad_10bit},
> > > > > + {.compatible = "ti,dac7573", .data = (void *)quad_12bit},
> > > > > {}
> > > > > };
> > > > > MODULE_DEVICE_TABLE(of, dac5571_of_id);
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices
2022-03-27 14:37 ` Laurent Pinchart
@ 2022-03-27 15:59 ` Jonathan Cameron
2022-03-27 16:00 ` Jonathan Cameron
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2022-03-27 15:59 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-iio, Lars-Peter Clausen, Sean Nyekjaer, Jose Cazarin,
Wolfram Sang, linux-i2c
On Sun, 27 Mar 2022 17:37:27 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> Hi Jonathan,
>
> On Sun, Mar 27, 2022 at 03:34:38PM +0100, Jonathan Cameron wrote:
> > On Fri, 25 Mar 2022 01:43:40 +0200 Laurent Pinchart wrote:
> >
> > > From: Jose Cazarin <joseespiriki@gmail.com>
> > >
> > > When matching an OF device, the match mechanism tries all components of
> > > the compatible property. This can result with a device matched with a
> > > compatible string that isn't the first in the compatible list. For
> > > instance, with a compatible property set to
> > >
> > > compatible = "ti,dac081c081", "ti,dac5571";
> > >
> > > the driver will match the second compatible string, as the first one
> > > isn't listed in the of_device_id table. The device will however be named
> > > "dac081c081" by the I2C core.
> > >
> > > This causes an issue when identifying the chip. The probe function
> > > receives a i2c_device_id that comes from the module's I2C device ID
> > > table. There is no entry in that table for "dac081c081", which results
> > > in a NULL pointer passed to the probe function.
> > >
> > > To fix this, add chip_id information in the data field of the OF device
> > > ID table, and retrieve it with of_device_get_match_data() for OF
> > > devices.
> >
> > You forgot to update the patch description for the change to device_get_match_data()
> >
> > One other thing inline.
>
> Thank you for the comments. I agree with both. Would you like me to send
> a v3, or is this something you'd be happy fixing when applying the patch
> to your tree ?
I've applied it how with those changes. However, it's on my fixes-togreg branch
and I don't plan to push that out until after rc1 when I can rebase it on something
'stable'.
Thanks,
Jonathan
>
> > > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com>
> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > > Changes since v1:
> > >
> > > - Use device_get_match_data()
> > > ---
> > > drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++----------
> > > 1 file changed, 18 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
> > > index 4a3b8d875518..4f50e31dffb0 100644
> > > --- a/drivers/iio/dac/ti-dac5571.c
> > > +++ b/drivers/iio/dac/ti-dac5571.c
> > > @@ -19,6 +19,7 @@
> > > #include <linux/i2c.h>
> > > #include <linux/module.h>
> > > #include <linux/mod_devicetable.h>
> > > +#include <linux/property.h>
> > > #include <linux/regulator/consumer.h>
> > >
> > > enum chip_id {
> > > @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client,
> > > const struct dac5571_spec *spec;
> > > struct dac5571_data *data;
> > > struct iio_dev *indio_dev;
> > > + enum chip_id chip_id;
> > > int ret, i;
> > >
> > > indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client,
> > > indio_dev->modes = INDIO_DIRECT_MODE;
> > > indio_dev->channels = dac5571_channels;
> > >
> > > - spec = &dac5571_spec[id->driver_data];
> > > + if (dev->of_node)
> >
> > if (dev_fwnode(dev))
> >
> > > + chip_id = (uintptr_t)device_get_match_data(dev);
> > > + else
> > > + chip_id = id->driver_data;
> > > +
> > > + spec = &dac5571_spec[chip_id];
> > > +
> > > indio_dev->num_channels = spec->num_channels;
> > > data->spec = spec;
> > >
> > > @@ -385,15 +393,15 @@ static int dac5571_remove(struct i2c_client *i2c)
> > > }
> > >
> > > static const struct of_device_id dac5571_of_id[] = {
> > > - {.compatible = "ti,dac5571"},
> > > - {.compatible = "ti,dac6571"},
> > > - {.compatible = "ti,dac7571"},
> > > - {.compatible = "ti,dac5574"},
> > > - {.compatible = "ti,dac6574"},
> > > - {.compatible = "ti,dac7574"},
> > > - {.compatible = "ti,dac5573"},
> > > - {.compatible = "ti,dac6573"},
> > > - {.compatible = "ti,dac7573"},
> > > + {.compatible = "ti,dac5571", .data = (void *)single_8bit},
> > > + {.compatible = "ti,dac6571", .data = (void *)single_10bit},
> > > + {.compatible = "ti,dac7571", .data = (void *)single_12bit},
> > > + {.compatible = "ti,dac5574", .data = (void *)quad_8bit},
> > > + {.compatible = "ti,dac6574", .data = (void *)quad_10bit},
> > > + {.compatible = "ti,dac7574", .data = (void *)quad_12bit},
> > > + {.compatible = "ti,dac5573", .data = (void *)quad_8bit},
> > > + {.compatible = "ti,dac6573", .data = (void *)quad_10bit},
> > > + {.compatible = "ti,dac7573", .data = (void *)quad_12bit},
> > > {}
> > > };
> > > MODULE_DEVICE_TABLE(of, dac5571_of_id);
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices
2022-03-27 15:59 ` Jonathan Cameron
@ 2022-03-27 16:00 ` Jonathan Cameron
2022-03-27 15:57 ` Laurent Pinchart
0 siblings, 1 reply; 7+ messages in thread
From: Jonathan Cameron @ 2022-03-27 16:00 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-iio, Lars-Peter Clausen, Sean Nyekjaer, Jose Cazarin,
Wolfram Sang, linux-i2c
On Sun, 27 Mar 2022 16:59:01 +0100
Jonathan Cameron <jic23@kernel.org> wrote:
> On Sun, 27 Mar 2022 17:37:27 +0300
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
>
> > Hi Jonathan,
> >
> > On Sun, Mar 27, 2022 at 03:34:38PM +0100, Jonathan Cameron wrote:
> > > On Fri, 25 Mar 2022 01:43:40 +0200 Laurent Pinchart wrote:
> > >
> > > > From: Jose Cazarin <joseespiriki@gmail.com>
> > > >
> > > > When matching an OF device, the match mechanism tries all components of
> > > > the compatible property. This can result with a device matched with a
> > > > compatible string that isn't the first in the compatible list. For
> > > > instance, with a compatible property set to
> > > >
> > > > compatible = "ti,dac081c081", "ti,dac5571";
> > > >
> > > > the driver will match the second compatible string, as the first one
> > > > isn't listed in the of_device_id table. The device will however be named
> > > > "dac081c081" by the I2C core.
> > > >
> > > > This causes an issue when identifying the chip. The probe function
> > > > receives a i2c_device_id that comes from the module's I2C device ID
> > > > table. There is no entry in that table for "dac081c081", which results
> > > > in a NULL pointer passed to the probe function.
> > > >
> > > > To fix this, add chip_id information in the data field of the OF device
> > > > ID table, and retrieve it with of_device_get_match_data() for OF
> > > > devices.
> > >
> > > You forgot to update the patch description for the change to device_get_match_data()
> > >
> > > One other thing inline.
> >
> > Thank you for the comments. I agree with both. Would you like me to send
> > a v3, or is this something you'd be happy fixing when applying the patch
> > to your tree ?
>
> I've applied it how with those changes. However, it's on my fixes-togreg branch
now.
I can't type on Sundays :)
Jonathan
> and I don't plan to push that out until after rc1 when I can rebase it on something
> 'stable'.
>
> Thanks,
>
> Jonathan
>
> >
> > > > Signed-off-by: Jose Cazarin <joseespiriki@gmail.com>
> > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > > Changes since v1:
> > > >
> > > > - Use device_get_match_data()
> > > > ---
> > > > drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++----------
> > > > 1 file changed, 18 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
> > > > index 4a3b8d875518..4f50e31dffb0 100644
> > > > --- a/drivers/iio/dac/ti-dac5571.c
> > > > +++ b/drivers/iio/dac/ti-dac5571.c
> > > > @@ -19,6 +19,7 @@
> > > > #include <linux/i2c.h>
> > > > #include <linux/module.h>
> > > > #include <linux/mod_devicetable.h>
> > > > +#include <linux/property.h>
> > > > #include <linux/regulator/consumer.h>
> > > >
> > > > enum chip_id {
> > > > @@ -311,6 +312,7 @@ static int dac5571_probe(struct i2c_client *client,
> > > > const struct dac5571_spec *spec;
> > > > struct dac5571_data *data;
> > > > struct iio_dev *indio_dev;
> > > > + enum chip_id chip_id;
> > > > int ret, i;
> > > >
> > > > indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > > > @@ -326,7 +328,13 @@ static int dac5571_probe(struct i2c_client *client,
> > > > indio_dev->modes = INDIO_DIRECT_MODE;
> > > > indio_dev->channels = dac5571_channels;
> > > >
> > > > - spec = &dac5571_spec[id->driver_data];
> > > > + if (dev->of_node)
> > >
> > > if (dev_fwnode(dev))
> > >
> > > > + chip_id = (uintptr_t)device_get_match_data(dev);
> > > > + else
> > > > + chip_id = id->driver_data;
> > > > +
> > > > + spec = &dac5571_spec[chip_id];
> > > > +
> > > > indio_dev->num_channels = spec->num_channels;
> > > > data->spec = spec;
> > > >
> > > > @@ -385,15 +393,15 @@ static int dac5571_remove(struct i2c_client *i2c)
> > > > }
> > > >
> > > > static const struct of_device_id dac5571_of_id[] = {
> > > > - {.compatible = "ti,dac5571"},
> > > > - {.compatible = "ti,dac6571"},
> > > > - {.compatible = "ti,dac7571"},
> > > > - {.compatible = "ti,dac5574"},
> > > > - {.compatible = "ti,dac6574"},
> > > > - {.compatible = "ti,dac7574"},
> > > > - {.compatible = "ti,dac5573"},
> > > > - {.compatible = "ti,dac6573"},
> > > > - {.compatible = "ti,dac7573"},
> > > > + {.compatible = "ti,dac5571", .data = (void *)single_8bit},
> > > > + {.compatible = "ti,dac6571", .data = (void *)single_10bit},
> > > > + {.compatible = "ti,dac7571", .data = (void *)single_12bit},
> > > > + {.compatible = "ti,dac5574", .data = (void *)quad_8bit},
> > > > + {.compatible = "ti,dac6574", .data = (void *)quad_10bit},
> > > > + {.compatible = "ti,dac7574", .data = (void *)quad_12bit},
> > > > + {.compatible = "ti,dac5573", .data = (void *)quad_8bit},
> > > > + {.compatible = "ti,dac6573", .data = (void *)quad_10bit},
> > > > + {.compatible = "ti,dac7573", .data = (void *)quad_12bit},
> > > > {}
> > > > };
> > > > MODULE_DEVICE_TABLE(of, dac5571_of_id);
> > >
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices
2022-03-24 23:43 [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart
2022-03-27 14:34 ` Jonathan Cameron
@ 2022-03-27 19:25 ` Andy Shevchenko
1 sibling, 0 replies; 7+ messages in thread
From: Andy Shevchenko @ 2022-03-27 19:25 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Sean Nyekjaer,
Jose Cazarin, Wolfram Sang, linux-i2c
On Fri, Mar 25, 2022 at 6:55 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> From: Jose Cazarin <joseespiriki@gmail.com>
>
> When matching an OF device, the match mechanism tries all components of
> the compatible property. This can result with a device matched with a
> compatible string that isn't the first in the compatible list. For
> instance, with a compatible property set to
>
> compatible = "ti,dac081c081", "ti,dac5571";
>
> the driver will match the second compatible string, as the first one
> isn't listed in the of_device_id table. The device will however be named
> "dac081c081" by the I2C core.
>
> This causes an issue when identifying the chip. The probe function
> receives a i2c_device_id that comes from the module's I2C device ID
> table. There is no entry in that table for "dac081c081", which results
> in a NULL pointer passed to the probe function.
>
> To fix this, add chip_id information in the data field of the OF device
> ID table, and retrieve it with of_device_get_match_data() for OF
> devices.
It's a good fix, but...
...
> + if (dev->of_node)
Why OF nodes?!
> + chip_id = (uintptr_t)device_get_match_data(dev);
> + else
> + chip_id = id->driver_data;
> + spec = &dac5571_spec[chip_id];
What I would rather expect here is
const ... *spec;
spec = device_get_match_data(dev);
if (!spec)
spec = (const ...)id->driver_data;
And don't use enum in the driver data with ugly castings.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-03-27 19:26 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-24 23:43 [PATCH v2] iio: dac: dac5571: Fix chip id detection for OF devices Laurent Pinchart
2022-03-27 14:34 ` Jonathan Cameron
2022-03-27 14:37 ` Laurent Pinchart
2022-03-27 15:59 ` Jonathan Cameron
2022-03-27 16:00 ` Jonathan Cameron
2022-03-27 15:57 ` Laurent Pinchart
2022-03-27 19:25 ` Andy Shevchenko
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.