Re: [PATCH v2 2/2] iio: dac: ad5766: add driver support for AD5766

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Fri, Dec 4, 2020 at 8:17 PM Cristian Pop <cristian.pop@analog.com> wrote:
>
> The AD5766/AD5767 are 16-channel, 16-bit/12-bit, voltage output dense DACs
> Digital-to-Analog converters.
>
> This change adds support for these DACs.

> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5766-5767.pdf

Can we use Datasheet: tag instead, please?

>
> Signed-off-by: Cristian Pop <cristian.pop@analog.com>

No blank lines in tag block, please.

...

> + * Analog Devices AD5766, AD5767
> + * Digital to Analog Converters driver

Looks like one line.

...

> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/bitfield.h>

Keep it sorted?

...

> +#define AD5766_CMD_WR_IN_REG(x)                        (0x10 | ((x) & 0xF))
> +#define AD5766_CMD_WR_DAC_REG(x)               (0x20 | ((x) & 0xF))

Why not GENMASK()?

...

> +#define AD5766_CMD_READBACK_REG(x)             (0x80 | ((x) & 0xF))

Ditto.

...

> +enum ad5766_type {
> +       ID_AD5766,
> +       ID_AD5767,
> +};

This may be problematic in case we switch to device_get_match_data().

...

> +enum ad5766_voltage_range {
> +       AD5766_VOLTAGE_RANGE_M20V_0V,
> +       AD5766_VOLTAGE_RANGE_M16V_to_0V,
> +       AD5766_VOLTAGE_RANGE_M10V_to_0V,
> +       AD5766_VOLTAGE_RANGE_M12V_to_14V,
> +       AD5766_VOLTAGE_RANGE_M16V_to_10V,
> +       AD5766_VOLTAGE_RANGE_M10V_to_6V,
> +       AD5766_VOLTAGE_RANGE_M5V_to_5V,
> +       AD5766_VOLTAGE_RANGE_M10V_to_10V,

> +       AD5766_VOLTAGE_RANGE_MAX,

No comma for terminator line.

> +};

...

> +enum {
> +       AD5766_DITHER_PWR,
> +       AD5766_DITHER_INVERT

+ comma

> +};

...

> +/*
> + * External dither signal can be composed with the DAC output, if activated.
> + * The dither signals are applied to the N0 and N1 input pins.
> + * Dither source for each of the channel can be selected by writing to
> + * "dither_source",a 32 bit variable and two bits are used for each of the 16
> + * channels: 0: NO_DITHER, 1: N0, 2: N1.
> + * This variable holds available dither source strings.
> + */
> +static const char * const ad5766_dither_sources[] = {
> +       "NO_DITHER",
> +       "N0",
> +       "N1",
> +};

Can't we rather be simpler, i.e. use 0,1 and -1, where the latter for
NO_DITHER cas?.

...

> +/*
> + * Dither signal can also be scaled.
> + * Available dither scale strings coresponding to "dither_scale" field in

corresponding

> + * "struct ad5766_state".
> + * "dither_scale" is a 32 bit variable and two bits are used for each of the 16
> + * channels: 0: NO_SCALING, 1: 75%_SCALING, 2: 50%_SCALING, 3: 25%_SCALING.
> + */
> +static const char * const ad5766_dither_scales[] = {
> +       "NO_SCALING",
> +       "75%_SCALING",
> +       "50%_SCALING",
> +       "25%_SCALING",
> +};

Oh, no. Please, use plain numbers in percentages.

...

> + * @cached_offset:     Cached range coresponding to the selected offset

corresponding
Please, run checkpatch.pl --code-spell (or how is it called?).

...

> + * @offset_avail:      Offest available table

Ditto.
Offset (I suppose)

...

> +static int _ad5766_spi_write(struct ad5766_state *st, u8 command, u16 data)
> +{
> +       st->data[0].b8[0] = command;
> +       st->data[0].b8[1] = (data & 0xFF00) >> 8;
> +       st->data[0].b8[2] = (data & 0x00FF) >> 0;

Please,  use get_unaliged_XX() / put_unaligned_XX() and other related
macros / APIs.

> +       return spi_write(st->spi, &st->data[0].b8[0], 3);
> +}

...

> +static int ad5766_reset(struct ad5766_state *st)
> +{
> +       int ret = 0;

In general it's a bad idea and in particular here it's not needed.

> +       return 0;
> +}

...

> +       ret = _ad5766_spi_write(st, AD5766_CMD_DITHER_SIG_1,

> +                         st->dither_source & 0xFFFF);

Do you really need this conjunction? If so, why not GENMASK()?

> +       if (ret)
> +               return ret;

...

> +       ret = _ad5766_spi_write(st, AD5766_CMD_INV_DITHER, st->dither_invert);
> +       if (ret)
> +               return ret;
> +
> +       return 0;

return _ad5766_spi_write(…);

...

> +static int ad5766_set_offset(struct ad5766_state *st, int val, int val2)
> +{
> +       int i;
> +       s32 (*tbl)[AD5766_VOLTAGE_RANGE_MAX][2] = &(st->offset_avail);

Too many parentheses.

> +
> +       for (i = 0; i < AD5766_VOLTAGE_RANGE_MAX; i++) {

ARRAY_SIZE() ?

> +               if ((*tbl)[i][0] == val && (*tbl)[i][1] == val2) {
> +                       st->cached_offset = i;
> +                       return 0;
> +               }
> +       }

Entire function hurts my eyes. Can you give some time and think over it again?

> +       return -EINVAL;
> +}

...

> +static int ad5766_set_scale(struct ad5766_state *st, int val, int val2)

Ditto.

...

> +               *vals = (const int *)st->scale_avail;

Why do you need casting?

...

> +               *vals = (const int *)st->offset_avail;

Ditto.

...

> +static int ad5766_read_raw(struct iio_dev *indio_dev,
> +                          struct iio_chan_spec const *chan,
> +                          int *val,
> +                          int *val2,
> +                          long m)

It may take much less LOCs.

...

> +static int ad5766_write_raw(struct iio_dev *indio_dev,
> +                           struct iio_chan_spec const *chan,
> +                           int val,
> +                           int val2,
> +                           long info)

Ditto.

...

> +               const int max_val = (1 << chan->scan_type.realbits);
> +
> +               if (val >= max_val || val < 0)
> +                       return -EINVAL;
> +               val <<= chan->scan_type.shift;

You can do better, i.e. drop unneeded temporary variable and use fls().

...

> +       return (source >> chan->channel * 2);

Seems parentheses in incorrect place in case you want to increase robustness.

> +}

...

> +       st->dither_source |= (mode << (chan->channel * 2));

It's not LISP, seriously.
I'm wondering if Analog has internal mailing list (and perhaps a wiki
with common tips and tricks for Linux kernel programming) for
review...

...

> +       return (scale >> chan->channel * 2);

As above.

...

> +       st->dither_scale |= (scale << (chan->channel * 2));

As above.

...

> +               return sprintf(buf, "%u\n", 0x01 &
> +                              ~(st->dither_power_ctrl >> chan->channel));

Oh là là.

!(st->dither_power_ctrl & BIT(chan->channel)) ?

...

> +               return sprintf(buf, "%u\n", 0x01 &
> +                              (st->dither_invert >> chan->channel));

Ditto.

...

> +       default:

> +               ret = -EINVAL;
> +               break;

Point of this? Can't return directly?

> +       }
> +
> +       return ret;

...

> +       switch ((u32)private) {

Why casting?

...

> +       default:
> +               ret = -EINVAL;
> +               break;

Why not to return here?

> +       }

> +       return ret ? ret : len;

return len; ?

...

> +               offset = div_s64(offset * 1000000, denom);
> +               st->offset_avail[i][0] = div_s64(offset, 1000000);
> +               div_s64_rem(offset, 1000000, &st->offset_avail[i][1]);

...

> +               scale = div_u64((scale * 1000000000), (1 << realbits));
> +               st->scale_avail[i][0] = (int)div_u64(scale, 1000000);
> +               div_s64_rem(scale, 1000000, &st->scale_avail[i][1]);

Perhaps it's time to extend units.h with mV / V / uV / etc?

...

> +static const struct of_device_id ad5766_dt_match[] = {
> +       { .compatible = "adi,ad5766" },
> +       { .compatible = "adi,ad5767" },

> +       {},

No comma for terninator.

> +};


-- 
With Best Regards,
Andy Shevchenko