Date:   Sat, 5 Nov 2022 13:20:45 +0000
From:   Jonathan Cameron <jic23@...nel.org>
To:     Ciprian Regus <ciprian.regus@...log.com>
Cc:     <krzysztof.kozlowski+dt@...aro.org>, <robh+dt@...nel.org>,
        <linux-iio@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] drivers: iio: dac: Add AD5754 DAC driver

On Fri, 4 Nov 2022 19:23:43 +0200
Ciprian Regus <ciprian.regus@...log.com> 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.

Trivial, but for patch descriptions, normal wrap is around 72-75 characters.
This looks a bit short so could be rewrapped to be a tiny bit nicer.
(I'd fix that up whilst applying but given you are going to be doing a v3
great if you can tidy it up!)

Some comments inline. Looking pretty good in general.

> 
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5724_5734_5754.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5722_5732_5752.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5724r_5734r_5754r.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5722R_5732R_5752R.pdf
> Signed-off-by: Ciprian Regus <ciprian.regus@...log.com>
> ---
> changes in v2:
>  - replaced the entire supported devices list in Kconfig with 'and similar'.
>  - mentioned the devices that have internal references as being supported.
>  - added the _mV suffix to the AD5754_INT_VREF macro.
>  - changed the macros by using numbers (where it was needed) instead of GENMASK or BIT.
>  - declared global variables as static.
>  - added 2 different arrays for 2 and 4 channels instead of using an array of arrays.
>  - set the CLR GPIO to high.
>  - removed the dev field in the state struct.
>  - casted the chip_info struct (probe function) to void *, so the line will look better.
>  - added the uV_PER_mV macro for vref mV conversions.
>  - removed commas from terminator lines.
>  - used module_spi_driver() instead of module_driver().
>  - powered off the internal reference or regulator and channels output on remove.
>  - removed the ad5754_int_vref_enable() function.
>  - fixed some probe return values.
>  - removed the dac_max_code and sub_lsb caching.
>  - simplified the scale attribute computation.
>  - set the max_register field in the regmap_config struct. 
>  MAINTAINERS              |   8 +
>  drivers/iio/dac/Kconfig  |  12 +
>  drivers/iio/dac/Makefile |   1 +
>  drivers/iio/dac/ad5754.c | 647 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 668 insertions(+)
>  create mode 100644 drivers/iio/dac/ad5754.c
> 
...

> diff --git a/drivers/iio/dac/ad5754.c b/drivers/iio/dac/ad5754.c
> new file mode 100644
> index 000000000000..ee52fcbef3a2
> --- /dev/null
> +++ b/drivers/iio/dac/ad5754.c
> @@ -0,0 +1,647 @@

> +
> +/*
> + * The channel addresses for 2 channel chip variants are not sequential:
> + *      A2 A1 A0 Channel
> + *	0  0  0   DAC A
> + *	0  1  0   DAC B
> + *
> + * This is not the case for 4 channel chips:
> + *	A2 A1 A0 Channel
> + *	0  0  0   DAC A
> + *	0  0  1   DAC B
> + *	0  1  0   DAC C
> + *	0  1  1   DAC D
> + */

> +
> +static int ad5754_reg_read(void *context, unsigned int reg, unsigned int *val)
> +{
> +	struct ad5754_state *st = context;
> +	struct spi_transfer xfer[] = {
> +		{
> +			.tx_buf = st->buff,
> +			.len = AD5754_FRAME_SIZE,
> +		},
> +	};
> +	int ret;
> +
> +	st->buff[0] = AD5754_REG_RD | reg;
> +	ret = spi_sync_transfer(st->spi, xfer, 1);
> +	if (ret)
> +		return ret;

We have two writes in here that are ultimately triggered from userspace sysfs
writes.  They aren't necessarily serialized.  So I'd expect to see some
locking in here as we can't rely on the bus locking (it doesn't know the
two spi_sync_transfer() are related obviously!)

> +
> +	xfer->rx_buf = st->buff;
> +	st->buff[0] = AD5754_REG_ADDR(AD5754_CTRL_REG, AD5754_NOOP_FUNC);
> +	st->buff[1] = 0;
> +	st->buff[2] = 0;
> +	ret = spi_sync_transfer(st->spi, xfer, 1);
> +	if (ret)
> +		return ret;
> +
> +	*val = get_unaligned_be16(&st->buff[1]);
> +
> +	return 0;
> +};

> +static int ad5754_set_dac_code(struct ad5754_state *st, u32 channel, u32 code)
> +{
> +	u32 sub_lsb = AD5754_MAX_RESOLUTION - st->chip_info->resolution;
> +	struct reg_sequence xfer_seq[2] = {
> +		{ AD5754_REG_ADDR(AD5754_DAC_REG, channel), code << sub_lsb },
> +		{ AD5754_REG_ADDR(AD5754_CTRL_REG, AD5754_LOAD_FUNC), 0 },
> +	};
> +
> +	return regmap_multi_reg_write(st->regmap, xfer_seq, 2);
ARRAY_SIZE() for that 2.

This is nice way of doing this.  I wonder how many other drivers
would benefit from similar use of regmap_multi_reg_write().

> +}
> +
> +static int ad5754_enable_channels(struct ad5754_state *st)
> +{
> +	struct fwnode_handle *channel_node;
> +	u32 real_channel;
> +	u32 power_reg;
> +	u32 index;
> +	int ret;
> +
> +	device_for_each_child_node(&st->spi->dev, channel_node) {
> +		ret = fwnode_property_read_u32(channel_node, "reg", &index);
> +		if (ret) {
> +			dev_err(&st->spi->dev, "Failed to read channel reg: %d\n", ret);
> +			goto free_node;
> +		}
> +		if (index >= st->chip_info->num_channels) {
> +			dev_err(&st->spi->dev, "Channel index %u is too large\n", index);
> +			goto free_node;
> +		}
> +
> +		ret = ad5754_real_ch(st, index, &real_channel);
> +		if (ret)
> +			goto free_node;
> +
> +		ret = ad5754_get_output_range(st, channel_node, index);
> +		if (ret)
> +			goto free_node;
> +
> +		ret = regmap_write_bits(st->regmap,
> +					AD5754_REG_ADDR(AD5754_RANGE_REG, real_channel),
> +					AD5754_RANGE_MASK, st->range_idx[index]);
> +		if (ret)
> +			goto free_node;
> +
> +		ret = regmap_read(st->regmap,
> +				  AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
> +				  &power_reg);
> +		if  (ret)
> +			goto free_node;
> +
> +		ret = regmap_update_bits(st->regmap,
> +					 AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
> +					 AD5754_PU_MASK, AD5754_PU_CH(real_channel) |
> +					 (power_reg & AD5754_PU_MASK));

This function should be side effect free if an error occurs on later channels.
So I'd expect to see any bits to do with enabling undone on the error path rather than
relying on a cleanup routine registered elsewhere.

> +		if (ret)
> +			goto free_node;
> +
> +		/* Channel power up delay */
> +		fsleep(10);

It's short enough that it probably doesn't matter, but do you need to do this for each
channel, or can you turn them all on and sleep only once?

> +	}
> +
> +	return 0;
> +
> +free_node:
> +	fwnode_handle_put(channel_node);
> +
> +	return ret;
> +}
> +

...

> +
> +static void ad5754_power_off(void *state)
> +{
> +	struct ad5754_state *st = state;
> +
> +	/* Power off the output for all channels */
> +	regmap_update_bits(st->regmap,
> +			   AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
> +			   AD5754_PU_MASK, FIELD_PREP(AD5754_PU_MASK, 0));

I think this first section is undoing what was configured in ad5754_enabled_channels()?
It presumably doesn't matter if we turn off channels when none have been turned on yet,
but from an ease of review point of view I'd prefer to see this callback split
into 2 (maybe 3) parts. Whether to separate the regulator disable vs internal ref
handling separately is less import (but might be slightly nicer) - but register
callbacks for that bit where you current register this one + register another callback
to do the power off above, only after ad5754_enable_channels().

That will bring the complexity that you'll need to make that function not have side effects
if an error occurs by turning these off manually there - the advantage is that the code
becomes easier to review / maintain as that is local.

> +
> +	if (!st->chip_info->internal_vref)
> +		regulator_disable(st->vref_reg);
> +	else
> +		regmap_update_bits(st->regmap,
> +				   AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
> +				   AD5754_INT_REF_MASK,
> +				   FIELD_PREP(AD5754_INT_REF_MASK, 0));
> +}
> +
> +static const struct iio_info ad5754_info = {
> +	.read_raw = &ad5754_read_raw,
> +	.write_raw = &ad5754_write_raw,
> +};
> +
> +static int ad5754_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad5754_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	st->spi = spi;
> +	st->chip_info = device_get_match_data(dev);
> +	if (!st->chip_info)
> +		st->chip_info = (void *)spi_get_device_id(spi)->driver_data;
> +
> +	st->regmap = devm_regmap_init(dev, NULL, st, &ad5754_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(dev, PTR_ERR(st->regmap),
> +				     "Regmap init error\n");
> +
> +	st->clr_gpio = devm_gpiod_get_optional(dev, "clr", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->clr_gpio))
> +		return PTR_ERR(st->clr_gpio);
> +
> +	/* Clear the DAC code registers */
> +	ret = regmap_write(st->regmap,
> +			   AD5754_REG_ADDR(AD5754_CTRL_REG, AD5754_CLEAR_FUNC),
> +			   0);
> +	if (ret)
> +		return ret;
> +
> +	st->vref_reg = devm_regulator_get_optional(dev, "vref");
> +	if (IS_ERR(st->vref_reg)) {
> +		if (!st->chip_info->internal_vref)
> +			return dev_err_probe(dev, PTR_ERR(st->vref_reg),
> +					     "Failed to get the vref regulator\n");
> +
> +		st->vref = AD5754_INT_VREF_mV;
> +		ret = regmap_update_bits(st->regmap,
> +					 AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
> +					 AD5754_INT_REF_MASK,
> +					 FIELD_PREP(AD5754_INT_REF_MASK, 1));
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regulator_enable(st->vref_reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to enable the vref regulator\n");

As mentioned above, I would suggest thinking about splitting the unwinding of the two
if/else paths here given there is really no overlap in what needs to be done.
It will result in more code due to two callbacks, but be slightly easier to follow.

> +
> +		ret = regulator_get_voltage(st->vref_reg);
> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to get vref\n");
> +
> +		st->vref = ret / uV_PER_mV;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, ad5754_power_off, st);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->name = st->chip_info->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->info = &ad5754_info;
> +	indio_dev->channels = st->chip_info->channels;
> +	indio_dev->num_channels = st->chip_info->num_channels;
> +
> +	ret = ad5754_enable_channels(st);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Powered by blists - more mailing lists