Re: [PATCH 10/10] iio: dac: add mcf54415 DAC

Nuno Sá
2026-05-04T19:12:21Z

Hi Angelo,

Minor stuff from me

On Mon, 2026-05-04 at 19:16 +0200, Angelo Dureghello wrote:
> From: Angelo Dureghello <adureghello@baylibre.com>
>
> 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. DAC_VREFL and DAC_VREFH defaults respectivley to
> 0 and 0xfff.
>
> 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.
>
> Basic tests done on stmark2 mcf54415-based board, voltage check on DAC0:
>
> /sys/bus/iio/devices/iio:device0 # ls
> name                 out_voltage_raw      subsystem
> out_conversion_mode  out_voltage_scale    uevent
>
> /sys/bus/iio/devices/iio:device0 # cat name
> mcf54415_dac.0
>
> /sys/bus/iio/devices/iio:device0 #
>
> echo 4095 > out_voltage_raw     => voltage abt 3.3V by oscilloscope
> echo 4096 > out_voltage_raw     => roll over to 0V
> echo 0 > out_voltage_raw        => voltage is 0V
> echo 2048 > out_voltage_raw     => voltage is abt 1.7V, mid scale
>
> Same behavior for /sys/bus/iio/devices/iio:device1.
>
> Generated a sine wave by shell script, sine shape is good.
>

Not sure if the above example belongs to the commit message. I would just write a
small doc if you really want to document the above. We're now seeing more docs for
IIO drivers.

> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> ---
>  drivers/iio/dac/Kconfig        |  10 +++
>  drivers/iio/dac/Makefile       |   1 +
>  drivers/iio/dac/mcf54415_dac.c | 200 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 211 insertions(+)
>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index cd4870b65415..17550e99cfdd 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -516,6 +516,16 @@ 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
> + 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.
> +
>  config MCP4725
>   tristate "MCP4725/6 DAC driver"
>   depends on I2C
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 2a80bbf4e80a..1cb93e83d0eb 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_MAX517) += max517.o
>  obj-$(CONFIG_MAX22007) += max22007.o
>  obj-$(CONFIG_MAX5522) += max5522.o
>  obj-$(CONFIG_MAX5821) += max5821.o
> +obj-$(CONFIG_MCF54415_DAC) += mcf54415_dac.o
>  obj-$(CONFIG_MCP4725) += mcp4725.o
>  obj-$(CONFIG_MCP4728) += mcp4728.o
>  obj-$(CONFIG_MCP47FEB02) += mcp47feb02.o
> diff --git a/drivers/iio/dac/mcf54415_dac.c b/drivers/iio/dac/mcf54415_dac.c
> new file mode 100644
> index 000000000000..4031a5dc1f9d
> --- /dev/null
> +++ b/drivers/iio/dac/mcf54415_dac.c
> @@ -0,0 +1,200 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * NXP mcf54415 DAC driver
> + *
> + * Copyright 2026 BayLibre - adureghello@baylibre.com
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.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
> +
> +#define MCF54415_DAC_READY_US 12
> +
> +struct mcf54415_dac {
> + struct clk *clk;
> + struct device *dev;
> + void __iomem *regs;
> +};
> +
> +static void mcf54415_dac_init(struct mcf54415_dac *info)
> +{
> + int val;
> +
> + /* Keeping defaults and enable DAC (bit 0 set to 0) */
> + val = MCF54415_DAC_CR_FILT;
> + val |= FIELD_PREP(MCF54415_DAC_CR_WMLVL, 1);
> +
> + writew(val, info->regs + MCF54415_DAC_CR);
> +
> + /* DAC is ready after 12us, from RM table 40-3  */
> + fsleep(MCF54415_DAC_READY_US);
> +}
> +
> +static void mcf54415_dac_exit(void *data)
> +{
> + struct mcf54415_dac *info = data;
> + int val;
> +
> + val = readw(info->regs + MCF54415_DAC_CR);
> + val |= MCF54415_DAC_CR_PDN;
> + writew(val, info->regs + MCF54415_DAC_CR);
> +}
> +
> +#define MCF54415_DAC_CHAN { \
> + .type = IIO_VOLTAGE, \
> + .output = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +static const struct iio_chan_spec mcf54415_dac_iio_channels[] = {
> + MCF54415_DAC_CHAN
> +};
> +
> +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);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + *val = readw(info->regs + MCF54415_DAC_DATA);
> + 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;
> + }
> +
> + return 0;
> +}
> +
> +static int mcf54415_write_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);
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + writew(val, info->regs + MCF54415_DAC_DATA);
> + return 0;
> +

No need for the new line.

> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info mcf54415_dac_iio_info = {
> + .read_raw = &mcf54415_read_raw,
> + .write_raw = &mcf54415_write_raw,
> +};
> +
> +static int mcf54415_dac_probe(struct platform_device *pdev)
> +{
> + struct iio_dev *indio_dev;
> + struct mcf54415_dac *info;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev,
> +   sizeof(struct mcf54415_dac));

Slight preference for sizeof(*info)

> + if (!indio_dev)
> + return -ENOMEM;
> +
> + info = iio_priv(indio_dev);
> + info->dev = &pdev->dev;
> +
> + info->regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(info->regs))
> + return dev_err_probe(&pdev->dev, PTR_ERR(info->regs),
> +      "failed to get io regs\n");
> +
> + info->clk = devm_clk_get_enabled(&pdev->dev, "dac");
> + if (IS_ERR(info->clk))
> + return dev_err_probe(&pdev->dev, PTR_ERR(info->clk),
> +      "failed getting clock\n");
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
>
> + indio_dev->name = dev_name(&pdev->dev);

Just use "mcf54415".

> + indio_dev->info = &mcf54415_dac_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = mcf54415_dac_iio_channels;
> + indio_dev->num_channels = ARRAY_SIZE(mcf54415_dac_iio_channels);
> +
> + mcf54415_dac_init(info);
> +
> + ret = devm_add_action_or_reset(&pdev->dev, mcf54415_dac_exit, info);
> + if (ret)
> + return ret;
> +
> + ret = devm_iio_device_register(&pdev->dev, indio_dev);
> + if (ret)
> + dev_err(&pdev->dev, "couldn't register the device\n");
> +

I would just return devm_iio_device_register();

- Nuno Sá