| Message ID | 20210724000654.23168-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | None | expand |
On Sat, 24 Jul 2021 03:06:54 +0300 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. > > 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> Interesting problem that I hadn't previously realised could happen. One request though, can we use device_get_match_data() here rather than the of specific version? Include property.h as well for that. That should allow the same issue with compatible to work correctly when using PRP0001 based ACPI methods. https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/acpi/bus.c#L891 Will result in acpi_of_device_get_match_data() being called which will match to the of_device_id table. Thanks, Jonathan > --- > Changes since v1: > > - Include linux/of_device.h > --- > 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 2a5ba1b08a1d..8ceb1b42b14e 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/of_device.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)of_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; > > @@ -384,15 +392,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);
On Sat, 24 Jul 2021 03:06:54 +0300 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. > > 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> Side note I failed to put in the review email. I definitely prefer a whole new series even when a change is just to a single patch like this. Much easier to track and eventually pick up as one unit. Thanks, Jonathan > --- > Changes since v1: > > - Include linux/of_device.h > --- > 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 2a5ba1b08a1d..8ceb1b42b14e 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/of_device.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)of_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; > > @@ -384,15 +392,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);
Hi Jonathan, On Sat, Jul 24, 2021 at 03:43:08PM +0100, Jonathan Cameron wrote: > On Sat, 24 Jul 2021 03:06:54 +0300 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. > > > > 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> > > Interesting problem that I hadn't previously realised could happen. > > One request though, can we use device_get_match_data() here rather than > the of specific version? Include property.h as well for that. > > That should allow the same issue with compatible to work correctly when > using PRP0001 based ACPI methods. > https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/acpi/bus.c#L891 > Will result in acpi_of_device_get_match_data() being called which will > match to the of_device_id table. Good point. I wasn't aware of PRP0001. I'll submit a v2 with this fixed, after giving a bit of time for additional review, if any (I'm in particular interested in whether this issue should be fixed in individual drivers or in the I2C core, as explained in the cover letter)). > > --- > > Changes since v1: > > > > - Include linux/of_device.h > > --- > > 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 2a5ba1b08a1d..8ceb1b42b14e 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/of_device.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)of_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; > > > > @@ -384,15 +392,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); >
> > 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> > > Interesting problem that I hadn't previously realised could happen. > > One request though, can we use device_get_match_data() here rather than > the of specific version? Include property.h as well for that. > > That should allow the same issue with compatible to work correctly when > using PRP0001 based ACPI methods. > https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/acpi/bus.c#L891 > Will result in acpi_of_device_get_match_data() being called which will > match to the of_device_id table. Couldn't you use the "new" probe_new() callback instead which will drop the i2c_device_id? Kieran was interested in such conversions IIRC.
Hi Wolfram, On Tue, Aug 17, 2021 at 10:44:20PM +0200, Wolfram Sang wrote: > > > > 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> > > > > Interesting problem that I hadn't previously realised could happen. > > > > One request though, can we use device_get_match_data() here rather than > > the of specific version? Include property.h as well for that. > > > > That should allow the same issue with compatible to work correctly when > > using PRP0001 based ACPI methods. > > https://elixir.bootlin.com/linux/v5.14-rc1/source/drivers/acpi/bus.c#L891 > > Will result in acpi_of_device_get_match_data() being called which will > > match to the of_device_id table. > > Couldn't you use the "new" probe_new() callback instead which will drop > the i2c_device_id? Kieran was interested in such conversions IIRC. It's a bit unrelated to this patch, but I can add another patch to the series. While I have your attention, there's a question for you in the cover letter :-) Could you please have a look ?
> > Couldn't you use the "new" probe_new() callback instead which will drop > > the i2c_device_id? Kieran was interested in such conversions IIRC. > > It's a bit unrelated to this patch, but I can add another patch to the > series. > > While I have your attention, there's a question for you in the cover > letter :-) Could you please have a look ? ? This was the answer to that question. Unless I misunderstood.
On Tue, Aug 17, 2021 at 10:58:19PM +0200, Wolfram Sang wrote: > > > > Couldn't you use the "new" probe_new() callback instead which will drop > > > the i2c_device_id? Kieran was interested in such conversions IIRC. > > > > It's a bit unrelated to this patch, but I can add another patch to the > > series. > > > > While I have your attention, there's a question for you in the cover > > letter :-) Could you please have a look ? > > ? This was the answer to that question. Unless I misunderstood. My point is that this patch shouldn't be needed. I'd like if the I2C core could get the driver data from the i2c_device_id table instead of duplicating it in the of_device_id. This isn't possible today as i2c_match_id() doesn't have the fallback mechanism that OF matching has.
Hi Wolfram, On Wed, Aug 18, 2021 at 12:20:03AM +0300, Laurent Pinchart wrote: > On Tue, Aug 17, 2021 at 10:58:19PM +0200, Wolfram Sang wrote: > > > > > > Couldn't you use the "new" probe_new() callback instead which will drop > > > > the i2c_device_id? Kieran was interested in such conversions IIRC. > > > > > > It's a bit unrelated to this patch, but I can add another patch to the > > > series. > > > > > > While I have your attention, there's a question for you in the cover > > > letter :-) Could you please have a look ? > > > > ? This was the answer to that question. Unless I misunderstood. > > My point is that this patch shouldn't be needed. I'd like if the I2C > core could get the driver data from the i2c_device_id table instead of > duplicating it in the of_device_id. This isn't possible today as > i2c_match_id() doesn't have the fallback mechanism that OF matching has. Any thought on this ?
> My point is that this patch shouldn't be needed. I'd like if the I2C > core could get the driver data from the i2c_device_id table instead of > duplicating it in the of_device_id. This isn't possible today as > i2c_match_id() doesn't have the fallback mechanism that OF matching has. I think the proper fix would be naming the I2C client after the actually matched compatible property, and not after the first one? I am a bit afraid of regressions when we change that, however...
On Mon, Mar 28, 2022 at 11:20:54AM +0200, Wolfram Sang wrote: > > > My point is that this patch shouldn't be needed. I'd like if the I2C > > core could get the driver data from the i2c_device_id table instead of > > duplicating it in the of_device_id. This isn't possible today as > > i2c_match_id() doesn't have the fallback mechanism that OF matching has. > > I think the proper fix would be naming the I2C client after the actually > matched compatible property, and not after the first one? I am a bit > afraid of regressions when we change that, however... That would be the right way indeed. I have the same concern regarding regressions though. Is it worth a try to see what could break ?
> > I think the proper fix would be naming the I2C client after the actually > > matched compatible property, and not after the first one? I am a bit > > afraid of regressions when we change that, however... > > That would be the right way indeed. I have the same concern regarding > regressions though. Is it worth a try to see what could break ? Sure! Only problem: Patches welcome(tm) or I put it on my to-do-list(tm) ;)
Hi Wolfram, On Mon, Mar 28, 2022 at 02:22:27PM +0200, Wolfram Sang wrote: > > > > I think the proper fix would be naming the I2C client after the actually > > > matched compatible property, and not after the first one? I am a bit > > > afraid of regressions when we change that, however... > > > > That would be the right way indeed. I have the same concern regarding > > regressions though. Is it worth a try to see what could break ? > > Sure! Only problem: Patches welcome(tm) or I put it on my to-do-list(tm) > ;) I've had a look, but it seems to be problematic. The name of the client is set in i2c_new_client_device(), way before we match with a driver. The name is used in the uevent sent to userspace, so changing it afterwards is likely not a good idea.
> I've had a look, but it seems to be problematic. The name of the client > is set in i2c_new_client_device(), way before we match with a driver. > The name is used in the uevent sent to userspace, so changing it > afterwards is likely not a good idea. Okay, that is a definitive no-go. Thanks for checking. Seems we really need to update i2c_match_id to handle the extra case as you suggested.
diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c index 2a5ba1b08a1d..8ceb1b42b14e 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/of_device.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)of_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; @@ -384,15 +392,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);