Re: [PATCH v3 11/11] iio: dac: add mcf54415 DAC

Hi Jonathan,

On Tue, May 26, 2026 at 02:30:50PM +0100, Jonathan Cameron wrote:
> On Fri, 22 May 2026 23:20:39 +0200
> Angelo Dureghello <[email protected]> wrote:
>
> > From: Angelo Dureghello <[email protected]>
> >
> > Add basic version of mcf54415 DAC driver. DAC is embedded in the cpu and
> > DAC configuration registers are mapped in the internal IO address space.
> >
> > The DAC accepts a 12-bit digital signal and creates a monotonic 12-bit
> > analog output varying from DAC_VREFL to DAC_VREFH. The DAC module
> > consists of a conversion unit, an output amplifier, and the associated
> > digital control blocks. Default register values for DAC_VREFL and DAC_VREFH
> > are respectively 0 and 0xfff, left untouched in this initial version.
> >
> > This initial version of the driver is minimalistic, "output raw" only, to
> > be extended in the future. DMA and external sync are disabled, default mode
> > is high speed, default format is right-justified 12bit on 16bit word.
> >
> > Signed-off-by: Angelo Dureghello <[email protected]>
> https://sashiko.dev/#/patchset/20260522-wip-stmark2-dac-v3-0-16be0ad35a67%40baylibre.com
>
> Given there were only a couple of comments I've included them below alongside
> my review.  All minor stuff.
>
> Thanks,
>
> Jonathan
>
> > ---
> > Changes in v2:
> > - remove tests from commit message, moved to patch 0
> > - remove additional blank lines
> > - remove dead code and unused definitions
> > - use regmap
> > - add limit check on raw write
> > - non functional style fixes
> > - add COMPILE_TEST to Kconfig
> > Changes in v3:
> > - add comments where needed
> > - code style changes
> > - remove unneeded variables
> > - use regmap_set_bits where possible
> > - remove macro not needed to define a single channel
> > - set up regmap to big_endian accesses for next patches that will come,
> >   that will adjust ColdFire readx/writex as standard LE (links in 0/x).
> > - add return value check on regmap calls
> > - sashiko: remove unneeded .io_port from regmap init.
> > - sashiko: add select REGMAP_MMIO in Kconfig
>
> Looks like you missed or disagreed with the previous sashiko comment on v2 about
> type passed to regmap_read()
>

Sahiko asks if regmap_read produces a compiler watning.
I don't see any,

> > ---
> >  drivers/iio/dac/Kconfig        |  11 +++
> >  drivers/iio/dac/Makefile       |   1 +
> >  drivers/iio/dac/mcf54415_dac.c | 207 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 219 insertions(+)
> >
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > index cd4870b65415..b1a578076188 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -516,6 +516,17 @@ config MAX5821
> >  	  Say yes here to build support for Maxim MAX5821
> >  	  10 bits DAC.
> >
> > +config MCF54415_DAC
> > +	tristate "NXP MCF54415 DAC driver"
> > +	depends on M5441x || COMPILE_TEST
> > +	select REGMAP_MMIO
> > +	help
> > +	  Say yes here to build support for NXP MCF54415
> > +	  12bit DAC.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called mcf54415_dac.
> > +
>
> > diff --git a/drivers/iio/dac/mcf54415_dac.c b/drivers/iio/dac/mcf54415_dac.c
> > new file mode 100644
> > index 000000000000..c8c87572d43d
> > --- /dev/null
> > +++ b/drivers/iio/dac/mcf54415_dac.c
> > @@ -0,0 +1,207 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * NXP mcf54415 DAC driver
> > + *
> > + * Copyright 2026 BayLibre - [email protected]
> > + */
> > +
> > +#include <linux/array_size.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/bits.h>
> > +#include <linux/clk.h>
> > +#include <linux/compiler_types.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/mutex.h>
>
> not seeing any use of this. I guess it 'evolved' away.
> Anyhow, please sanity check these all one more time for v4.
>

thanks,
removed it and also linux/array_size.h that's unused now.

> > +#include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/iio/iio.h>
> > +
> > +#define MCF54415_DAC_CR			0x00
> > +#define MCF54415_DAC_CR_PDN		BIT(0)
> > +#define MCF54415_DAC_CR_HSLS		BIT(6)
> > +#define MCF54415_DAC_CR_WMLVL		GENMASK(9, 8)
> > +#define MCF54415_DAC_CR_FILT		BIT(12)
> > +
> > +#define MCF54415_DAC_DATA		0x02
> > +
> > +struct mcf54415_dac {
> > +	struct regmap *map;
> > +	struct clk *clk;
> > +};
> > +
> > +static const struct regmap_config mcf54415_dac_regmap_config = {
> > +	.reg_bits = 16,
> > +	.reg_stride = 2,
> > +	.val_bits = 16,
> > +	.max_register = 0x0c, /* DACX_FILTCNT,  R.M. Table 30-2 */
> > +	.val_format_endian = REGMAP_ENDIAN_BIG,
> > +	.reg_format_endian = REGMAP_ENDIAN_BIG,
> > +};
> > +
> > +static int mcf54415_dac_init(struct mcf54415_dac *info)
> > +{
> > +	int ret;
> > +
> > +	/* Keeping defaults and enable DAC (bit 0 set to 0) */
> > +	ret = regmap_write(info->map, MCF54415_DAC_CR, MCF54415_DAC_CR_FILT |
> > +			   FIELD_PREP(MCF54415_DAC_CR_WMLVL, 1));
>
> Perhaps use a local variable. It is a tiny bit too easy to miss that
> parameter being split over two lines.
>
> 	u16 val = MCF54415_DAC_CR_FILT | FIELD_PREP(MCF54415_DAC_CR_WMLVL, 1);
>
> would avoid that.

ack, done.

>
> Alternatively perhaps just reflowing as:
> 	ret = regmap_write(info->map, MCF54415_DAC_CR,
> 			   MCF54415_DAC_CR_FILT |
> 			   FIELD_PREP(MCF54415_DAC_CR_WMLVL, 1));
>
> avoids that reading issue (I read it wrong ;)
>
>
>
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* DAC is ready after 12us, from RM table 40-3  */
> > +	fsleep(12);
> > +
> > +	return 0;
> > +}
>
> > +
> > +static int mcf54415_read_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int *val, int *val2, long mask)
> > +{
> > +	struct mcf54415_dac *info = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = regmap_read(info->map, MCF54415_DAC_DATA, val);
>
> Sashiko pointed out that regmap_read() expects a u32* so this should use
> a local variable for the read.
>

Sashiko asked if this is causing a warning. I don't see any.
Register read is a 16bit reg, and the valid 12bits are masked just after
to ensure returned value is always correct.

So can stay as is, or to make sashiko happy i can do something as:

	struct mcf54415_dac *info = iio_priv(indio_dev);
	int ret;
	u32 reg;

	switch (mask) {
	case IIO_CHAN_INFO_RAW:
		ret = regmap_read(info->map, MCF54415_DAC_DATA, &reg);
		if (ret)
			return -EIO;
		*val = reg & 0xfff;
		return IIO_VAL_INT;

> > +		if (ret)
> > +			return -EIO;
>
> Another one sashiko got. Why is this eating the possibly more useful error
> code from regmap_read()?
>

fixed with return ret

> > +		*val &= 0xfff;
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		/* Reference voltage as per ColdFire datasheet is 3.3V */
> > +		*val = 3300 /* mV */;
> > +		*val2 = 12;
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
>
> > +static int mcf54415_dac_resume(struct device *dev)
> > +{
> > +	struct mcf54415_dac *info = iio_priv(dev_get_drvdata(dev));
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(info->clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	mcf54415_dac_init(info);
> If this fails should we report it?  I think you'd at least want
> some print to help with debug. (Sashiko got this)
>

Ok, so doing


	ret = mcf54415_dac_init(info);

	if (ret) {
		dev_err(dev, "could not resume device\n");
		return ret;
	}



> > +
> > +	return 0;
> > +}
>
>

Regards,
angelo