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