* Re: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-18 5:13 ` [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC Alexis Czezar Torreno
@ 2026-03-18 8:16 ` Andy Shevchenko
2026-03-19 5:23 ` Torreno, Alexis Czezar
2026-03-21 18:39 ` Jonathan Cameron
2026-03-25 11:02 ` Nuno Sá
2 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2026-03-18 8:16 UTC (permalink / raw)
To: Alexis Czezar Torreno
Cc: Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree,
linux-kernel
On Wed, Mar 18, 2026 at 01:13:36PM +0800, Alexis Czezar Torreno wrote:
> Add support for the Analog Devices AD5706R, a 4-channel 16-bit
> current output digital-to-analog converter with SPI interface.
>
> Features:
> - 4 independent DAC channels
> - Hardware and software LDAC trigger
> - Configurable output range
> - PWM-based LDAC control
> - Dither and toggle modes
> - Dynamically configurable SPI speed
> ---
> Changes since v1:
> - Removed PWM, GPIO, clock generator, debugfs, regmap, IIO_BUFFER
Why was regmap removed?! Was it not used?
> - Removed all custom ext_info sysfs attributes
> - Simplified to basic raw read/write and read-only scale
> - SPI read/write can handle multibyte registers
...
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
+ errno.h
> +#include <linux/iio/iio.h>
+ mod_devicetable.h
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.h>
+ types.h
> +#include <linux/unaligned.h>
Follow IWYU principle.
...
> +static int ad5706r_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct ad5706r_state *st = iio_priv(indio_dev);
> + u16 reg_val;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + scoped_guard(mutex, &st->lock) {
Can't it be simply guard()() ?
> + ret = ad5706r_spi_read(st, AD5706R_REG_DAC_DATA_READBACK_CH(chan->channel),
It's too long line.
> + ®_val);
> +
> + if (ret)
> + return ret;
> +
> + *val = reg_val;
> + }
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = 50;
> + *val2 = AD5706R_DAC_RESOLUTION;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + }
> +
> + return -EINVAL;
> +}
...
> +static int ad5706r_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct ad5706r_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val < 0 || val >= AD5706R_DAC_MAX_CODE)
in_range()?
(will need minmax.h)
> + return -EINVAL;
> +
> + guard(mutex)(&st->lock);
> + return ad5706r_spi_write(st,
> + AD5706R_REG_DAC_INPUT_A_CH(chan->channel),
> + val);
> + default:
> + return -EINVAL;
> + }
> +}
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-18 8:16 ` Andy Shevchenko
@ 2026-03-19 5:23 ` Torreno, Alexis Czezar
2026-03-19 7:07 ` Andy Shevchenko
0 siblings, 1 reply; 17+ messages in thread
From: Torreno, Alexis Czezar @ 2026-03-19 5:23 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron,
David Lechner, Sa, Nuno, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
> On Wed, Mar 18, 2026 at 01:13:36PM +0800, Alexis Czezar Torreno wrote:
> > Add support for the Analog Devices AD5706R, a 4-channel 16-bit current
> > output digital-to-analog converter with SPI interface.
> >
> > Features:
> > - 4 independent DAC channels
> > - Hardware and software LDAC trigger
> > - Configurable output range
> > - PWM-based LDAC control
> > - Dither and toggle modes
> > - Dynamically configurable SPI speed
>
>
> > ---
> > Changes since v1:
> > - Removed PWM, GPIO, clock generator, debugfs, regmap, IIO_BUFFER
>
> Why was regmap removed?! Was it not used?
As far as I understand it, regmap also gives access to debugfs. When I removed
debugfs I also added regmap as removed.
For the spi write/read I am not using regmap as the device has some features
that I think regmap_read/write couldn't support. Namely the variable data width,
as the device only accepts exact amount of clock cycles. Future patches will also add
variable SPI speed.
>
> > - Removed all custom ext_info sysfs attributes
> > - Simplified to basic raw read/write and read-only scale
> > - SPI read/write can handle multibyte registers
>
> ...
>
> > +#include <linux/array_size.h>
> > +#include <linux/bits.h>
> > +#include <linux/cleanup.h>
>
> + errno.h
>
> > +#include <linux/iio/iio.h>
>
> + mod_devicetable.h
>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
> > +#include <linux/spi/spi.h>
>
> + types.h
>
> > +#include <linux/unaligned.h>
>
> Follow IWYU principle.
I did miss errno.h but it seems the IWYU is stricter than pragmatic.
Will adhere to it better.
>
> ...
>
> > +static int ad5706r_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int *val,
> > + int *val2, long mask)
> > +{
> > + struct ad5706r_state *st = iio_priv(indio_dev);
> > + u16 reg_val;
> > + int ret;
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + scoped_guard(mutex, &st->lock) {
>
>
> Can't it be simply guard()() ?
Can be, yes, will edit.
>
> > + ret = ad5706r_spi_read(st,
> > +AD5706R_REG_DAC_DATA_READBACK_CH(chan->channel),
>
> It's too long line.
Will simplify this and other similar lines
>
...
>
> > +static int ad5706r_write_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *chan, int val,
> > + int val2, long mask)
> > +{
> > + struct ad5706r_state *st = iio_priv(indio_dev);
> > +
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + if (val < 0 || val >= AD5706R_DAC_MAX_CODE)
>
> in_range()?
>
> (will need minmax.h)
>
Will replace, much more readable that way
> > + return -EINVAL;
> > +
> > + guard(mutex)(&st->lock);
> > + return ad5706r_spi_write(st,
> > +
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-19 5:23 ` Torreno, Alexis Czezar
@ 2026-03-19 7:07 ` Andy Shevchenko
2026-03-25 1:07 ` Torreno, Alexis Czezar
0 siblings, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2026-03-19 7:07 UTC (permalink / raw)
To: Torreno, Alexis Czezar
Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron,
David Lechner, Sa, Nuno, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Thu, Mar 19, 2026 at 05:23:07AM +0000, Torreno, Alexis Czezar wrote:
> > On Wed, Mar 18, 2026 at 01:13:36PM +0800, Alexis Czezar Torreno wrote:
...
> > > Changes since v1:
> > > - Removed PWM, GPIO, clock generator, debugfs, regmap, IIO_BUFFER
> >
> > Why was regmap removed?! Was it not used?
>
> As far as I understand it, regmap also gives access to debugfs. When I removed
> debugfs I also added regmap as removed.
Not only debugfs, and it's unrelated to the any custom debugfs interfaces in
the driver, it's just a feature out-of-the-box of regmap.
> For the spi write/read I am not using regmap as the device has some features
> that I think regmap_read/write couldn't support. Namely the variable data width,
> as the device only accepts exact amount of clock cycles. Future patches will also add
> variable SPI speed.
We have a lot of flexibility in regmap core. Do you think it can be improved /
extended to cover the cases like yours?
> > > - Removed all custom ext_info sysfs attributes
> > > - Simplified to basic raw read/write and read-only scale
> > > - SPI read/write can handle multibyte registers
...
> > > +#include <linux/array_size.h>
> > > +#include <linux/bits.h>
> > > +#include <linux/cleanup.h>
> >
> > + errno.h
> >
> > > +#include <linux/iio/iio.h>
> >
> > + mod_devicetable.h
> >
> > > +#include <linux/module.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/spi/spi.h>
> >
> > + types.h
> >
> > > +#include <linux/unaligned.h>
> >
> > Follow IWYU principle.
>
> I did miss errno.h
You missed more as I showed above.
> but it seems the IWYU is stricter than pragmatic.
No, it's not true. The required headers is exactly very pragmatic in order to
untangle the dependency hell we have in the kernel. You can browse the lore
archive for that keyword and find tons of problems people reported in the past
10+ years with that.
> Will adhere to it better.
Please, do.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-19 7:07 ` Andy Shevchenko
@ 2026-03-25 1:07 ` Torreno, Alexis Czezar
2026-03-25 10:13 ` Nuno Sá
2026-03-25 12:12 ` Andy Shevchenko
0 siblings, 2 replies; 17+ messages in thread
From: Torreno, Alexis Czezar @ 2026-03-25 1:07 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron,
David Lechner, Sa, Nuno, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
> > > > Changes since v1:
> > > > - Removed PWM, GPIO, clock generator, debugfs, regmap,
> > > > IIO_BUFFER
> > >
> > > Why was regmap removed?! Was it not used?
> >
> > As far as I understand it, regmap also gives access to debugfs. When I
> > removed debugfs I also added regmap as removed.
>
> Not only debugfs, and it's unrelated to the any custom debugfs interfaces in
> the driver, it's just a feature out-of-the-box of regmap.
>
> > For the spi write/read I am not using regmap as the device has some
> > features that I think regmap_read/write couldn't support. Namely the
> > variable data width, as the device only accepts exact amount of clock
> > cycles. Future patches will also add variable SPI speed.
>
> We have a lot of flexibility in regmap core. Do you think it can be improved /
> extended to cover the cases like yours?
>
To neatly summarize, my needs are: (in future patches)
1. SPI read/write can have different frequencies and runtime changeable
2. SPI data bits needs to be exactly 8bits or 16bits depending on register width
3. DAC Device reads SPI command bits [14:12] for communication, not just chip select
For regmap to be used
1. regmap_config would need new read_speed and write_speed entries.
2. val_bits must now be changeable depending on the need.
3. I think the read/write_flag_mask can do this.
1) is relatively easy I think, but am not sure with 2) as it might break other regmap core code
that already assumes it to be fixed.
Feels like a lot of work for a niche amount of devices, I may still lean on the opinion of
keeping regmap as is.
Regards,
Alexis
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-25 1:07 ` Torreno, Alexis Czezar
@ 2026-03-25 10:13 ` Nuno Sá
2026-03-25 12:12 ` Andy Shevchenko
1 sibling, 0 replies; 17+ messages in thread
From: Nuno Sá @ 2026-03-25 10:13 UTC (permalink / raw)
To: Torreno, Alexis Czezar, Andy Shevchenko
Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron,
David Lechner, Sa, Nuno, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Wed, 2026-03-25 at 01:07 +0000, Torreno, Alexis Czezar wrote:
> > > > > Changes since v1:
> > > > > - Removed PWM, GPIO, clock generator, debugfs, regmap,
> > > > > IIO_BUFFER
> > > >
> > > > Why was regmap removed?! Was it not used?
> > >
> > > As far as I understand it, regmap also gives access to debugfs. When I
> > > removed debugfs I also added regmap as removed.
> >
> > Not only debugfs, and it's unrelated to the any custom debugfs interfaces in
> > the driver, it's just a feature out-of-the-box of regmap.
> >
> > > For the spi write/read I am not using regmap as the device has some
> > > features that I think regmap_read/write couldn't support. Namely the
> > > variable data width, as the device only accepts exact amount of clock
> > > cycles. Future patches will also add variable SPI speed.
> >
> > We have a lot of flexibility in regmap core. Do you think it can be improved /
> > extended to cover the cases like yours?
> >
>
> To neatly summarize, my needs are: (in future patches)
> 1. SPI read/write can have different frequencies and runtime changeable
> 2. SPI data bits needs to be exactly 8bits or 16bits depending on register width
> 3. DAC Device reads SPI command bits [14:12] for communication, not just chip select
>
> For regmap to be used
> 1. regmap_config would need new read_speed and write_speed entries.
> 2. val_bits must now be changeable depending on the need.
> 3. I think the read/write_flag_mask can do this.
>
Do not forget you can just implement your custom regmap_bus which also opens new "doors".
- Nuno Sá
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-25 1:07 ` Torreno, Alexis Czezar
2026-03-25 10:13 ` Nuno Sá
@ 2026-03-25 12:12 ` Andy Shevchenko
2026-03-27 9:01 ` Torreno, Alexis Czezar
1 sibling, 1 reply; 17+ messages in thread
From: Andy Shevchenko @ 2026-03-25 12:12 UTC (permalink / raw)
To: Torreno, Alexis Czezar
Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron,
David Lechner, Sa, Nuno, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
On Wed, Mar 25, 2026 at 01:07:44AM +0000, Torreno, Alexis Czezar wrote:
> > > > > Changes since v1:
> > > > > - Removed PWM, GPIO, clock generator, debugfs, regmap,
> > > > > IIO_BUFFER
> > > >
> > > > Why was regmap removed?! Was it not used?
> > >
> > > As far as I understand it, regmap also gives access to debugfs. When I
> > > removed debugfs I also added regmap as removed.
> >
> > Not only debugfs, and it's unrelated to the any custom debugfs interfaces in
> > the driver, it's just a feature out-of-the-box of regmap.
> >
> > > For the spi write/read I am not using regmap as the device has some
> > > features that I think regmap_read/write couldn't support. Namely the
> > > variable data width, as the device only accepts exact amount of clock
> > > cycles. Future patches will also add variable SPI speed.
> >
> > We have a lot of flexibility in regmap core. Do you think it can be improved /
> > extended to cover the cases like yours?
>
> To neatly summarize, my needs are: (in future patches)
> 1. SPI read/write can have different frequencies and runtime changeable
How does it related to regmap? Is it dependent on the register?
> 2. SPI data bits needs to be exactly 8bits or 16bits depending on register width
This is solved very easily with regmap, no problem at all (two regmaps with
configuration for 8-bit and 16-bit registers), I believe we have even driver
in kernel that does exactly this.
> 3. DAC Device reads SPI command bits [14:12] for communication, not just chip select
Okay, but I'm not sure how this is a limitation...
> For regmap to be used
> 1. regmap_config would need new read_speed and write_speed entries.
> 2. val_bits must now be changeable depending on the need.
> 3. I think the read/write_flag_mask can do this.
>
> 1) is relatively easy I think, but am not sure with 2) as it might break other regmap core code
> that already assumes it to be fixed.
> Feels like a lot of work for a niche amount of devices, I may still lean on the opinion of
> keeping regmap as is.
Okay, I leave it to others, for the simplicity we can leave driver as is, but
make sure you put the summary of this into the cover letter, so we will be
crystal clear why regmap hasn't been chosen.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-25 12:12 ` Andy Shevchenko
@ 2026-03-27 9:01 ` Torreno, Alexis Czezar
0 siblings, 0 replies; 17+ messages in thread
From: Torreno, Alexis Czezar @ 2026-03-27 9:01 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lars-Peter Clausen, Hennerich, Michael, Jonathan Cameron,
David Lechner, Sa, Nuno, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-iio@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org
> >
> > To neatly summarize, my needs are: (in future patches) 1. SPI
> > read/write can have different frequencies and runtime changeable
>
> How does it related to regmap? Is it dependent on the register?
>
> > 2. SPI data bits needs to be exactly 8bits or 16bits depending on
> > register width
>
> This is solved very easily with regmap, no problem at all (two regmaps with
> configuration for 8-bit and 16-bit registers), I believe we have even driver in
> kernel that does exactly this.
>
> > 3. DAC Device reads SPI command bits [14:12] for communication, not
> > just chip select
>
> Okay, but I'm not sure how this is a limitation...
>
> > For regmap to be used
> > 1. regmap_config would need new read_speed and write_speed entries.
> > 2. val_bits must now be changeable depending on the need.
> > 3. I think the read/write_flag_mask can do this.
> >
> > 1) is relatively easy I think, but am not sure with 2) as it might
> > break other regmap core code that already assumes it to be fixed.
> > Feels like a lot of work for a niche amount of devices, I may still
> > lean on the opinion of keeping regmap as is.
>
> Okay, I leave it to others, for the simplicity we can leave driver as is, but make
> sure you put the summary of this into the cover letter, so we will be crystal
> clear why regmap hasn't been chosen.
>
With some discussion with Nuno, I get now that I could still use regmap but employ
a custom regmap_bus for my use case. Will do this on v4, I think I can still
satisfy my future requirements with it. Thanks!
Regards,
Alexis
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-18 5:13 ` [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC Alexis Czezar Torreno
2026-03-18 8:16 ` Andy Shevchenko
@ 2026-03-21 18:39 ` Jonathan Cameron
2026-03-25 1:22 ` Torreno, Alexis Czezar
2026-03-25 11:02 ` Nuno Sá
2 siblings, 1 reply; 17+ messages in thread
From: Jonathan Cameron @ 2026-03-21 18:39 UTC (permalink / raw)
To: Alexis Czezar Torreno
Cc: Lars-Peter Clausen, Michael Hennerich, David Lechner,
Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-iio, devicetree, linux-kernel
On Wed, 18 Mar 2026 13:13:36 +0800
Alexis Czezar Torreno <alexisczezar.torreno@analog.com> wrote:
> Add support for the Analog Devices AD5706R, a 4-channel 16-bit
> current output digital-to-analog converter with SPI interface.
>
> Features:
> - 4 independent DAC channels
> - Hardware and software LDAC trigger
> - Configurable output range
> - PWM-based LDAC control
> - Dither and toggle modes
> - Dynamically configurable SPI speed
>
> Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
>
> ---
> Changes since v1:
> - Removed PWM, GPIO, clock generator, debugfs, regmap, IIO_BUFFER
> - Removed all custom ext_info sysfs attributes
> - Simplified to basic raw read/write and read-only scale
> - SPI read/write can handle multibyte registers
Hi Alexis,
I've tried not to overlap too much with other feedback.
Some comments inline.
Thanks,
Jonathan
> diff --git a/drivers/iio/dac/ad5706r.c b/drivers/iio/dac/ad5706r.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..840ee7b6db2e05ea35b27ff776f0c5c8a961d9c1
> --- /dev/null
> +++ b/drivers/iio/dac/ad5706r.c
> +
> +struct ad5706r_state {
> + struct spi_device *spi;
> + struct mutex lock; /* Protects SPI transfers */
I assume it actually protects the buffers below. Where possible talk
about what 'data' is being protected. It's easier to reason about than
transfers which will be protected anyway by internal bus driver locking.
> +
> + u8 tx_buf[4] __aligned(ARCH_DMA_MINALIGN);
> + u8 rx_buf[2];
> +};
> +
> +static int ad5706r_reg_len(unsigned int reg)
> +{
> + if (reg >= AD5706R_MULTIBYTE_REG_START && reg <= AD5706R_MULTIBYTE_REG_END)
> + return AD5706R_DOUBLE_BYTE_LEN;
> +
> + return AD5706R_SINGLE_BYTE_LEN;
> +}
> +
> +static int ad5706r_spi_write(struct ad5706r_state *st, u16 reg, u16 val)
> +{
> + unsigned int num_bytes = ad5706r_reg_len(reg);
> + struct spi_transfer xfer = {
> + .tx_buf = st->tx_buf,
> + .len = num_bytes + 2,
> + };
> +
> + put_unaligned_be16(reg, &st->tx_buf[0]);
> +
> + if (num_bytes == 1)
> + st->tx_buf[2] = val;
> + else if (num_bytes == 2)
> + put_unaligned_be16(val, &st->tx_buf[2]);
> + else
> + return -EINVAL;
> +
> + return spi_sync_transfer(st->spi, &xfer, 1);
Even though you are writing only, consider using spi_write_then_read() here
as well. Might end up simpler and it works fine with an rx size of 0 and
NULL buffer for rx
> +}
> +
> +static int ad5706r_spi_read(struct ad5706r_state *st, u16 reg, u16 *val)
> +{
> + unsigned int num_bytes = ad5706r_reg_len(reg);
> + u16 cmd;
> + int ret;
> +
> + struct spi_transfer xfer[] = {
> + {
> + .tx_buf = st->tx_buf,
> + .len = 2,
> + },
> + {
> + .rx_buf = st->rx_buf,
> + .len = num_bytes,
> + },
> + };
> +
> + cmd = AD5706R_RD_MASK | (reg & AD5706R_ADDR_MASK);
> + put_unaligned_be16(cmd, &st->tx_buf[0]);
> +
> + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
Can you use spi_write_the_read()?
Has the added advantage that it bounces the data so doesn't need DMA safe.
Can also use more meaningful types like
__be16 tx;
__be16 rx16;
u8 rx8;
> + if (ret)
> + return ret;
> +
> + if (num_bytes == 1)
> + *val = st->rx_buf[0];
> + else if (num_bytes == 2)
> + *val = get_unaligned_be16(st->rx_buf);
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int ad5706r_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct ad5706r_state *st = iio_priv(indio_dev);
> + u16 reg_val;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + scoped_guard(mutex, &st->lock) {
Why?
case IIO_CHAN_INFO_RAW: {
guard(mutex)(&st->lock);
ret = ....
return IIO_VAL_INT;
}
is easier to read and reduces the huge indent.
Only use scoped_guard() when you can't do it in a more readable fashion.
> + ret = ad5706r_spi_read(st, AD5706R_REG_DAC_DATA_READBACK_CH(chan->channel),
> + ®_val);
> +
> + if (ret)
> + return ret;
> +
> + *val = reg_val;
> + }
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = 50;
> + *val2 = AD5706R_DAC_RESOLUTION;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + }
> +
I'd prefer an explicit
defualt:
return -EINVAL;
}
to end the switch statement and make it clear within the switch that all other options
are errors.
> + return -EINVAL;
> +}
> +
> +static int ad5706r_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct ad5706r_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val < 0 || val >= AD5706R_DAC_MAX_CODE)
> + return -EINVAL;
> +
> + guard(mutex)(&st->lock);
What's the scope? add {} to the case block to make it clearly defined.
> + return ad5706r_spi_write(st,
> + AD5706R_REG_DAC_INPUT_A_CH(chan->channel),
> + val);
> + default:
> + return -EINVAL;
> + }
> +}
> +static const struct of_device_id ad5706r_of_match[] = {
> + { .compatible = "adi,ad5706r" },
> + {}
As below.
> +};
> +MODULE_DEVICE_TABLE(of, ad5706r_of_match);
> +
> +static const struct spi_device_id ad5706r_id[] = {
> + { "ad5706r" },
> + {}
Trivial style thing, but we've standardized on
{ }
for IIO. Had to pick one of the two choices and that one looked
nicer to me ;)
> +};
> +MODULE_DEVICE_TABLE(spi, ad5706r_id);
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-21 18:39 ` Jonathan Cameron
@ 2026-03-25 1:22 ` Torreno, Alexis Czezar
2026-03-25 3:13 ` Torreno, Alexis Czezar
0 siblings, 1 reply; 17+ messages in thread
From: Torreno, Alexis Czezar @ 2026-03-25 1:22 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Hennerich, Michael, David Lechner, Sa, Nuno,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
> > +
> > +struct ad5706r_state {
> > + struct spi_device *spi;
> > + struct mutex lock; /* Protects SPI transfers */
> I assume it actually protects the buffers below. Where possible talk about what
> 'data' is being protected. It's easier to reason about than transfers which will be
> protected anyway by internal bus driver locking.
Ah yes, I'll edit the comment to something like
struct mutex lock; /* Protects tx_buf and rx_buf from concurrent access */
>
> > +
> > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
>
> Can you use spi_write_the_read()?
> Has the added advantage that it bounces the data so doesn't need DMA safe.
> Can also use more meaningful types like
> __be16 tx;
> __be16 rx16;
> u8 rx8;
Ok, will shift to spi_write_the_read(). There was another place to use this above,
just trimmed it off here. Noted on the more meaningful variables.
...
>
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + scoped_guard(mutex, &st->lock) {
> Why?
>
> case IIO_CHAN_INFO_RAW: {
> guard(mutex)(&st->lock);
>
> ret = ....
>
> return IIO_VAL_INT;
> }
>
> is easier to read and reduces the huge indent.
>
> Only use scoped_guard() when you can't do it in a more readable fashion.
Will edit to simpler guard()
>
...
> > + }
> > +
> I'd prefer an explicit
> defualt:
> return -EINVAL;
> }
> to end the switch statement and make it clear within the switch that all other
> options
> are errors.
Wil add the default
> > + switch (mask) {
> > + case IIO_CHAN_INFO_RAW:
> > + if (val < 0 || val >= AD5706R_DAC_MAX_CODE)
> > + return -EINVAL;
> > +
> > + guard(mutex)(&st->lock);
>
> What's the scope? add {} to the case block to make it clearly defined.
Just the raw, I'll add the {}
> > +static const struct of_device_id ad5706r_of_match[] = {
> > + { .compatible = "adi,ad5706r" },
> > + {}
>
> As below.
>
> > +};
> > +MODULE_DEVICE_TABLE(of, ad5706r_of_match);
> > +
> > +static const struct spi_device_id ad5706r_id[] = {
> > + { "ad5706r" },
> > + {}
>
> Trivial style thing, but we've standardized on
> { }
> for IIO. Had to pick one of the two choices and that one looked
> nicer to me ;)
>
Nice, I did wonder if either style was ok as long as consistent, noted on the {}
Regards,
Alexis
^ permalink raw reply [flat|nested] 17+ messages in thread* RE: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-25 1:22 ` Torreno, Alexis Czezar
@ 2026-03-25 3:13 ` Torreno, Alexis Czezar
2026-03-25 10:14 ` Nuno Sá
0 siblings, 1 reply; 17+ messages in thread
From: Torreno, Alexis Czezar @ 2026-03-25 3:13 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Lars-Peter Clausen, Hennerich, Michael, David Lechner, Sa, Nuno,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
> > > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> >
> > Can you use spi_write_the_read()?
> > Has the added advantage that it bounces the data so doesn't need DMA safe.
> > Can also use more meaningful types like
> > __be16 tx;
> > __be16 rx16;
> > u8 rx8;
>
> Ok, will shift to spi_write_the_read(). There was another place to use this
> above, just trimmed it off here. Noted on the more meaningful variables.
>
I only just checked spi_write_then_read() but, it seems this doesn't support
variable spi_speed_frequency for my future patches. Should I still use it for now
and revert in the future?
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-25 3:13 ` Torreno, Alexis Czezar
@ 2026-03-25 10:14 ` Nuno Sá
0 siblings, 0 replies; 17+ messages in thread
From: Nuno Sá @ 2026-03-25 10:14 UTC (permalink / raw)
To: Torreno, Alexis Czezar, Jonathan Cameron
Cc: Lars-Peter Clausen, Hennerich, Michael, David Lechner, Sa, Nuno,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
linux-iio@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
On Wed, 2026-03-25 at 03:13 +0000, Torreno, Alexis Czezar wrote:
> > > > + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> > >
> > > Can you use spi_write_the_read()?
> > > Has the added advantage that it bounces the data so doesn't need DMA safe.
> > > Can also use more meaningful types like
> > > __be16 tx;
> > > __be16 rx16;
> > > u8 rx8;
> >
> > Ok, will shift to spi_write_the_read(). There was another place to use this
> > above, just trimmed it off here. Noted on the more meaningful variables.
> >
>
> I only just checked spi_write_then_read() but, it seems this doesn't support
> variable spi_speed_frequency for my future patches. Should I still use it for now
> and revert in the future?
There's still the open regmap discussion. Independent of that, if you can't use
spi_write_then_read(), you need to make sure the buffers you pass spi are DMA safe.
- Nuno Sá
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC
2026-03-18 5:13 ` [PATCH v3 2/2] iio: dac: ad5706r: Add support for AD5706R DAC Alexis Czezar Torreno
2026-03-18 8:16 ` Andy Shevchenko
2026-03-21 18:39 ` Jonathan Cameron
@ 2026-03-25 11:02 ` Nuno Sá
2 siblings, 0 replies; 17+ messages in thread
From: Nuno Sá @ 2026-03-25 11:02 UTC (permalink / raw)
To: Alexis Czezar Torreno, Lars-Peter Clausen, Michael Hennerich,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-iio, devicetree, linux-kernel
On Wed, 2026-03-18 at 13:13 +0800, Alexis Czezar Torreno wrote:
> Add support for the Analog Devices AD5706R, a 4-channel 16-bit
> current output digital-to-analog converter with SPI interface.
>
> Features:
> - 4 independent DAC channels
> - Hardware and software LDAC trigger
> - Configurable output range
> - PWM-based LDAC control
> - Dither and toggle modes
> - Dynamically configurable SPI speed
>
> Signed-off-by: Alexis Czezar Torreno <alexisczezar.torreno@analog.com>
>
> ---
Not sure if everyone is aware of this but AI review in:
https://sashiko.dev/#/patchset/20260318-dev_ad5706r-v3-0-5d078f41e988%40analog.com
The DMA comment is an interesting one. Likely it needs improvement in the review-prompt repo given
it's one of those case we know what we're doing (at least we think we know :)). Still has a point
in that IIO_DMA_MINALIGN should be used.
At some point we need to add some IIO specific bits :)
- Nuno Sá
> Changes since v1:
> - Removed PWM, GPIO, clock generator, debugfs, regmap, IIO_BUFFER
> - Removed all custom ext_info sysfs attributes
> - Simplified to basic raw read/write and read-only scale
> - SPI read/write can handle multibyte registers
> ---
> ---
> MAINTAINERS | 1 +
> drivers/iio/dac/Kconfig | 10 +++
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/ad5706r.c | 222 ++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 234 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 17a3d2d45fccb9cd3c93fd35666fb85d17d53cde..3d7bd98b4d1b55836e40687a9a3ac9f4935a8acb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1502,6 +1502,7 @@ L: linux-iio@vger.kernel.org
> S: Supported
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/iio/dac/adi,ad5706r.yaml
> +F: drivers/iio/dac/ad5706r.c
>
> ANALOG DEVICES INC AD7091R DRIVER
> M: Marcelo Schmitt <marcelo.schmitt@analog.com>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index db9f5c711b3df90641f017652fbbef594cc1627d..8ccbdf6dfbca8640a47bf05b4afc6b4bf90a7e26 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -178,6 +178,16 @@ config AD5624R_SPI
> Say yes here to build support for Analog Devices AD5624R, AD5644R and
> AD5664R converters (DAC). This driver uses the common SPI interface.
>
> +config AD5706R
> + tristate "Analog Devices AD5706R DAC driver"
> + depends on SPI
> + help
> + Say yes here to build support for Analog Devices AD5706R 4-channel,
> + 16-bit current output DAC.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called ad5706r.
> +
> config AD9739A
> tristate "Analog Devices AD9739A RF DAC spi driver"
> depends on SPI
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 2a80bbf4e80ad557da79ed916027cedff286984b..0034317984985035f7987a744899924bfd4612e3 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -21,6 +21,7 @@ obj-$(CONFIG_AD5449) += ad5449.o
> obj-$(CONFIG_AD5592R_BASE) += ad5592r-base.o
> obj-$(CONFIG_AD5592R) += ad5592r.o
> obj-$(CONFIG_AD5593R) += ad5593r.o
> +obj-$(CONFIG_AD5706R) += ad5706r.o
> obj-$(CONFIG_AD5755) += ad5755.o
> obj-$(CONFIG_AD5758) += ad5758.o
> obj-$(CONFIG_AD5761) += ad5761.o
> diff --git a/drivers/iio/dac/ad5706r.c b/drivers/iio/dac/ad5706r.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..840ee7b6db2e05ea35b27ff776f0c5c8a961d9c1
> --- /dev/null
> +++ b/drivers/iio/dac/ad5706r.c
> @@ -0,0 +1,222 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AD5706R 16-bit Current Output Digital to Analog Converter
> + *
> + * Copyright 2026 Analog Devices Inc.
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/spi/spi.h>
> +#include <linux/unaligned.h>
> +
> +/* SPI frame layout */
> +#define AD5706R_RD_MASK BIT(15)
> +#define AD5706R_ADDR_MASK GENMASK(11, 0)
> +
> +/* Registers */
> +#define AD5706R_REG_DAC_INPUT_A_CH(x) (0x60 + ((x) * 2))
> +#define AD5706R_REG_DAC_DATA_READBACK_CH(x) (0x68 + ((x) * 2))
> +
> +#define AD5706R_DAC_RESOLUTION 16
> +#define AD5706R_DAC_MAX_CODE BIT(16)
> +#define AD5706R_MULTIBYTE_REG_START 0x14
> +#define AD5706R_MULTIBYTE_REG_END 0x71
> +#define AD5706R_SINGLE_BYTE_LEN 1
> +#define AD5706R_DOUBLE_BYTE_LEN 2
> +
> +struct ad5706r_state {
> + struct spi_device *spi;
> + struct mutex lock; /* Protects SPI transfers */
> +
> + u8 tx_buf[4] __aligned(ARCH_DMA_MINALIGN);
> + u8 rx_buf[2];
> +};
> +
> +static int ad5706r_reg_len(unsigned int reg)
> +{
> + if (reg >= AD5706R_MULTIBYTE_REG_START && reg <= AD5706R_MULTIBYTE_REG_END)
> + return AD5706R_DOUBLE_BYTE_LEN;
> +
> + return AD5706R_SINGLE_BYTE_LEN;
> +}
> +
> +static int ad5706r_spi_write(struct ad5706r_state *st, u16 reg, u16 val)
> +{
> + unsigned int num_bytes = ad5706r_reg_len(reg);
> + struct spi_transfer xfer = {
> + .tx_buf = st->tx_buf,
> + .len = num_bytes + 2,
> + };
> +
> + put_unaligned_be16(reg, &st->tx_buf[0]);
> +
> + if (num_bytes == 1)
> + st->tx_buf[2] = val;
> + else if (num_bytes == 2)
> + put_unaligned_be16(val, &st->tx_buf[2]);
> + else
> + return -EINVAL;
> +
> + return spi_sync_transfer(st->spi, &xfer, 1);
> +}
> +
> +static int ad5706r_spi_read(struct ad5706r_state *st, u16 reg, u16 *val)
> +{
> + unsigned int num_bytes = ad5706r_reg_len(reg);
> + u16 cmd;
> + int ret;
> +
> + struct spi_transfer xfer[] = {
> + {
> + .tx_buf = st->tx_buf,
> + .len = 2,
> + },
> + {
> + .rx_buf = st->rx_buf,
> + .len = num_bytes,
> + },
> + };
> +
> + cmd = AD5706R_RD_MASK | (reg & AD5706R_ADDR_MASK);
> + put_unaligned_be16(cmd, &st->tx_buf[0]);
> +
> + ret = spi_sync_transfer(st->spi, xfer, ARRAY_SIZE(xfer));
> + if (ret)
> + return ret;
> +
> + if (num_bytes == 1)
> + *val = st->rx_buf[0];
> + else if (num_bytes == 2)
> + *val = get_unaligned_be16(st->rx_buf);
> + else
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int ad5706r_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct ad5706r_state *st = iio_priv(indio_dev);
> + u16 reg_val;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + scoped_guard(mutex, &st->lock) {
> + ret = ad5706r_spi_read(st, AD5706R_REG_DAC_DATA_READBACK_CH(chan-
> >channel),
> + ®_val);
> +
> + if (ret)
> + return ret;
> +
> + *val = reg_val;
> + }
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + *val = 50;
> + *val2 = AD5706R_DAC_RESOLUTION;
> + return IIO_VAL_FRACTIONAL_LOG2;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int ad5706r_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct ad5706r_state *st = iio_priv(indio_dev);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + if (val < 0 || val >= AD5706R_DAC_MAX_CODE)
> + return -EINVAL;
> +
> + guard(mutex)(&st->lock);
> + return ad5706r_spi_write(st,
> + AD5706R_REG_DAC_INPUT_A_CH(chan->channel),
> + val);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info ad5706r_info = {
> + .read_raw = ad5706r_read_raw,
> + .write_raw = ad5706r_write_raw,
> +};
> +
> +#define AD5706R_CHAN(_channel) { \
> + .type = IIO_CURRENT, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .output = 1, \
> + .indexed = 1, \
> + .channel = _channel, \
> +}
> +
> +static const struct iio_chan_spec ad5706r_channels[] = {
> + AD5706R_CHAN(0),
> + AD5706R_CHAN(1),
> + AD5706R_CHAN(2),
> + AD5706R_CHAN(3),
> +};
> +
> +static int ad5706r_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad5706r_state *st;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + st->spi = spi;
> +
> + ret = devm_mutex_init(&spi->dev, &st->lock);
> + if (ret)
> + return ret;
> +
> + indio_dev->name = "ad5706r";
> + indio_dev->info = &ad5706r_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = ad5706r_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ad5706r_channels);
> +
> + return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct of_device_id ad5706r_of_match[] = {
> + { .compatible = "adi,ad5706r" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ad5706r_of_match);
> +
> +static const struct spi_device_id ad5706r_id[] = {
> + { "ad5706r" },
> + {}
> +};
> +MODULE_DEVICE_TABLE(spi, ad5706r_id);
> +
> +static struct spi_driver ad5706r_driver = {
> + .driver = {
> + .name = "ad5706r",
> + .of_match_table = ad5706r_of_match,
> + },
> + .probe = ad5706r_probe,
> + .id_table = ad5706r_id,
> +};
> +module_spi_driver(ad5706r_driver);
> +
> +MODULE_AUTHOR("Alexis Czezar Torreno <alexisczezar.torreno@analog.com>");
> +MODULE_DESCRIPTION("AD5706R 16-bit Current Output DAC driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 17+ messages in thread