[v1.1,2/2] iio: dac: dac5571: Fix chip id detection for OF devices

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:

- Include linux/of_device.h
---
 drivers/iio/dac/ti-dac5571.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

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);