Messages in this thread |  | | Subject | Re: [PATCH 10/10] iio: dac: add mcf54415 DAC | | From | Nuno Sá <> | | Date | Mon, 04 May 2026 20:13:19 +0100 |
| |
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á
|  |