linux-kernel - Re: [PATCH v5 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] [day] [month] [year] [list]

Message-ID: <20250421144800.0db0a84e@jic23-huawei>
Date: Mon, 21 Apr 2025 14:48:00 +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>, David
 Lechner <dlechner@...libre.com>, Nuno Sá
 <noname.nuno@...il.com>, Nuno Sá <nuno.sa@...log.com>, Andy
 Shevchenko <andy@...nel.org>, <linux-iio@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and
 AD3531R

On Mon, 21 Apr 2025 12:24:54 +0800
Kim Seer Paller <kimseer.paller@...log.com> wrote:

> 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.
> 
> Support for monitoring internal die temperature, output voltages, and
> current of a selected channel via the MUXOUT pin using an external ADC
> is currently not implemented.
> 
> Reviewed-by: David Lechner <dlechner@...libre.com>
> Signed-off-by: Kim Seer Paller <kimseer.paller@...log.com>
Hi.

Just one thing from a final pre merge look through.

The initialization of powerdown mode works but only because the NORMAL
mode == 0.  That should be setting it explicitly for each set of 4 channels
as needed.

I don't really mind how you solve that.  There are lots of options
to build up the 4 fields in each of those registers.

Jonathan

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

> +
> +static ssize_t ad3530r_set_dac_powerdown(struct iio_dev *indio_dev,
> +					 uintptr_t private,
> +					 const struct iio_chan_spec *chan,
> +					 const char *buf, size_t len)
> +{
> +	struct ad3530r_state *st = iio_priv(indio_dev);
> +	int ret;
> +	unsigned int mask, val, reg;
> +	bool powerdown;
> +
> +	ret = kstrtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	guard(mutex)(&st->lock);
> +	mask = GENMASK(chan->address + 1, chan->address);

I think maybe we need a macro to get the mask from the channel number?
Using address for this seems overkill given how simple that maths is.
Ideally that macro could perhaps be used in the code below to avoid
all the defines I suggested.


> +	reg = chan->channel < AD3531R_MAX_CHANNELS ?
> +	      AD3530R_OUTPUT_OPERATING_MODE_0 :
> +	      AD3530R_OUTPUT_OPERATING_MODE_1;
> +	val = (powerdown ? st->chan[chan->channel].powerdown_mode : 0)
> +	       << chan->address;
> +


> +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> +			 bool has_external_vref)
> +{

> +
> +	if (has_external_vref)
> +		st->vref_mv = range_multiplier * vref / 1000;
> +
> +	/* Set operating mode to normal operation. */
> +	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0,
> +			   AD3530R_NORMAL_OPERATION);
Is this actually doing that?  I think the register is lots of 2 bit
fields and this is only setting it for the first channel?

This works because that value is 0.  Logically however we should set it
to
	(AD3530R_NORMAL_OPERATION << 6) |
	(AD3530R_NORMAL_OPERATION << 4) |
	(AD3530R_NORMAL_OPERATION << 2) |
	(AD3530R_NORMAL_OPERATION << 0)

Or possibly better as

	FIELD_PREP(AD3530R_OP_MODE_0_CHAN0_MSK, AD3530R_NORMAL_OPERATION) |
	FIELD_PREP(AD3530R_OP_MODE_0_CHAN1_MSK, AD3530R_NORMAL_OPERATION) |

etc

Names are a bit long, so maybe consider shortening some of the defines.

> +	if (ret)
> +		return ret;
> +
> +	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {

If we have it explicit that we have multiple fields this will become more obvious.
However I'd use the number 4 here rather than the number of channels the AD3531R happens
to have.


> +		ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_1,
> +				   AD3530R_NORMAL_OPERATION);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < st->chip_info->num_channels; i++)
> +		st->chan[i].powerdown_mode = AD3530R_POWERDOWN_32K;
> +
> +	st->ldac_gpio = devm_gpiod_get_optional(dev, "ldac", GPIOD_OUT_LOW);
> +	if (IS_ERR(st->ldac_gpio))
> +		return dev_err_probe(dev, PTR_ERR(st->ldac_gpio),
> +				     "Failed to get ldac GPIO\n");
> +
> +	return 0;
> +}
> +
> +static const struct regmap_config ad3530r_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.max_register = AD3530R_MAX_REG_ADDR,
> +};