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] [thread-next>] [day] [month] [year] [list]
Message-ID:
 <PH0PR03MB7141E6D1A077B0E02368CBDDF9BA2@PH0PR03MB7141.namprd03.prod.outlook.com>
Date: Wed, 23 Apr 2025 07:50:51 +0000
From: "Paller, Kim Seer" <KimSeer.Paller@...log.com>
To: Jonathan Cameron <jic23@...nel.org>
CC: Lars-Peter Clausen <lars@...afoo.de>,
        "Hennerich, Michael"
	<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>,
        "Sa, Nuno" <Nuno.Sa@...log.com>, Andy Shevchenko
	<andy@...nel.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH v5 3/3] iio: dac: ad3530r: Add driver for AD3530R and
 AD3531R

> -----Original Message-----
> From: Jonathan Cameron <jic23@...nel.org>
> Sent: Monday, April 21, 2025 9:48 PM
> To: Paller, Kim Seer <KimSeer.Paller@...log.com>
> Cc: Lars-Peter Clausen <lars@...afoo.de>; Hennerich, Michael
> <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>; Sa,
> Nuno <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
> 
> [External]
> 
> 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..05bd191e5225bd267f42ba
> 36bbd42a18e6f22291
> > --- /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.

The motivation for using the chan->address field was to hide the calculation a bit.
However, would using a macro like 
#define AD3530R_OP_MODE_CHAN_MSK(chan)	GENMASK(2 * chan + 1, 2 * chan) 
be a good approach in this case? This drops the need for the address field and
can also be used to explicitly set the operating mode for the 4 fields of the register.
What do you think?

> 
> 
> > +	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) |
> 

Considering the above macro, would this also suffice?

	val = FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(0), AD3530R_NORMAL_OP) |
	      FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(1), AD3530R_NORMAL_OP) |
	      FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(2), AD3530R_NORMAL_OP) |
	      FIELD_PREP(AD3530R_OP_MODE_CHAN_MSK(3), AD3530R_NORMAL_OP);

	ret = regmap_write(st->regmap, AD3530R_OUTPUT_OPERATING_MODE_0, val);
	...

> 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,
> > +};