linux-kernel - Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and AD3531R

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250405171155.091edbae@jic23-huawei>
Date: Sat, 5 Apr 2025 17:11:55 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Kim Seer Paller <kimseer.paller@...log.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Rob Herring <robh@...nel.org>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <devicetree@...r.kernel.org>
Subject: Re: [PATCH v3 3/3] iio: dac: ad3530r: Add driver for AD3530R and
 AD3531R

On Thu, 3 Apr 2025 13:33:57 +0800
Kim Seer Paller <kimseer.paller@...log.com> wrote:

Hi Kim,
> The AD3530/AD3530R (8-channel) and AD3531/AD3531R (4-channel) are
> low-power, 16-bit, buffered voltage output DACs with software-
> programmable gain controls, providing full-scale output spans of 2.5V or
> 5V for reference voltages of 2.5V. These devices operate from a single
> 2.7V to 5.5V supply and are guaranteed monotonic by design. The "R"
> variants include a 2.5V, 5ppm/°C internal reference, which is disabled
> by default.


As below. Given the bindings provide for use with an external ADC to read
a wide range of signals, if intent is not to provide that support 'yet'
(which is fine) then add a paragraph here to say that.

> 
> Signed-off-by: Kim Seer Paller <kimseer.paller@...log.com>


A few additional comments from me inline.  Particular fun is the
long running SPI regmap DMA safety assumptions question.
For now I think we have to assume bulk read/write still need
a DMA safe buffer.

Jonathan

> diff --git a/drivers/iio/dac/ad3530r.c b/drivers/iio/dac/ad3530r.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..4b757e19f0c8349999f72e53abb1a4f483a44eb2
> --- /dev/null
> +++ b/drivers/iio/dac/ad3530r.c
> @@ -0,0 +1,514 @@

> +
> +#define AD3530R_INTERFACE_CONFIG_A		0x00
> +#define AD3530R_OUTPUT_OPERATING_MODE_0		0x20
> +#define AD3530R_OUTPUT_OPERATING_MODE_1		0x21
> +#define AD3530R_OUTPUT_CONTROL_0		0x2A
> +#define AD3530R_REFERENCE_CONTROL_0		0x3C
> +#define AD3530R_SW_LDAC_TRIG_A			0xE5
> +#define AD3530R_INPUT_CH(c)			(2 * (c) + 0xEB)
> +
> +#define AD3531R_SW_LDAC_TRIG_A			0xDD
> +#define AD3531R_INPUT_CH(c)			(2 * (c) + 0xE3)

I'd add a define for the first channel and then just have the
rest of this in the device specific function that uses it.

> +
> +#define AD3530R_SW_LDAC_TRIG_MASK		BIT(7)
> +#define AD3530R_OUTPUT_CONTROL_MASK		BIT(2)
> +#define AD3530R_REFERENCE_CONTROL_MASK		BIT(0)
> +#define AD3530R_REG_VAL_MASK			GENMASK(15, 0)
> +
> +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
> +#define AD3530R_MAX_CHANNELS			8
> +#define AD3531R_MAX_CHANNELS			4
> +#define AD3530R_CH(c)				(c)

This seems a little unnecessary...
Given it is only called in device specific functions I'd just encode that
there.

> +#define AD3530R_32KOHM_POWERDOWN_MODE		3
> +#define AD3530R_INTERNAL_VREF_MV		2500
> +#define AD3530R_LDAC_PULSE_US			100

> +static int ad3530r_dac_write(struct ad3530r_state *st, unsigned int chan,
> +			     unsigned int val)
> +{
> +	int ret;
> +	__be16 reg_val;
> +
> +	guard(mutex)(&st->lock);
> +	reg_val = cpu_to_be16(val);
> +
> +	ret = regmap_bulk_write(st->regmap, st->chip_info->input_ch_reg(chan),
> +				&reg_val, sizeof(reg_val));

As below. regmap_bulk_write() shouldn't assume the buffer is bounced so
needs a DMA safe buffer.

> +	if (ret)
> +		return ret;
> +
> +	if (st->ldac_gpio)
> +		return ad3530r_trigger_hw_ldac(st->ldac_gpio);
> +
> +	return regmap_update_bits(st->regmap, st->chip_info->sw_ldac_trig_reg,
> +				  AD3530R_SW_LDAC_TRIG_MASK,
> +				  FIELD_PREP(AD3530R_SW_LDAC_TRIG_MASK, 1));
> +}
> +
> +static int ad3530r_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long info)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	__be16 reg_val;
> +
> +	switch (info) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_bulk_read(st->regmap,
> +				       st->chip_info->input_ch_reg(chan->channel),
> +				       &reg_val, sizeof(reg_val));

So this runs into the old question of whether we need a DMA safe buffer
for a regmap_bulk_read when the bus is SPI.  In practice we probably
don't because of internal details of the regmap but I believe nothing has
changed on the guidance to not assume that in drivers.  So this regval
should be DMA safe. Easiest option is a
__be16 val __aligned(IIO_DMA_MINALIGN) at then end of the st structure
though we will also need to take a mutex to prevent multiple uses of that
buffer.

Maybe we should revisit this with Mark. I checked briefly and 'think'
there is always a copy.

> +		if (ret)
> +			return ret;
> +
> +		*val = FIELD_GET(AD3530R_REG_VAL_MASK, be16_to_cpu(reg_val));
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mv;
> +		*val2 = 16;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +#define AD3530R_CHAN_EXT_INFO(_name, _what, _shared, _read, _write) {	\
> +	.name = (_name),						\
> +	.read = (_read),						\
> +	.write = (_write),						\
> +	.private = (_what),						\
> +	.shared = (_shared),						\
> +}
> +
> +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> +	AD3530R_CHAN_EXT_INFO("powerdown", 0, IIO_SEPARATE,
> +			      ad3530r_get_dac_powerdown,
> +			      ad3530r_set_dac_powerdown),
As there is only one don't use a macro.


	{
		.name = "powerdown",
		.shared = IIO_SEPARATE,
		.read = ...
		.write= ...
	},

> +	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &ad3530r_powerdown_mode_enum),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", IIO_SHARED_BY_TYPE,
> +			   &ad3530r_powerdown_mode_enum),
> +	{ },
> +};
> +
> +#define AD3530R_CHAN(_chan) {						\
> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.channel = _chan,						\
> +	.output = 1,							\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |			\
> +			      BIT(IIO_CHAN_INFO_SCALE),			\
> +	.ext_info = ad3530r_ext_info,					\
> +}
> +
> +static const struct iio_chan_spec ad3530r_channels[] = {

Hmm. You only have output channels but we have the stuff
to read the mux value on a pin.  (Nuno mentioned this as well).
If that is enabled I'd expect to see all the input channels to reflect what
we might read by putting it on that pin and reading it with an ADC we
are consumer.

I'm fine if we make that a job for another day though
just state that really clearly in the patch description.

> +	AD3530R_CHAN(0),
> +	AD3530R_CHAN(1),
> +	AD3530R_CHAN(2),
> +	AD3530R_CHAN(3),
> +	AD3530R_CHAN(4),
> +	AD3530R_CHAN(5),
> +	AD3530R_CHAN(6),
> +	AD3530R_CHAN(7),
> +};
> +
> +static const struct iio_chan_spec ad3531r_channels[] = {
> +	AD3530R_CHAN(0),
> +	AD3530R_CHAN(1),
> +	AD3530R_CHAN(2),
> +	AD3530R_CHAN(3),
> +};