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