Re: [PATCH v3 11/11] iio: dac: add mcf54415 DAC
From: Angelo Dureghello
Date: Tue May 26 2026 - 17:28:51 EST
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 <adureghello@xxxxxxxxxxxx> wrote:
>
> > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> >
> > 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 <adureghello@xxxxxxxxxxxx>
> 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 - adureghello@xxxxxxxxxxxx
> > + */
> > +
> > +#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, ®);
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