Re: [PATCH v2 1/2] iio: dac: Add AD5758 support

[Andy Shevchenko]

On Thu, Jun 7, 2018 at 4:09 PM, Stefan Popa <stefan.popa@xxxxxxxxxx> wrote:
> The AD5758 is a single channel DAC with 16-bit precision which uses the SPI
> interface that operates at clock rates up to 50MHz.
>
> The output can be configured as voltage or current and is available on a
> single terminal.
>
> Datasheet:
> http://www.analog.com/media/en/technical-documentation/data-sheets/ad5758.pdf

> +#include <linux/fs.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/sysfs.h>
> +#include <linux/spi/spi.h>
> +#include <linux/delay.h>

Can you keep the list in order?

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/property.h>

Same, and property.h more generic if you wish than iio.

> +/* AD5758_DIGITAL_DIAG_RESULTS */
> +#define AD5758_DIG_DIAG_RES_CAL_MEM_UNREFRESHED_MSK    BIT(15)

This name way too long.
Also, check the rest near to this length and try to shorten them.

> +#define AD5758_REG_WRITE(x)    (0x80 | ((x) & 0x1F))

Name of macro is misleading. It rather some value preparations than "write".

> +/* x^8 + x^2 + x^1 + x^0 */
> +#define AD5758_CRC8_POLY       0x07

Where is it used?

> +enum ad5758_output_range {
> +       AD5758_RANGE_0V_5V,
> +       AD5758_RANGE_0V_10V,
> +       AD5758_RANGE_PLUSMINUS_5V,
> +       AD5758_RANGE_PLUSMINUS_10V,
> +       AD5758_RANGE_0mA_20mA = 8,

Hmm... why it starts from 8? Perhaps some explanation on top of enum?

> +       AD5758_RANGE_0mA_24mA,
> +       AD5758_RANGE_4mA_24mA,
> +       AD5758_RANGE_PLUSMINUS_20mA,
> +       AD5758_RANGE_PLUSMINUS_24mA,
> +       AD5758_RANGE_MINUS_1mA_PLUS_22mA,
> +};

> +static int ad5758_get_array_index(const int *array, unsigned int size, int val)
> +{
> +       int i;
> +
> +       for (i = 0; i < size; i++) {
> +               if (val == array[i])
> +                       return i;
> +       }
> +
> +       return -EINVAL;
> +}

bsearch? (lib/bsearch.c)

> +static int ad5758_wait_for_task_complete(struct ad5758_state *st,
> +                                        unsigned int reg,
> +                                        unsigned int mask)
> +{
> +       unsigned int timeout;
> +       int ret;
> +
> +       timeout = 4;
> +       do {
> +               ret = ad5758_spi_reg_read(st, reg);
> +               if (ret < 0)
> +                       return ret;
> +
> +               if (!(ret & mask))
> +                       return 0;
> +

> +               mdelay(1);

No way. CPU would be busy for 1ms at least!

> +       } while (--timeout);
> +
> +       dev_err(&st->spi->dev,
> +               "Error reading bit 0x%x in 0x%x register\n", mask, reg);
> +
> +       return -EIO;
> +}

> +static int ad5758_soft_reset(struct ad5758_state *st)
> +{
> +       int ret;
> +
> +       ret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_1);
> +       if (ret < 0)
> +               return ret;
> +
> +       ret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_2);
> +

> +       mdelay(1);

Explanation must be given for such a long delay without idling.

> +
> +       return ret;
> +}

> +       if (readval == NULL) {

Why not positive check?

if (readval) {
 ...
} else {
 ...
}

> +               ret =  ad5758_spi_reg_write(st, reg, writeval);
> +       } else {
> +               ret = ad5758_spi_reg_read(st, reg);
> +               if (ret < 0) {
> +                       mutex_unlock(&st->lock);
> +                       return ret;
> +               }
> +
> +               *readval = ret;
> +               ret = 0;
> +       }

> +       ret = strtobool(buf, &pwr_down);
> +       if (ret)
> +               return ret;

kstrtobool(), please.

> +static const struct iio_chan_spec_ext_info ad5758_ext_info[] = {
> +       {
> +               .name = "powerdown",
> +               .read = ad5758_read_powerdown,
> +               .write = ad5758_write_powerdown,
> +               .shared = IIO_SEPARATE,
> +       },
> +       { },

Terminator is better without comma.

> +};

> +       st->data[0].d32 =

> +               cpu_to_be32(
> +               (AD5758_REG_WRITE(AD5758_DIGITAL_DIAG_CONFIG) << 24) | 0x5C3A);

Either do this one line, or introduce a temporary variable for
parameter of cpu_to_be32().

> +       if (!device_property_read_u32(&st->spi->dev, "adi,range", &tmp)) {

Would be better to have
ret = device_property_...();
if (ret) {
 ...
} else {
 ...
}

if it's optional and

if (ret) {
 ...
 return -ERRNO; // goto cleanup; etc
}

> +               for (i = 0; i < ARRAY_SIZE(ad5758_min_max_table); i++) {
> +                       if (tmp == ad5758_min_max_table[i].reg) {
> +                               st->out_range = i;
> +                               break;
> +                       }
> +               }
> +
> +               if (i == ARRAY_SIZE(ad5758_min_max_table))
> +                       dev_warn(&st->spi->dev,
> +                                "range invalid, using 0V to 5V\n");
> +       } else {
> +               dev_dbg(&st->spi->dev,
> +                       "Missing \"range\" property, using 0V to 5V\n");
> +       }

> +       if (!device_property_read_u32_array(&st->spi->dev, "adi,slew",
> +                                           tmparray, 3)) {

...

> +       } else {
> +               dev_dbg(&st->spi->dev,
> +                        "Missing \"slew\" property, using 16kHz and 4 LSB\n");
> +       }

Ditto.

> +       ad5758_parse_dt(st);

No errors? Nothing fatal?

-- 
With Best Regards,
Andy Shevchenko