Re: [PATCH v2 2/2] iio: dac: mcp4725: Use i2c_get_match_data()

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 18 Aug 2023 18:49:05 +0100
Biju Das <biju.das.jz@bp.renesas.com> wrote:

> Replace local variable chip_id with struct iio_chan_spec for device data
> and use its .ext_info for chip identification. After this replace
> device_get_match_data() and id lookup for retrieving match data
> by i2c_get_match_data() by converting enum->pointer for data in the
> match table.
> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>

The use of ext_info on a channel is messy.  Please just define
a clearly named flag in chip_info to make it simpler to maintain
and extend.

Jonathan

> ---
> v1->v2:
>  * No change.
> ---
>  drivers/iio/dac/mcp4725.c | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
> index 33a61f65bc25..f9ce01c4cc53 100644
> --- a/drivers/iio/dac/mcp4725.c
> +++ b/drivers/iio/dac/mcp4725.c
> @@ -386,7 +386,7 @@ static int mcp4725_probe(struct i2c_client *client)
>  	struct mcp4725_data *data;
>  	struct iio_dev *indio_dev;
>  	struct mcp4725_platform_data *pdata, pdata_dt;
> -	int chip_id;
> +	const struct iio_chan_spec *ch;
>  	u8 inbuf[4];
>  	u8 pd;
>  	u8 ref;
> @@ -398,10 +398,8 @@ static int mcp4725_probe(struct i2c_client *client)
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
> -	if (dev_fwnode(&client->dev))
> -		chip_id = (uintptr_t)device_get_match_data(&client->dev);
> -	else
> -		chip_id = id->driver_data;
> +	ch = i2c_get_match_data(client);
> +
>  	pdata = dev_get_platdata(&client->dev);
>  
>  	if (!pdata) {
> @@ -414,7 +412,7 @@ static int mcp4725_probe(struct i2c_client *client)
>  		pdata = &pdata_dt;
>  	}
>  
> -	if (chip_id == MCP4725 && pdata->use_vref) {
> +	if (ch->ext_info == mcp4725_ext_info && pdata->use_vref) {

I think this will end up being hard to maintain.

Please define a mcp4725_chip_info structure and put the channels in there
+ a flag that says if the device in question supports an external reference.

>  		dev_err(&client->dev,
>  			"external reference is unavailable on MCP4725");
>  		return -EINVAL;
> @@ -455,12 +453,12 @@ static int mcp4725_probe(struct i2c_client *client)
>  
>  	indio_dev->name = id->name;
>  	indio_dev->info = &mcp4725_info;
> -	indio_dev->channels = &mcp472x_channel[chip_id];
> +	indio_dev->channels = ch;
>  	indio_dev->num_channels = 1;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
>  	/* read current DAC value and settings */
> -	err = i2c_master_recv(client, inbuf, chip_id == MCP4725 ? 3 : 4);
> +	err = i2c_master_recv(client, inbuf, ch->ext_info == mcp4725_ext_info ? 3 : 4);
>  
>  	if (err < 0) {
>  		dev_err(&client->dev, "failed to read DAC value");
> @@ -470,10 +468,10 @@ static int mcp4725_probe(struct i2c_client *client)
>  	data->powerdown = pd > 0;
>  	data->powerdown_mode = pd ? pd - 1 : 2; /* largest resistor to gnd */
>  	data->dac_value = (inbuf[1] << 4) | (inbuf[2] >> 4);
> -	if (chip_id == MCP4726)
> +	if (ch->ext_info == mcp4726_ext_info)
>  		ref = (inbuf[3] >> 3) & 0x3;

Add a flag to the chip_info structure for whatever we are controlling here
and then have
if (info->x_is_supported)
	...


>  
> -	if (chip_id == MCP4726 && ref != data->ref_mode) {
> +	if (ch->ext_info == mcp4726_ext_info && ref != data->ref_mode) {

I think this can also use the vref bool I suggest adding above.

>  		dev_info(&client->dev,
>  			"voltage reference mode differs (conf: %u, eeprom: %u), setting %u",
>  			data->ref_mode, ref, data->ref_mode);
> @@ -511,8 +509,8 @@ static void mcp4725_remove(struct i2c_client *client)
>  }
>  
>  static const struct i2c_device_id mcp4725_id[] = {
> -	{ "mcp4725", MCP4725 },
> -	{ "mcp4726", MCP4726 },
> +	{ "mcp4725", (kernel_ulong_t)&mcp472x_channel[MCP4725] },
> +	{ "mcp4726", (kernel_ulong_t)&mcp472x_channel[MCP4726] },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, mcp4725_id);
> @@ -520,11 +518,11 @@ MODULE_DEVICE_TABLE(i2c, mcp4725_id);
>  static const struct of_device_id mcp4725_of_match[] = {
>  	{
>  		.compatible = "microchip,mcp4725",
> -		.data = (void *)MCP4725
> +		.data = &mcp472x_channel[MCP4725]
>  	},
>  	{
>  		.compatible = "microchip,mcp4726",
> -		.data = (void *)MCP4726
> +		.data = &mcp472x_channel[MCP4726]
>  	},
>  	{ }
>  };