Re: [PATCH v2 2/2] iio: dac: ad5766: add driver support for AD5766
From: Andy Shevchenko
Date: Wed Dec 09 2020 - 10:53:09 EST
On Fri, Dec 4, 2020 at 8:17 PM Cristian Pop <cristian.pop@xxxxxxxxxx> 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@xxxxxxxxxx>
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