Message-ID:
 <PH0PR03MB71410D0F7716DAB4A2103AA1F9BA2@PH0PR03MB7141.namprd03.prod.outlook.com>
Date: Wed, 23 Apr 2025 07:53:37 +0000
From: "Paller, Kim Seer" <KimSeer.Paller@...log.com>
To: Andy Shevchenko <andy@...nel.org>
CC: Jonathan Cameron <jic23@...nel.org>, 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>,
        "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: Andy Shevchenko <andy@...nel.org>
> Sent: Tuesday, April 22, 2025 11:11 PM
> To: Paller, Kim Seer <KimSeer.Paller@...log.com>
> Cc: Jonathan Cameron <jic23@...nel.org>; 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>; 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, Apr 21, 2025 at 12:24:54PM +0800, Kim Seer Paller 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.
> 
> ...
> 
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/delay.h>
> 
> > +#include <linux/device.h>
> 
> I don't see how you use this. But
> 
> + dev_printk.h
> 
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/mutex.h>
> > +#include <linux/property.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h> #include <linux/spi/spi.h>
> 
> ...
> 
> > +#define AD3530R_SW_RESET			(BIT(7) | BIT(0))
> 
> > +#define AD3530R_MAX_CHANNELS			8
> > +#define AD3531R_MAX_CHANNELS			4
> 
> Sounds to me that these two shouldn't be grouped with the rest here. Perhaps
> move them out to after the LDAC_PULSE?
> 
> > +#define AD3530R_INTERNAL_VREF_MV		2500
> 
> _mV (yes, with Volts and Amperes we use proper spelling).
> 
> > +#define AD3530R_LDAC_PULSE_US			100
> 
> ...
> 
> > +	int (*input_ch_reg)(unsigned int c);
> 
> c? channel?

Thank you for the feedback. I'll change this to 'channel' to make it clearer.

> 
> ...
> 
> > +	int vref_mv;
> 
> _mV
> 
> ...
> 
> > +static int ad3530r_input_ch_reg(unsigned int c) {
> > +	return 2 * c + AD3530R_INPUT_CH;
> > +}
> > +
> > +static int ad3531r_input_ch_reg(unsigned int c) {
> > +	return 2 * c + AD3531R_INPUT_CH;
> > +}
> 
> c --> channel
> 
> ...
> 
> > +static const char * const ad3530r_powerdown_modes[] = {
> > +	"1kohm_to_gnd",
> 
> kOhm
> 
> > +	"7.7kohm_to_gnd",
> 
> Ditto.
> 
> > +	"32kohm_to_gnd",
> 
> Ditto.
> 
> > +};
> 
> ...
> 
> > +static const struct iio_enum ad3530r_powerdown_mode_enum = {
> > +	.items = ad3530r_powerdown_modes,
> > +	.num_items = ARRAY_SIZE(ad3530r_powerdown_modes),
> 
> + array_size.h
> 
> > +	.get = ad3530r_get_powerdown_mode,
> > +	.set = ad3530r_set_powerdown_mode,
> > +};
> 
> ...
> 
> > +static ssize_t ad3530r_get_dac_powerdown(struct iio_dev *indio_dev,
> > +					 uintptr_t private,
> > +					 const struct iio_chan_spec *chan,
> > +					 char *buf)
> > +{
> > +	struct ad3530r_state *st = iio_priv(indio_dev);
> > +
> > +	guard(mutex)(&st->lock);
> > +	return sysfs_emit(buf, "%d\n", st->chan[chan->channel].powerdown);
> 
> + sysfs.h
> 
> > +}
> 
> ...
> 
> > +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);
> 
> + kstrtox.h
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	guard(mutex)(&st->lock);
> > +	mask = GENMASK(chan->address + 1, chan->address);
> > +	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;
> 
> Please, move the operator to the previous line, Or even
> 
> 	... pdmode;
> 
> 	pdmode = powerdown ? st->chan[chan->channel].powerdown_mode :
> 0;
> 	val = pdmode << ...;
> 
> > +	ret = regmap_update_bits(st->regmap, reg, mask, val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->chan[chan->channel].powerdown = powerdown;
> > +
> > +	return len;
> > +}
> 
> ...
> 
> > +	struct ad3530r_state *st = iio_priv(indio_dev);
> > +
> > +	switch (info) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (val < 0 || val > U16_MAX)
> 
> U16_MAX is an abstract type with this limit, do you have any predefined HW
> limit instead? Probably better to use that one as defined via BIT() / GENMASK().

Will add a macro for this hardware limit.

> 
> > +			return -EINVAL;
> > +
> > +		return ad3530r_dac_write(st, chan->channel, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> 
> ...
> 
> > +static const struct iio_chan_spec_ext_info ad3530r_ext_info[] = {
> > +	{
> > +		.name = "powerdown",
> > +		.shared = IIO_SEPARATE,
> > +		.read = ad3530r_get_dac_powerdown,
> > +		.write = ad3530r_set_dac_powerdown,
> > +	},
> > +	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, _pos) {
> 	\
> 
> Slightly better to have the curly braces on the same column as it's easier to
> read.
> 
> #define AD3530R_CHAN(_chan, _pos)				\
> {								\
> 
> (and make it one TAB less for the backslash).
> 
> > +	.type = IIO_VOLTAGE,						\
> > +	.indexed = 1,							\
> > +	.channel = _chan,						\
> > +	.address = _pos,						\
> > +	.output = 1,							\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> 	\
> > +			      BIT(IIO_CHAN_INFO_SCALE),			\
> > +	.ext_info = ad3530r_ext_info,					\
> > +}
> 
> ...
> 
> > +static int ad3530r_setup(struct ad3530r_state *st, int vref,
> > +			 bool has_external_vref)
> > +{
> > +	struct device *dev = regmap_get_device(st->regmap);
> > +	struct gpio_desc *reset_gpio;
> > +	int i, ret;
> > +	u8 range_multiplier;
> 
> + types.h (and especially for boolean type along with true/false definitions.
> 
> > +
> > +	reset_gpio = devm_gpiod_get_optional(dev, "reset",
> GPIOD_OUT_HIGH);
> > +	if (IS_ERR(reset_gpio))
> > +		return dev_err_probe(dev, PTR_ERR(reset_gpio),
> > +				     "Failed to get reset GPIO\n");
> 
> + err.h
> 
> > +	if (reset_gpio) {
> > +		/* Perform hardware reset */
> > +		fsleep(1000);
> 
> (1 * USEC_PER_MSEC) ?
> 
> > +		gpiod_set_value_cansleep(reset_gpio, 0);
> > +	} else {
> > +		/* Perform software reset */
> > +		ret = regmap_update_bits(st->regmap,
> AD3530R_INTERFACE_CONFIG_A,
> > +					 AD3530R_SW_RESET,
> AD3530R_SW_RESET);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> > +	fsleep(10000);
> 
> 10 * USEC_PER_MSEC
> 
> With these constants it's less error prone (when 3 or more zeroes) and easier to
> get the units without looking into fsleep() implementation / documentation.

I'll take note of this.

> 
> > +	range_multiplier = 1;
> > +	if (device_property_read_bool(dev, "adi,range-double")) {
> > +		ret = regmap_set_bits(st->regmap,
> AD3530R_OUTPUT_CONTROL_0,
> > +				      AD3530R_OUTPUT_CONTROL_RANGE);
> > +		if (ret)
> > +			return ret;
> > +
> > +		range_multiplier = 2;
> > +	}
> > +
> > +	if (!has_external_vref && st->chip_info->internal_ref_support) {
> > +		ret = regmap_set_bits(st->regmap,
> AD3530R_REFERENCE_CONTROL_0,
> > +				      AD3530R_REFERENCE_CONTROL_SEL);
> > +		if (ret)
> > +			return ret;
> > +
> > +		st->vref_mv = range_multiplier *
> AD3530R_INTERNAL_VREF_MV;
> > +	}
> > +
> > +	if (has_external_vref)
> > +		st->vref_mv = range_multiplier * vref / 1000;
> 
> MILLI?

Yes this is milli, will change this also to vref_mV

	st->vref_mV = range_multiplier * vref_mV / 1000;

> 
> 
> > +	/* Set operating mode to normal operation. */
> > +	ret = regmap_write(st->regmap,
> AD3530R_OUTPUT_OPERATING_MODE_0,
> > +			   AD3530R_NORMAL_OPERATION);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (st->chip_info->num_channels > AD3531R_MAX_CHANNELS) {
> > +		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;
> > +}
> 
> ...
> 
> > +	vref = devm_regulator_get_enable_read_voltage(dev, "ref");
> > +	if (vref < 0 && vref != -ENODEV)
> > +		return vref;
> > +
> > +	has_external_vref = vref != -ENODEV;
> 
> Wouldn't be better just make this 0 when it's == -ENODEV and check just the
> value without having this additional boolean variable (note, I haven't checked
> the meaning of Vref == 0 in case it's possible in real life and hardware behaves
> adequately)?

I think it could be simpler to set vref to 0 when it's -ENODEV and check its value directly
without having additional boolean variable. I'll try this approach.

> 
> > +	if (!st->chip_info->internal_ref_support && !has_external_vref)
> > +		return -ENODEV;
> 
> > +	ret = ad3530r_setup(st, vref, has_external_vref);
> > +	if (ret)
> > +		return ret;
> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Powered by blists - more mailing lists