[PATCH v2 0/2] Use i2c_get_match_data() for mcp4725 DAC

[PATCH v2 0/2] Use i2c_get_match_data() for mcp4725 DAC

[Biju Das]

Use i2c_get_match_data() to get match data for I2C, ACPI and DT-based
matching instead of device_get_match_data() the one for ACPI/DT-based
matching.

This patch series is only compile tested.

v1->v2:
 * Updated commit description for patch#1 with reason for change as
   mentioned by Marek.

Biju Das (2):
  iio: dac: mcp4725: Replace variable 'id' from struct mcp4725_data
  iio: dac: mcp4725: Use i2c_get_match_data()

 drivers/iio/dac/mcp4725.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH v2 1/2] iio: dac: mcp4725: Replace variable 'id' from struct mcp4725_data

[Biju Das]

Replace the variable 'id' from struct mcp4725_data with local variable
chip_id in probe() as the id variable is not used elsewhere in the
driver.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v1->v2:
 * Update commit description with reason for change.
---
 drivers/iio/dac/mcp4725.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/dac/mcp4725.c b/drivers/iio/dac/mcp4725.c
index f4a3124d29f2..33a61f65bc25 100644
--- a/drivers/iio/dac/mcp4725.c
+++ b/drivers/iio/dac/mcp4725.c
@@ -32,7 +32,6 @@
 
 struct mcp4725_data {
 	struct i2c_client *client;
-	int id;
 	unsigned ref_mode;
 	bool vref_buffered;
 	u16 dac_value;
@@ -387,6 +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;
 	u8 inbuf[4];
 	u8 pd;
 	u8 ref;
@@ -399,9 +399,9 @@ static int mcp4725_probe(struct i2c_client *client)
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
 	if (dev_fwnode(&client->dev))
-		data->id = (uintptr_t)device_get_match_data(&client->dev);
+		chip_id = (uintptr_t)device_get_match_data(&client->dev);
 	else
-		data->id = id->driver_data;
+		chip_id = id->driver_data;
 	pdata = dev_get_platdata(&client->dev);
 
 	if (!pdata) {
@@ -414,7 +414,7 @@ static int mcp4725_probe(struct i2c_client *client)
 		pdata = &pdata_dt;
 	}
 
-	if (data->id == MCP4725 && pdata->use_vref) {
+	if (chip_id == MCP4725 && pdata->use_vref) {
 		dev_err(&client->dev,
 			"external reference is unavailable on MCP4725");
 		return -EINVAL;
@@ -455,12 +455,12 @@ static int mcp4725_probe(struct i2c_client *client)
 
 	indio_dev->name = id->name;
 	indio_dev->info = &mcp4725_info;
-	indio_dev->channels = &mcp472x_channel[id->driver_data];
+	indio_dev->channels = &mcp472x_channel[chip_id];
 	indio_dev->num_channels = 1;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
 	/* read current DAC value and settings */
-	err = i2c_master_recv(client, inbuf, data->id == MCP4725 ? 3 : 4);
+	err = i2c_master_recv(client, inbuf, chip_id == MCP4725 ? 3 : 4);
 
 	if (err < 0) {
 		dev_err(&client->dev, "failed to read DAC value");
@@ -470,10 +470,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 (data->id == MCP4726)
+	if (chip_id == MCP4726)
 		ref = (inbuf[3] >> 3) & 0x3;
 
-	if (data->id == MCP4726 && ref != data->ref_mode) {
+	if (chip_id == MCP4726 && ref != data->ref_mode) {
 		dev_info(&client->dev,
 			"voltage reference mode differs (conf: %u, eeprom: %u), setting %u",
 			data->ref_mode, ref, data->ref_mode);
-- 
2.25.1

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

[Biju Das]

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>
---
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) {
 		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;
 
-	if (chip_id == MCP4726 && ref != data->ref_mode) {
+	if (ch->ext_info == mcp4726_ext_info && ref != data->ref_mode) {
 		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]
 	},
 	{ }
 };
-- 
2.25.1

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

[Jonathan Cameron]

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]
>  	},
>  	{ }
>  };

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

[Biju Das]

Hi Jonathan Cameron,

> Subject: Re: [PATCH v2 2/2] iio: dac: mcp4725: Use i2c_get_match_data()
> 
> 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.
OK

> 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.

Agreed.

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

OK.

> 	...
> 
> 
> >
> > -	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.

OK.

Cheers,
Biju
> 
> >  		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]
> >  	},
> >  	{ }
> >  };