From: Andy Shevchenko <andriy.shevchenko@intel•com>
To: Angelo Dureghello <adureghello@baylibre•com>
Cc: "Greg Ungerer" <gerg@linux-m68k•org>,
"Geert Uytterhoeven" <geert@linux-m68k•org>,
"Steven King" <sfking@fdwdc•com>, "Arnd Bergmann" <arnd@arndb•de>,
"Maxime Coquelin" <mcoquelin.stm32@gmail•com>,
"Alexandre Torgue" <alexandre.torgue@foss•st.com>,
"Jonathan Cameron" <jic23@kernel•org>,
"David Lechner" <dlechner@baylibre•com>,
"Nuno Sá" <nuno.sa@analog•com>,
"Andy Shevchenko" <andy@kernel•org>,
"Greg Ungerer" <gerg@uclinux•org>,
linux-m68k@lists•linux-m68k.org, linux-kernel@vger•kernel.org,
linux-stm32@st-md-mailman•stormreply.com,
linux-arm-kernel@lists•infradead.org, linux-iio@vger•kernel.org
Subject: Re: [PATCH v4 11/11] iio: dac: add mcf54415 DAC
Date: Tue, 2 Jun 2026 12:39:15 +0300 [thread overview]
Message-ID: <ah6kw-oTM9jgkWBL@ashevche-desk.local> (raw)
In-Reply-To: <20260531-wip-stmark2-dac-v4-11-7e65ab4215dd@baylibre.com>
On Sun, May 31, 2026 at 05:26:04PM +0200, Angelo Dureghello wrote:
> Add basic version of mcf54415 DAC driver. DAC is embedded in the cpu and
CPU
(or maybe you wanted use 'SoC' acronym)
> 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.
Be consistent: 12-bit on 16-bit
...
> Changes in v4:
> - remove unused includes
> - sashiko: return "ret" as regmap_read ret value in case of error
> - sashiko: using u32 as regmap_read value
Why? regmap API uses 'unsigned int'. Then you can take out any bits, fields,
et cetera from it into fixed-width type of variables.
> - use local variable in mcf54415_dac_init() for better readability
> - sashiko: check mcf54415_dac_init return value also in resume()
...
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/compiler_types.h>
Drop this (see below).
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
+ types.h (it also guarantees compile_types.h).
...
> +static int mcf54415_dac_init(struct mcf54415_dac *info)
> +{
> + int ret;
> + u16 val = MCF54415_DAC_CR_FILT | FIELD_PREP(MCF54415_DAC_CR_WMLVL, 1);
Can we move towards reversed xmas tree order?
u16 val = MCF54415_DAC_CR_FILT | FIELD_PREP(MCF54415_DAC_CR_WMLVL, 1);
int ret;
> + /* Fixed defaults and enable DAC (bit 0 set to 0) */
> + ret = regmap_write(info->map, MCF54415_DAC_CR, val);
> + if (ret)
> + return ret;
> +
> + /* DAC is ready after 12us, from RM table 40-3 */
> + fsleep(12);
> +
> + 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:
> + /* Check based on RM 30.3.2 (DACn_DATA) reg. resolution */
> + if (val < 0 || val > 4095)
> + return -EINVAL;
> + return regmap_write(info->map, MCF54415_DAC_DATA, val);
So, for example, -1 will be written as 0xffffffff (with the respective bits
taken into account). Is it a problem?
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int mcf54415_dac_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct iio_dev *indio_dev;
> + struct mcf54415_dac *info;
> + void __iomem *regs;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*info));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + info = iio_priv(indio_dev);
> +
> + regs = devm_platform_ioremap_resource(pdev, 0);
> + if (IS_ERR(regs))
> + return dev_err_probe(dev, PTR_ERR(regs),
> + "failed to get io regs\n");
One line. It's more than decade that checkpatch stopped complaining on
the trailing string literals.
> + info->map = devm_regmap_init_mmio(dev, regs,
> + &mcf54415_dac_regmap_config);
One line (yes, 82 characters).
> + if (IS_ERR(info->map))
> + return PTR_ERR(info->map);
> +
> + info->clk = devm_clk_get_enabled(dev, "dac");
> + if (IS_ERR(info->clk))
> + return dev_err_probe(dev, PTR_ERR(info->clk),
> + "failed getting clock\n");
Also one line.
> + platform_set_drvdata(pdev, indio_dev);
> +
> + indio_dev->name = "mcf54415";
> + indio_dev->info = &mcf54415_dac_iio_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = &mcf54415_dac_iio_channel;
> + indio_dev->num_channels = 1;
> +
> + ret = mcf54415_dac_init(info);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(dev, mcf54415_dac_exit, info);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
...
> +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;
> +
> + ret = mcf54415_dac_init(info);
> + if (ret) {
> + dev_err(dev, "could not resume device\n");
> + return ret;
> + }
> +
> + return 0;
> +
Besides stray blank line the above can be
ret = mcf54415_dac_init(info);
if (ret)
dev_err(dev, "could not resume device\n");
return ret;
> +}
--
With Best Regards,
Andy Shevchenko
prev parent reply other threads:[~2026-06-02 9:40 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-05-31 15:25 [PATCH v4 00/11] add mcf54415 DAC driver Angelo Dureghello
2026-05-31 15:25 ` [PATCH v4 01/11] m68k: mcf5441x: fix clocks numbering Angelo Dureghello
2026-05-31 15:25 ` [PATCH v4 02/11] m68k: mcf5441x: add clock for DAC channel 1 Angelo Dureghello
2026-05-31 15:25 ` [PATCH v4 03/11] m68k: mcf5441x: initialize DAC clocks by iio DAC driver name Angelo Dureghello
2026-05-31 15:25 ` [PATCH v4 04/11] m68k: defconfig: update stmark2 defconfig Angelo Dureghello
2026-05-31 15:51 ` Jonathan Cameron
2026-06-02 9:24 ` Andy Shevchenko
2026-06-02 9:40 ` Andy Shevchenko
2026-05-31 15:25 ` [PATCH v4 05/11] m68k: add DAC modules base addresses Angelo Dureghello
2026-05-31 15:25 ` [PATCH v4 06/11] m68k: mcf5441x: add CCM registers Angelo Dureghello
2026-05-31 15:26 ` [PATCH v4 07/11] m68k: mcf5441x: add CCR MISCCR2 bitfields Angelo Dureghello
2026-05-31 15:26 ` [PATCH v4 08/11] m68k: stmark2: use ioport.h macros for resources Angelo Dureghello
2026-05-31 15:26 ` [PATCH v4 09/11] m68k: stmark2: add mcf5441x DAC platform devices Angelo Dureghello
2026-06-02 9:27 ` Andy Shevchenko
2026-05-31 15:26 ` [PATCH v4 10/11] m68k: stmark2: enable DACs outputs Angelo Dureghello
2026-05-31 15:26 ` [PATCH v4 11/11] iio: dac: add mcf54415 DAC Angelo Dureghello
2026-05-31 16:37 ` Jonathan Cameron
2026-06-02 9:39 ` Andy Shevchenko [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ah6kw-oTM9jgkWBL@ashevche-desk.local \
--to=andriy.shevchenko@intel$(echo .)com \
--cc=adureghello@baylibre$(echo .)com \
--cc=alexandre.torgue@foss$(echo .)st.com \
--cc=andy@kernel$(echo .)org \
--cc=arnd@arndb$(echo .)de \
--cc=dlechner@baylibre$(echo .)com \
--cc=geert@linux-m68k$(echo .)org \
--cc=gerg@linux-m68k$(echo .)org \
--cc=gerg@uclinux$(echo .)org \
--cc=jic23@kernel$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-iio@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-m68k@lists$(echo .)linux-m68k.org \
--cc=linux-stm32@st-md-mailman$(echo .)stormreply.com \
--cc=mcoquelin.stm32@gmail$(echo .)com \
--cc=nuno.sa@analog$(echo .)com \
--cc=sfking@fdwdc$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox