Re: [PATCH 2/2] drivers: iio: dac: Add AD5754 DAC driver

Andy Shevchenko
2022-10-04T08:51:07Z

On Tue, Oct 04, 2022 at 10:18:25AM +0300, Ciprian Regus wrote:
> The AD5724/AD5734/AD5754 are quad, 12-/14-/16-bit, serial
> input, voltage output DACs. The devices operate from single-
> supply voltages from +4.5 V up to +16.5 V or dual-supply
> voltages from ±4.5 V up to ±16.5 V. The input coding is
> user-selectable twos complement or offset binary for a bipolar
> output (depending on the state of Pin BIN/2sComp), and straight
> binary for a unipolar output.

...

> +#define AD5754_INT_VREF 2500

Units? (Like _mV or _uV or what? Note, small u is OK to have in such cases)

...

> +#define AD5754_CLEAR_FUNC BIT(2)
> +#define AD5754_LOAD_FUNC (BIT(2) | BIT(0))
> +#define AD5754_NOOP_FUNC GENMASK(4, 3)

Seems like abuse of BIT and GENMASK, use plain numbers as it's probably is.
Otherwise _each_ bit should have it's own descriptive meaning.

...

> +#define AD5754_DAC_REG 0
> +#define AD5754_RANGE_REG BIT(0)
> +#define AD5754_PWR_REG BIT(1)

...

> +#define AD5754_CTRL_REG GENMASK(1, 0)

Why _REG uses GENMASK()?

...

> +struct ad5754_span_tbl {
> + int min;
> + int max;
> +};

I'm wondering if linear_range.h can anyhow help with this code.

...

> +struct ad5754_state {
> + struct regmap *regmap;
> + struct spi_device *spi;
> + struct device *dev;

You always can derive dev from regmap, is this one different?

> +
> + const struct ad5754_chip_info *chip_info;
> +
> + u32 range_idx[AD5754_MAX_CHANNELS];
> + int offset[AD5754_MAX_CHANNELS];
> + u32 dac_max_code;
> + u32 data_mask;
> + u32 sub_lsb;
> + u32 vref;
> +
> + /*
> + * DMA (thus cache coherency maintenance) may require the
> + * transfer buffers to live in their own cache lines.
> + */
> + u8 buff[AD5754_FRAME_SIZE] __aligned(IIO_DMA_MINALIGN);
> +};

...

> +static const struct regmap_config ad5754_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .reg_write = ad5754_reg_write,
> + .reg_read = ad5754_reg_read,

No max register address?

> +};

...

> + struct fwnode_handle *channel_node = NULL;

Redundant assignment.

...

> + fwnode_for_each_available_child_node(dev_fwnode(st->dev), channel_node) {

Why not device_for_each_child_node() ?

(Yes, it uses available ones)

> + }

...

> + range = &ad5754_range[st->range_idx[chan->channel]];
> + gain = (range->max - range->min) / 2500;
> + *val = st->vref * gain / 1000;
> + *val2 = st->chip_info->resolution;

Yeah, looks familiar to the linear_range APIs.

...

> +static int ad5754_probe(struct spi_device *spi)
> +{
> + struct regulator *vref_reg;
> + struct iio_dev *indio_dev;
> + struct ad5754_state *st;
> + struct device *dev;
> + int ret;

> + dev = &spi->dev;

Can be done in the definition block (inline).

...

> + st->chip_info = device_get_match_data(dev);
> + if (!st->chip_info)
> + st->chip_info =
> + (const struct ad5754_chip_info *)spi_get_device_id(spi)->driver_data;

This can look better with a temporary variable. But doesn't matter since we
would like to have these lines to be packed in a new SPI API helper in the
future.

...

> + st->vref = ret / 1000;

Do we have uV_PER_mV or so?

...

> +static const struct spi_device_id ad5754_id[] = {

> + {},

No comma for the terminator line.

> +};

...

> +static const struct of_device_id ad5754_dt_id[] = {

> + {},

Ditto.

> +};

...

> +module_driver(ad5754_driver,
> + ad5754_register_driver,
> + ad5754_unregister_driver);

Why not module_spi_driver()?

--
With Best Regards,
Andy Shevchenko