Re: [PATCH 1/2] iio: dac: ad5337: Add driver for the AD5337 DAC

[Joshua Crofts <joshua.crofts1@gmail.com>]

On Fri, 12 Jun 2026 15:31:21 +0200
Marcin Bis <marcin@bis-linux.com> wrote:

> Add driver for the AD5337 dual 8-bit I2C voltage-output DAC.
> This part uses the pointer-byte protocol and is not compatible with
> the AD5337R nanoDAC+ driver (adi,ad5337r).
> 
> Signed-off-by: Marcin Bis <marcin@bis-linux.com>
> ---
>  MAINTAINERS              |   5 ++
>  drivers/iio/dac/Kconfig  |  12 ++-
>  drivers/iio/dac/Makefile |   1 +
>  drivers/iio/dac/ad5337.c | 159 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 176 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/iio/dac/ad5337.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e035a3be797c..614589b6efa4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -438,6 +438,11 @@ W:	http://wiki.analog.com/AD5398
>  W:	https://ez.analog.com/linux-software-drivers
>  F:	drivers/regulator/ad5398.c
>  
> +AD5337  ANALOG DEVICES INC AD5337 DAC DRIVER
> +M:	Marcin Bis <marcin@bis-linux.com>
> +S:	Maintained
> +F:	drivers/iio/dac/ad5337.c

Once you reorder your patches, this should go into the (first)
dt-binding patch.

> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Analog Devices AD5337 I2C DAC driver
> + *
> + * Copyright 2017 Marcin Bis <marcin@bis-linux.com>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regulator/consumer.h>

You're definitely missing array_size.h, types.h, mod_devicetable.h
and err.h.

> +#include <linux/iio/iio.h>
> +
> +#define AD5337_NUM_CHANNELS	2
> +#define AD5337_RESOLUTION	8
> +/*
> + * AD5338 - 10bit, AD5339 - 12bit
> + */

This comment can just be on one line.

> +
> +#define AD5337_PTR_DAC_A	0x01
> +#define AD5337_PTR_DAC_B	0x02
> +
> +struct ad5337_state {
> +	struct i2c_client *client;
> +	struct mutex lock;

Add a comment on why you need a mutex.

> +	unsigned int vref_mv;
> +	u8 cache[AD5337_NUM_CHANNELS];
> +};
> +
> +static int ad5337_write_dac(struct ad5337_state *st, int channel, u8 value)
> +{
> +	u8 buf[3];
> +	int ret;
> +
> +	if (channel)
> +		buf[0] = AD5337_PTR_DAC_B;
> +	else
> +		buf[0] = AD5337_PTR_DAC_A;
> +
> +	/* PD1=0, PD0=0, CLR=1, LDAC=0, D[7:4] */
> +	buf[1] = 0x20 | (value >> 4);
> +	buf[2] = (value & 0x0f) << 4;

You should definitely use FIELD_GET/FIELD_PREP macros from the 
<linux/bitfield.h> header, it improves readability by a lot!

Also, use macros instead of just defining magic numbers like this.

> +
> +	ret = i2c_master_send(st->client, buf, 3);

Use sizeof(buf) instead of just hardcoding the size.

> +
> +	return (ret < 0) ? ret : (ret == 3 ? 0 : -EIO);

I'd definitely abandon using a terniary operator and just break it into
if statements, on first glance this is really hard to read.

> +}
> +
> +static int ad5337_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ad5337_state *st = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = st->cache[chan->channel];
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = st->vref_mv;
> +		*val2 = AD5337_RESOLUTION;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int ad5337_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ad5337_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (mask != IIO_CHAN_INFO_RAW)
> +		return -EINVAL;
> +
> +	if (val < 0 || val > ((1 << AD5337_RESOLUTION) - 1))
> +		return -EINVAL;
> +
> +	mutex_lock(&st->lock);

Consider using the guard(mutex)() macro from cleanup.h, it saves lines
and prevents future developers from accidentally forgetting to release
locks (don't forget to add the header as well though)!

> +	ret = ad5337_write_dac(st, chan->channel, val);
> +	if (!ret)
> +		st->cache[chan->channel] = val;
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info ad5337_info = {
> +	.read_raw = ad5337_read_raw,
> +	.write_raw = ad5337_write_raw,
> +};
> +
> +#define AD5337_CHANNEL(_chan) {						\

A very tiny nitpick, but you don't need to align your backslashes when defining
a macro.

> +	.type = IIO_VOLTAGE,						\
> +	.indexed = 1,							\
> +	.output = 1,							\
> +	.channel = (_chan),						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),		\
> +}
> +
> +static const struct iio_chan_spec ad5337_channels[] = {
> +	AD5337_CHANNEL(0),
> +	AD5337_CHANNEL(1),
> +};
> +
> +static int ad5337_probe(struct i2c_client *client)
> +{
> +	struct ad5337_state *st;
> +	struct iio_dev *indio_dev;

Very much a nit, but swap these lines.

> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> +		return -ENODEV;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	st->client = client;
> +	mutex_init(&st->lock);

Use devm_mutex_init().

> +
> +	ret = devm_regulator_get_enable_read_voltage(&client->dev, "vcc");
> +	if (ret < 0 && ret != -ENODEV)
> +		return ret;

Return dev_err_probe() here.

> +
> +	st->vref_mv = ret > 0 ? ret / 1000 : 3300;

More magic numbers. If you're converting units, consider using
macros from <linux/units.h>.

-- 
Kind regards

CJD