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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 13 May 2026 11:14:35 +0200
Angelo Dureghello <adureghello@baylibre.com> 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. 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@baylibre.com>

As for all new IIO code, Sashiko will eventually take a look at it - I think
it's running about a day behind at the moment.  Once it catches up please
take a look (so far it's not even listing the series as pending!)

Looks good to me. Some trivial stuff inline. Only one I'm that fussed about
is not having a macro for the definition of a single channel. 
Superficially looks like no compile time dependencies between this and
the rest of the series I think so once people are happy with the whole thing
I'll pick this up through the IIO tree.

Jonathan

> diff --git a/drivers/iio/dac/mcf54415_dac.c b/drivers/iio/dac/mcf54415_dac.c
> new file mode 100644
> index 000000000000..e95ab6b89b17
> --- /dev/null
> +++ b/drivers/iio/dac/mcf54415_dac.c
...


> +static void mcf54415_dac_exit(void *data)
> +{
> +	struct mcf54415_dac *info = data;
> +
> +	regmap_update_bits(info->map, MCF54415_DAC_CR, MCF54415_DAC_CR_PDN,
> +			   MCF54415_DAC_CR_PDN);

regmap_set_bits() just to be a tiny bit more compact.

> +}
> +
> +#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,

For a single channel case like this I'd skip the macro - it's just
making things a little harder to read. i.e.
	.type = IIO_VOLTAGE,
etc here

> +};
> +

> +static int mcf54415_dac_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
Given the type passed to iio_priv() is very well known this
isn't really bringing any type safety - so you could just do

	struct mcf54415_dac *info = iio_priv(dev_get_drvdata(dev));
here and in resume.
I don't really care either way!


> +	struct mcf54415_dac *info = iio_priv(indio_dev);
> +
> +	mcf54415_dac_exit(info);
> +	clk_disable_unprepare(info->clk);
> +
> +	return 0;
> +}
> +
> +static int mcf54415_dac_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct mcf54415_dac *info = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(info->clk);
> +	if (ret)
> +		return ret;
> +
> +	mcf54415_dac_init(info);
> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(mcf54415_dac_pm_ops,
> +				mcf54415_dac_suspend,
> +				mcf54415_dac_resume);
> +

Trivial but might as well wrap as:
static DEFINE_SIMPLE_DEV_PM_OPS(mcf54415_dac_pm_ops,
				mcf54415_dac_suspend, mcf54415_dac_resume);

And save us scrolling one line extra.