On Tue, Aug 10, 2021 at 03:49:52PM +0300, Andy Shevchenko wrote:

On Mon, Aug 9, 2021 at 10:59 AM Mugilraj Dhavachelvan
[off-list ref] wrote:

quoted

The AD5110/AD5112/AD5114 provide a nonvolatile solution
for 128-/64-/32-position adjustment applications, offering
guaranteed low resistor tolerance errors of ±8% and up to
±6 mA current density.

...

quoted

+/*
+ * Analog Devices AD5110 digital potentiometer driver
+ *
+ * Copyright (C) 2021 Mugilraj Dhavachelvan [off-list ref]
+ *
+ * Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5110_5112_5114.pdf

quoted

+ *

Redundant blank line.

fixed in v3

quoted

+ */

...

quoted

+#define WIPER_RESISTANCE       70

Missed prefix?

fixed in v3

...

quoted

+static const struct iio_chan_spec ad5110_channels[] = {
+       {
+               .type = IIO_RESISTANCE,
+               .output = 1,
+               .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_OFFSET) |
+                                       BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_ENABLE),
+       }

+ comma.

fixed in v3

quoted

+};

...

quoted

+       ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
+       if (ret != sizeof(data->buf)) {

quoted

+               ret = -EIO;

Shadowed error code when ret < 0.

fixed in v3

quoted

+               goto error;
+       }

...

quoted

+       ret = i2c_master_send_dmasafe(data->client, data->buf, sizeof(data->buf));
+       if (ret != sizeof(data->buf))
+               ret = -EIO;

Ditto.

fixed in v3

quoted

+       mutex_unlock(&data->lock);
+
+       return ret < 0 ? ret : 0;
+}

...

quoted

+       data->tol = data->cfg->kohms * (val & GENMASK(6, 0)) * 10 / 8;
+       if (!(val & BIT(7)))
+               data->tol *= -1;

Shouldn't you simple use corresponding sign_extend*()?

I'm not able see any sign_extend for 16 bit. Is there any other way?

...

quoted

+       ret = ad5110_write(data, AD5110_EEPROM_WR, 0);
+       if (ret) {
+               dev_err(&data->client->dev, "RDAC to EEPROM write failed\n");
+               return ret;
+       }

quoted

+       msleep(20);

Each long sleeps like this must be explained.

fixed in v3

...

quoted

+static IIO_DEVICE_ATTR(store_eeprom, 0644,
+                      ad5110_eeprom_read,
+                      ad5110_eeprom_write, 0);

IIO_DEVICE_ATTR_RW() ?

fixed in v3

...

quoted

+static struct attribute *ad5110_attributes[] = {
+       &iio_dev_attr_store_eeprom.dev_attr.attr,
+       NULL,

No comma for a terminator line.

fixed in v3

quoted

+};

...

quoted

+       data->cfg = device_get_match_data(dev);

quoted

+       if (!data->cfg)
+               data->cfg = &ad5110_cfg[i2c_match_id(ad5110_id, client)->driver_data];

Not sure this is not a dead code since you are using ->probe_new().

Even I'm suspecting that and also removing id_table. But I'm not sure of
it so just left as it is.

...

quoted

+static struct i2c_driver ad5110_driver = {
+       .driver = {
+               .name   = "ad5110",
+               .of_match_table = ad5110_of_match,
+       },
+       .probe_new      = ad5110_probe,
+       .id_table       = ad5110_id,
+};

quoted

+

Redundant blank line

fixed in v3.

quoted

+module_i2c_driver(ad5110_driver);

Thanks for the feedback :)

-- 
With Best Regards,
Andy Shevchenko