Message-ID: <20221009180925.441795c0@jic23-huawei>
Date:   Sun, 9 Oct 2022 18:09:25 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Ciprian Regus <ciprian.regus@...log.com>
Cc:     <krzysztof.kozlowski+dt@...aro.org>, <robh+dt@...nel.org>,
        <andriy.shevchenko@...ux.intel.com>,
        <linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>,
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH 2/2] drivers: iio: dac: Add AD5754 DAC driver

On Tue, 4 Oct 2022 10:18:25 +0300
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.
> 
> 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>

Hi Ciprian

I'll try and avoid to much duplication of the comments of others!

> ---
>  MAINTAINERS              |   8 +
>  drivers/iio/dac/Kconfig  |  12 +
>  drivers/iio/dac/Makefile |   1 +
>  drivers/iio/dac/ad5754.c | 668 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 689 insertions(+)
>  create mode 100644 drivers/iio/dac/ad5754.c
> 

> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 80521bd28d0f..013278b629f9 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -170,6 +170,18 @@ config AD5696_I2C
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ad5696.
>  
> +config AD5754
> +	tristate "Analog Devices AD5752/AD5732/AD5722/AD5754/AD5734/AD5724 DAC driver"

This is close to the 'and similar' boundary.  We can't keep extending names
in the menu option for ever. If you judge there to be little chance of other
parts being added to this driver thine as is. 

> +	depends on SPI
> +	select REGMAP_SPI
> +	help
> +	  Say yes here to build support for Analog Devices AD5752, AD5732,
> +	  AD5722, AD5754, AD5734, AD5724 dual/quad channel Digital to
> +	  Analog Converter.

What are the R parts and should we mention those?

> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad5754.
> +
>  config AD5755
>  	tristate "Analog Devices AD5755/AD5755-1/AD5757/AD5735/AD5737 DAC driver"
>  	depends on SPI_MASTER

> diff --git a/drivers/iio/dac/ad5754.c b/drivers/iio/dac/ad5754.c
> new file mode 100644
> index 000000000000..71623067363f
> --- /dev/null
> +++ b/drivers/iio/dac/ad5754.c
> @@ -0,0 +1,668 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Analog Devices, Inc.
> + * Author: Ciprian Regus <ciprian.regus@...log.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define AD5754_INT_VREF			2500
> +#define AD5754_FRAME_SIZE		3
> +#define AD5754_MAX_CHANNELS		4
> +#define AD5754_MAX_RESOLUTION		16
> +
> +#define AD5754_DATA_MASK(_lsb)		GENMASK(15, _lsb)
> +
> +#define AD5754_RANGE_MASK		GENMASK(2, 0)
> +
> +#define AD5754_REG_RD			BIT(7)
> +
> +#define AD5754_CLEAR_FUNC		BIT(2)
> +#define AD5754_LOAD_FUNC		(BIT(2) | BIT(0))
> +#define AD5754_NOOP_FUNC		GENMASK(4, 3)
> +
> +#define AD5754_PU_ADDR			0
> +#define AD5754_PU_MASK			GENMASK(3, 0)
> +#define AD5754_PU_CH(x)			BIT(x)
> +#define AD5754_INT_REF_MASK		BIT(4)
> +#define AD5754_INT_REF			BIT(4)
> +
> +#define AD5754_DAC_REG			0
> +#define AD5754_RANGE_REG		BIT(0)
> +#define AD5754_PWR_REG			BIT(1)
> +#define AD5754_CTRL_REG			GENMASK(1, 0)

Huh.  A really strange datasheet table for these as it breaks them
down into 3 separate 1 bit fields that are next to each other.
I'd ignore that and treat them as a 3 bit field.

Then these are just 0, 1, 2, 3 within that field.



> +
> +enum ad5754_num_channels {
> +	AD5754_2_CHANNELS,
> +	AD5754_4_CHANNELS,
> +};

This can go after you move away from an array of arrays of channels.


> +
> +const struct iio_chan_spec ad5754_channels[][AD5754_MAX_CHANNELS] = {
> +	[AD5754_2_CHANNELS] = {
> +		AD5754_CHANNEL(0),
> +		AD5754_CHANNEL(1),
> +	},
No real point in having these in an array.  Have
const struct iio_chan_spec ad5754_2_channels[] = { ... };
const struct iio_chan_spec ad5754_4_channels[] = { ... }

Storage is smaller and easier code also shorter as you don't
have to index the array.

> +	[AD5754_4_CHANNELS] = {
> +		AD5754_CHANNEL(0),
> +		AD5754_CHANNEL(1),
> +		AD5754_CHANNEL(2),
> +		AD5754_CHANNEL(3),
> +	},
> +};
> +
> +const struct ad5754_chip_info ad5754_chip_info_data[] = {
> +	[AD5722] = {
> +		.name = "ad5722",
> +		.resolution = 12,
> +		.data_mask = AD5754_DATA_MASK(4),
> +		.internal_vref = false,
> +		.num_channels = 2,
> +		.channels = ad5754_channels[AD5754_2_CHANNELS],
> +	},
> +	[AD5732] = {
> +		.name = "ad5732",
> +		.resolution = 14,
> +		.data_mask = AD5754_DATA_MASK(2),
> +		.internal_vref = false,
> +		.num_channels = 2,
> +		.channels = ad5754_channels[AD5754_2_CHANNELS],
> +	},
> +	[AD5752] = {
> +		.name = "ad5752",
> +		.resolution = 16,
> +		.data_mask = AD5754_DATA_MASK(0),
> +		.internal_vref = false,
> +		.num_channels = 2,
> +		.channels = ad5754_channels[AD5754_2_CHANNELS],
> +	},
> +	[AD5724] = {
> +		.name = "ad5724",
> +		.resolution = 12,
> +		.data_mask = AD5754_DATA_MASK(4),
> +		.internal_vref = false,
> +		.num_channels = 4,
> +		.channels = ad5754_channels[AD5754_4_CHANNELS],
> +	},
> +	[AD5734] = {
> +		.name = "ad5734",
> +		.resolution = 14,
> +		.data_mask = AD5754_DATA_MASK(2),
> +		.internal_vref = false,
> +		.num_channels = 4,
> +		.channels = ad5754_channels[AD5754_4_CHANNELS],
> +	},
> +	[AD5754] = {
> +		.name = "ad5754",
> +		.resolution = 16,
> +		.data_mask = AD5754_DATA_MASK(0),
> +		.internal_vref = false,
> +		.num_channels = 4,
> +		.channels = ad5754_channels[AD5754_4_CHANNELS],
> +	},
> +	[AD5722R] = {
> +		.name = "ad5722r",
> +		.resolution = 12,
> +		.data_mask = AD5754_DATA_MASK(4),
> +		.internal_vref = true,
> +		.num_channels = 2,
> +		.channels = ad5754_channels[AD5754_2_CHANNELS],
> +	},
> +	[AD5732R] = {
> +		.name = "ad5732r",
> +		.resolution = 14,
> +		.data_mask = AD5754_DATA_MASK(2),
> +		.internal_vref = true,
> +		.num_channels = 2,
> +		.channels = ad5754_channels[AD5754_2_CHANNELS],
> +	},
> +	[AD5752R] = {
> +		.name = "ad5752r",
> +		.resolution = 16,
> +		.data_mask = AD5754_DATA_MASK(0),
> +		.internal_vref = true,
> +		.num_channels = 2,
> +		.channels = ad5754_channels[AD5754_2_CHANNELS],
> +	},
> +	[AD5724R] = {
> +		.name = "ad5724r",
> +		.resolution = 12,
> +		.data_mask = AD5754_DATA_MASK(4),
> +		.internal_vref = true,
> +		.num_channels = 4,
> +		.channels = ad5754_channels[AD5754_4_CHANNELS],
> +	},
> +	[AD5734R] = {
> +		.name = "ad5734r",
> +		.resolution = 14,
> +		.data_mask = AD5754_DATA_MASK(2),
> +		.internal_vref = true,
> +		.num_channels = 4,
> +		.channels = ad5754_channels[AD5754_4_CHANNELS],
> +	},
> +	[AD5754R] = {
> +		.name = "ad5754r",
> +		.resolution = 16,
> +		.data_mask = AD5754_DATA_MASK(0),
> +		.internal_vref = true,
> +		.num_channels = 4,
> +		.channels = ad5754_channels[AD5754_4_CHANNELS],
> +	}

Add a comma here as we may add more entries in future and that will reduce
the noise in such a patch set

> +};
> +



> +static int ad5754_enable_channels(struct ad5754_state *st)
> +{
> +	struct fwnode_handle *channel_node = NULL;
> +	u32 real_channel;
> +	u32 power_reg;
> +	u32 index;
> +	int ret;
> +
> +	fwnode_for_each_available_child_node(dev_fwnode(st->dev), channel_node) {
> +		ret = fwnode_property_read_u32(channel_node, "reg", &index);
> +		if (ret) {
> +			dev_err(st->dev, "Failed to read channel reg: %d\n", ret);
> +			goto free_node;
> +		}
> +		if (index >= st->chip_info->num_channels) {
> +			dev_err(st->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);

I'm going to take a wild guess (without reading the datasheet) that this turned something on.
Do we not want to ensure it's turned off again in the remove / devm_ unwind path?

> +		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));
> +		if (ret)
> +			goto free_node;
> +
> +		/* Channel power up delay */
> +		fsleep(10);
> +	}
> +
> +	return 0;
> +
> +free_node:
> +	fwnode_handle_put(channel_node);
> +
> +	return ret;
> +}
> +


> +static int ad5754_int_vref_enable(struct ad5754_state *st)
> +{
> +	return 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));

Whilst it is going to make for a long line / less than perfect formatting
I'd prefer you don't have this wrapper, but just call the regmap_update_bits()
at the call site

> +}
> +
> +static void ad5754_disable_regulator(void *regulator)
> +{
> +	regulator_disable(regulator);
> +}
> +
> +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 regulator *vref_reg;
> +	struct iio_dev *indio_dev;
> +	struct ad5754_state *st;
> +	struct device *dev;
> +	int ret;
> +
> +	dev = &spi->dev;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	spi_set_drvdata(spi, indio_dev);

Used anywhere? I might have missed it.

> +
> +	st->spi = spi;
> +	st->dev = dev;

Keeping copies of dev and spi seems excessive.

> +	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;
> +
> +	st->regmap = devm_regmap_init(st->dev, NULL, st, &ad5754_regmap_config);
> +	if (IS_ERR(st->regmap))
> +		return dev_err_probe(st->dev, PTR_ERR(vref_reg),
> +				     "Regmap init error\n");
> +
> +	st->dac_max_code = BIT(st->chip_info->resolution) - 1;

Why cache this?  Just check against >= BIT(...) where needed

> +	st->sub_lsb = AD5754_MAX_RESOLUTION - st->chip_info->resolution;

Similarly, seems unnecessary to cache this value given how cheap it
is to compute.

> +
> +	vref_reg = devm_regulator_get_optional(st->dev, "vref");
> +	if (IS_ERR(vref_reg)) {
> +		if (!st->chip_info->internal_vref)
> +			return dev_err_probe(st->dev, PTR_ERR(vref_reg),
> +			       "Failed to get the vref regulator\n");
> +
> +		st->vref = AD5754_INT_VREF;
> +		ret = ad5754_int_vref_enable(st);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regulator_enable(vref_reg);
> +		if (ret)
> +			return dev_err_probe(st->dev, PTR_ERR(vref_reg),

ret

> +				"Failed to enable the vref regulator\n");
> +
> +		ret = devm_add_action_or_reset(dev, ad5754_disable_regulator, vref_reg);
> +		if (ret)
> +			return ret;
> +
> +		ret = regulator_get_voltage(vref_reg);

Could move this to where it's used.  Tends not to be a hot path so cost of asking
the supply for ti's voltage is normally fine.

> +		if (ret < 0)
> +			return dev_err_probe(dev, ret, "Failed to get vref\n");
> +
> +		st->vref = ret / 1000;
> +	}
> +
> +	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(st->dev, indio_dev);
> +}
> +

> +
> +static const struct spi_device_id ad5754_id[] = {
> +	{ "ad5722", (kernel_ulong_t)&ad5754_chip_info_data[AD5722], },
> +	{ "ad5732", (kernel_ulong_t)&ad5754_chip_info_data[AD5732], },
> +	{ "ad5752", (kernel_ulong_t)&ad5754_chip_info_data[AD5752], },
> +	{ "ad5724", (kernel_ulong_t)&ad5754_chip_info_data[AD5724], },
> +	{ "ad5734", (kernel_ulong_t)&ad5754_chip_info_data[AD5734], },
> +	{ "ad5754", (kernel_ulong_t)&ad5754_chip_info_data[AD5754], },
> +	{ "ad5722r", (kernel_ulong_t)&ad5754_chip_info_data[AD5722R], },
> +	{ "ad5732r", (kernel_ulong_t)&ad5754_chip_info_data[AD5732R], },
> +	{ "ad5752r", (kernel_ulong_t)&ad5754_chip_info_data[AD5752R], },
> +	{ "ad5724r", (kernel_ulong_t)&ad5754_chip_info_data[AD5724R], },
> +	{ "ad5734r", (kernel_ulong_t)&ad5754_chip_info_data[AD5734R], },
> +	{ "ad5754r", (kernel_ulong_t)&ad5754_chip_info_data[AD5754R], },
> +	{},

Nice to drop commas on null terminators as nothing can ever come after them anyway.

> +};
> +
> +static const struct of_device_id ad5754_dt_id[] = {
> +	{
> +		.compatible = "adi,ad5722",
> +		.data = &ad5754_chip_info_data[AD5722],
> +	},
> +	{

If there was ever a case for the acceptable option of 
	}, {
this is it :)

> +		.compatible = "adi,ad5732",
> +		.data = &ad5754_chip_info_data[AD5732],
> +	},
> +	{
> +		.compatible = "adi,ad5752",
> +		.data = &ad5754_chip_info_data[AD5752],
> +	},
> +	{
> +		.compatible = "adi,ad5724",
> +		.data = &ad5754_chip_info_data[AD5724],
> +	},
> +	{
> +		.compatible = "adi,ad5734",
> +		.data = &ad5754_chip_info_data[AD5734],
> +	},
> +	{
> +		.compatible = "adi,ad5754",
> +		.data = &ad5754_chip_info_data[AD5754],
> +	},
> +	{
> +		.compatible = "adi,ad5722r",
> +		.data = &ad5754_chip_info_data[AD5722R],
> +	},
> +	{
> +		.compatible = "adi,ad5732r",
> +		.data = &ad5754_chip_info_data[AD5732R],
> +	},
> +	{
> +		.compatible = "adi,ad5752r",
> +		.data = &ad5754_chip_info_data[AD5752R],
> +	},
> +	{
> +		.compatible = "adi,ad5724r",
> +		.data = &ad5754_chip_info_data[AD5724R],
> +	},
> +	{
> +		.compatible = "adi,ad5734r",
> +		.data = &ad5754_chip_info_data[AD5734R],
> +	},
> +	{
> +		.compatible = "adi,ad5754r",
> +		.data = &ad5754_chip_info_data[AD5754R],
> +	},
> +	{},

No comma on NULL terminators.

> +};
> +MODULE_DEVICE_TABLE(of, ad5754_dt_id);

Powered by blists - more mailing lists