Re: [PATCH 2/3] iio: adc: add stm32 mdf support

[Krzysztof Kozlowski <krzk@kernel.org>]

On Thu, Oct 01, 2026 at 04:56:46PM +0200, Olivier Moysan wrote:
> +static int stm32_mdf_core_of_cck_get(struct platform_device *pdev, struct stm32_mdf_priv *priv)
> +{
> +	struct device *dev = &pdev->dev;
> +	u32 freq;
> +	int ret;
> +
> +	ret = device_property_read_u32(dev, "clock-frequency", &freq);

How? Your driver depends on OF and there is no such property in OF, so
why can you read it?

Otherwise what is this:

"depends on (ARCH_STM32 && OF)"

> +	if (ret < 0) {
> +		/* If property does not exist return immediately */
> +		if (ret == -EINVAL)
> +			return 0;
> +
> +		dev_err(dev, "Failed to read clock-frequency property: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!freq) {
> +		dev_err(dev, "Null frequency not allowed for cck output frequency\n");
> +		return -EINVAL;
> +	}
> +	priv->cck_freq = freq;
> +
> +	return 0;
> +}
> +
> +static int stm32_mdf_core_parse_clocks(struct platform_device *pdev, struct stm32_mdf_priv *priv)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct clk *kclk;
> +	int ret;
> +
> +	kclk = devm_clk_get(dev, "ker_ck");
> +	if (IS_ERR(kclk))
> +		return dev_err_probe(dev, PTR_ERR(kclk), "Failed to get kernel clock\n");
> +
> +	priv->kclk = kclk;
> +	priv->kclk_rate = clk_get_rate(kclk);
> +
> +	/* CCK0 and CCK1 clocks are optional. Used only in SPI master modes. */
> +	ret = stm32_mdf_core_of_cck_get(pdev, priv);
> +	if (ret)
> +		return ret;
> +
> +	if (priv->cck_freq) {
> +		ret = stm32_mdf_core_cck_divider_set_rate(pdev, priv, priv->kclk_rate);
> +		if (ret) {
> +			dev_err(dev, "Failed to set cck rate: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	ret = stm32_mdf_core_register_clock_provider(pdev, priv);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int stm32_mdf_core_parse_of(struct platform_device *pdev, struct stm32_mdf_priv *priv)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct reset_control *rst;
> +	struct device *dev = &pdev->dev;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	struct fwnode_handle *handle;
> +	struct fwnode_handle **fh;
> +	int count, ret, i;
> +
> +	if (!node)
> +		return -EINVAL;
> +
> +	rst = devm_reset_control_get_optional_exclusive(&pdev->dev, "mdf");
> +	if (IS_ERR(rst))
> +		return dev_err_probe(dev, PTR_ERR(rst), "Failed to get reset controller\n");
> +
> +	ret = reset_control_reset(rst);
> +	if (ret) {
> +		dev_err(&pdev->dev, "reset_control_reset failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = stm32_mdf_core_parse_clocks(pdev, priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (device_property_present(&pdev->dev, "st,interleave")) {
> +		count = fwnode_property_count_u32(fwnode, "st,interleave");
> +		if (count < 2 || count > priv->mdf.nbf) {
> +			dev_err(dev, "Wrong interleave filters number [%d]\n", count);
> +			return -EINVAL;
> +		}
> +
> +		fh = devm_kzalloc(dev, count * sizeof(*fh), GFP_KERNEL);
> +		if (!fh)
> +			return -ENOMEM;
> +		priv->mdf.fh_interleave = fh;
> +
> +		for (i = 0; i < count; i++) {
> +			handle = fwnode_find_reference(fwnode, "st,interleave", i);
> +			if (IS_ERR(handle)) {
> +				dev_err(dev, "Failed to read filter handle: %ld\n",
> +					PTR_ERR(handle));
> +				return PTR_ERR(handle);
> +			}
> +			priv->mdf.fh_interleave[i] = handle;
> +		}
> +
> +		priv->mdf.nb_interleave = count;
> +
> +		/* Configure GCR */
> +		ret = regmap_update_bits(priv->regmap, MDF_GCR_REG,
> +					 MDF_GCR_ILVNB_MASK, MDF_GCR_ILVNB(count - 1));
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct of_device_id stm32_mdf_of_match[] = {
> +	{ .compatible = "st,stm32mp25-mdf" },
> +	{ .compatible = "st,stm32mp23-mdf", .data = (void *)STM32_MDF_MP23_FILTER_NB },

Again odd order of entries. Why doing things reverse from natural sort order?

> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, stm32_mdf_of_match);
> +
> +static int stm32_mdf_core_identification(struct platform_device *pdev, struct stm32_mdf_priv *priv)
> +{
> +	struct stm32_mdf *mdf = &priv->mdf;
> +	u32 val;
> +	int ret;
> +
> +	/* If filter number is explicitly defined, don't check identification registers */
> +	mdf->nbf = (uintptr_t)device_get_match_data(&pdev->dev);

Don't use uintptr_t, but unsigned long.

> +	if (mdf->nbf)
> +		return 0;
> +
> +	ret = regmap_read(priv->regmap, MDF_IPIDR_REG, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val == STM32MP25_MDF_IPIDR_NUMBER) {
> +		ret = regmap_read(priv->regmap, MDF_HWCFGR_REG, &val);
> +		if (ret)
> +			return ret;
> +
> +		mdf->nbf = FIELD_GET(MDF_HWCFGR_NBF_MASK, val);
> +
> +		ret = regmap_read(priv->regmap, MDF_VERR_REG, &val);
> +
> +		dev_dbg(&pdev->dev, "MDF version: %lu.%lu\n", FIELD_GET(MDF_VERR_MAJREV_MASK, val),
> +			FIELD_GET(MDF_VERR_MINREV_MASK, val));
> +	} else {
> +		dev_err(&pdev->dev, "Unexpected ID number: 0x%x\n", val);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int stm32_mdf_core_probe(struct platform_device *pdev)
> +{
> +	struct stm32_mdf_priv *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->pdev = pdev;
> +	spin_lock_init(&priv->lock);
> +
> +	priv->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(priv->base))
> +		return PTR_ERR(priv->base);
> +	priv->phys_base = res->start;
> +
> +	priv->regmap =
> +		devm_regmap_init_mmio_clk(&pdev->dev, "ker_ck", priv->base, &stm32_mdf_regmap_cfg);

Odd wrappung.

> +	if (IS_ERR(priv->regmap)) {
> +		ret = PTR_ERR(priv->regmap);
> +		dev_err(&pdev->dev, "Failed to allocate regmap: %d\n", ret);

We do not print allocation errors.

OTOH, every other probe failure should use return dev_err_probe syntax.
Again, same with bindings, please take existing recent code as starting
point, not upstream your old vendor code.


> +		return ret;
> +	}
> +
> +	ret = stm32_mdf_core_identification(pdev, priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = stm32_mdf_core_parse_of(pdev, priv);
> +	if (ret < 0)
> +		return ret;
> +
> +	INIT_LIST_HEAD(&priv->mdf.sitf_list);
> +	INIT_LIST_HEAD(&priv->mdf.filter_list);
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
> +	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
> +	if (ret)
> +		goto pm_put;
> +
> +	pm_runtime_put(&pdev->dev);
> +
> +	return 0;
> +
> +pm_put:
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +
> +	return ret;
> +}
> +
> +static void stm32_mdf_core_remove(struct platform_device *pdev)
> +{
> +	pm_runtime_get_sync(&pdev->dev);
> +	of_platform_depopulate(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_put_noidle(&pdev->dev);
> +}
> +
> +static int stm32_mdf_core_suspend(struct device *dev)
> +{
> +	struct stm32_mdf *mdf = dev_get_drvdata(dev);
> +	struct stm32_mdf_priv *priv = to_stm32_mdf_priv(mdf);
> +	int ret;
> +
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	regcache_cache_only(priv->regmap, true);
> +	regcache_mark_dirty(priv->regmap);
> +
> +	/* Balance devm_regmap_init_mmio_clk() clk_prepare() */
> +	clk_unprepare(priv->kclk);
> +
> +	return pinctrl_pm_select_sleep_state(dev);
> +}
> +
> +static int stm32_mdf_core_resume(struct device *dev)
> +{
> +	struct stm32_mdf *mdf = dev_get_drvdata(dev);
> +	struct stm32_mdf_priv *priv = to_stm32_mdf_priv(mdf);
> +	int ret;
> +
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to set pins default state: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare(priv->kclk);
> +	if (ret) {
> +		dev_err(dev, "Failed to prepare kernel clock: %d\n", ret);
> +		goto err_clk;
> +	}
> +
> +	regcache_cache_only(priv->regmap, false);
> +	ret = regcache_sync(priv->regmap);
> +	if (ret) {
> +		dev_err(dev, "Failed to sync cache: %d\n", ret);
> +		goto err_cache;
> +	}
> +
> +	return pm_runtime_force_resume(dev);
> +
> +err_cache:
> +	clk_unprepare(priv->kclk);
> +err_clk:
> +	pinctrl_pm_select_sleep_state(dev);
> +
> +	return ret;
> +}
> +
> +static int stm32_mdf_core_runtime_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static int stm32_mdf_core_runtime_resume(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops stm32_mdf_core_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(stm32_mdf_core_suspend, stm32_mdf_core_resume)
> +	SET_RUNTIME_PM_OPS(stm32_mdf_core_runtime_suspend, stm32_mdf_core_runtime_resume, NULL)
> +};
> +
> +static struct platform_driver stm32_mdf_driver = {
> +	.probe = stm32_mdf_core_probe,
> +	.remove = stm32_mdf_core_remove,
> +	.driver = {
> +		.name = "stm32-mdf",
> +		.of_match_table = stm32_mdf_of_match,
> +		.pm = &stm32_mdf_core_pm_ops,
> +	},
> +};
> +
> +module_platform_driver(stm32_mdf_driver);
> +
> +MODULE_AUTHOR("Olivier Moysan <olivier.moysan@foss.st.com>");
> +MODULE_DESCRIPTION("STMicroelectronics STM32 MDF driver");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/adc/stm32-mdf-serial.c b/drivers/iio/adc/stm32-mdf-serial.c
> new file mode 100644
> index 000000000000..15a58cf833be
> --- /dev/null
> +++ b/drivers/iio/adc/stm32-mdf-serial.c
> @@ -0,0 +1,309 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * This file is part of STM32 MDF driver
> + *
> + * Copyright (C) 2023, STMicroelectronics - All Rights Reserved
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#include "stm32-mdf.h"
> +
> +#define STM32_MDF_MODE_SZ 12
> +
> +enum {
> +	STM32_MDF_SCKSRC_CCK0,
> +	STM32_MDF_SCKSRC_CCK1,
> +	STM32_MDF_SCKSRC_CLK,
> +	STM32_MDF_SCKSRC_NONE,
> +};
> +
> +struct stm32_mdf_sf_mode {
> +	const char *name;
> +	u32 mode;
> +};
> +
> +static const struct stm32_mdf_sf_mode stm32_mdf_mode[STM32_MDF_MODE_NB] = {
> +	{ "spi", STM32_MDF_MODE_SPI },
> +	{ "lf_spi", STM32_MDF_MODE_LF_SPI },
> +	{ "manchester_r", STM32_MDF_MODE_MANCHESTER_R },
> +	{ "manchester_f", STM32_MDF_MODE_MANCHESTER_F },
> +};
> +
> +static bool stm32_mdf_sitf_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case MDF_SITFCR_REG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static bool stm32_mdf_sitf_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case MDF_SITFCR_REG:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +static const struct regmap_config stm32_sitf_regmap_cfg = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = sizeof(u32),
> +	.max_register = MDF_SITFCR_REG,
> +	.readable_reg = stm32_mdf_sitf_readable_reg,
> +	.writeable_reg = stm32_mdf_sitf_writeable_reg,
> +	.fast_io = true,
> +	/* Do not use regcache as it does not support single register map */
> +};
> +
> +int stm32_mdf_sitf_start(struct stm32_mdf_sitf *sitf)
> +{
> +	int ret;
> +
> +	spin_lock(&sitf->lock);
> +
> +	ret = regmap_set_bits(sitf->regmap, MDF_SITFCR_REG, MDF_SITFCR_SITFEN);
> +	if (!ret)
> +		sitf->refcnt++;
> +
> +	spin_unlock(&sitf->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(stm32_mdf_sitf_start);

You need kerneldoc for all of exports.


> +
> +int stm32_mdf_sitf_stop(struct stm32_mdf_sitf *sitf)
> +{
> +	int ret = 0;
> +
> +	spin_lock(&sitf->lock);
> +
> +	if (!sitf->refcnt) {
> +		dev_err(sitf->dev, "Unbalanced serial interface stop ?\n");
> +		ret = -EPERM;
> +		goto out;
> +	} else {
> +		sitf->refcnt--;
> +	}
> +
> +	if (!sitf->refcnt)
> +		ret = regmap_clear_bits(sitf->regmap, MDF_SITFCR_REG, MDF_SITFCR_SITFEN);
> +
> +out:
> +	spin_unlock(&sitf->lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(stm32_mdf_sitf_stop);
> +
> +static int stm32_mdf_sitf_get_clk(struct device *dev, struct stm32_mdf_sitf *sitf)
> +{
> +	struct clk *sck;
> +
> +	/* Optional clock. Clock not needed in Manchester mode */
> +	sck = clk_get_optional(sitf->dev, 0);
> +	if (IS_ERR(sck))
> +		return dev_err_probe(sitf->dev, PTR_ERR(sck), "Can't get serial clock\n");
> +
> +	sitf->sck = sck;
> +
> +	if (sitf->sck) {
> +		if (!strncmp(__clk_get_name(sck), STM32_MDF_CCK0, sizeof(STM32_MDF_CCK0)))
> +			sitf->scksrc = STM32_MDF_SCKSRC_CCK0;
> +		else if (!strncmp(__clk_get_name(sck), STM32_MDF_CCK1, sizeof(STM32_MDF_CCK1)))
> +			sitf->scksrc = STM32_MDF_SCKSRC_CCK1;
> +		else
> +			sitf->scksrc = STM32_MDF_SCKSRC_CLK;
> +	}
> +
> +	return 0;
> +};
> +
> +static int stm32_mdf_sitf_parse(struct platform_device *pdev, struct stm32_mdf_sitf *sitf)
> +{
> +	struct device *dev = &pdev->dev;
> +	const char *str;
> +	int ret, i = 0;
> +	u32 idx, mode, sitfcr;
> +
> +	ret = device_property_read_u32(dev, "reg", &idx);
> +	if (ret) {
> +		dev_err(dev, "Could not get interface index: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (idx % 0x80) {
> +		dev_err(dev, "Unexpected reg property value [%x]\n", idx);
> +		return -EINVAL;
> +	}
> +
> +	idx = (idx >> 7) - 1;
> +	if (idx > sitf->mdf->nbf) {
> +		dev_err(dev, "Interface index [%d] exceeds maximum [%d]\n", idx, sitf->mdf->nbf);
> +		return -EINVAL;
> +	}
> +
> +	/* Get SITF mode */
> +	ret = device_property_read_string(dev, "st,sitf-mode", &str);
> +	if (ret) {
> +		dev_err(dev, "Could not get interface mode: %d\n", ret);
> +		return ret;
> +	}
> +
> +	while (i < STM32_MDF_MODE_NB) {
> +		if (!strncmp(stm32_mdf_mode[i].name, str, STM32_MDF_MODE_SZ)) {
> +			mode = stm32_mdf_mode[i].mode;
> +			break;
> +		}
> +		i++;
> +	}
> +
> +	if (i >= STM32_MDF_MODE_NB) {
> +		dev_err(dev, "Unknown serial interface mode [%s]\n", str);
> +		return -EINVAL;
> +	}
> +
> +	sitf->mode = mode;
> +	sitfcr = MDF_SITFCR_SITFMOD(mode);
> +
> +	ret = stm32_mdf_sitf_get_clk(dev, sitf);
> +	if (ret)
> +		return ret;
> +
> +	if (mode == STM32_MDF_MODE_SPI && sitf->scksrc == STM32_MDF_SCKSRC_NONE) {
> +		dev_err(dev, "Missing clock for serial interface [%d] in SPI mode\n", idx);
> +		return -EINVAL;
> +	}
> +
> +	if (mode == STM32_MDF_MODE_LF_SPI && (sitf->scksrc != STM32_MDF_SCKSRC_CCK0 &&
> +					      sitf->scksrc != STM32_MDF_SCKSRC_CCK1)) {
> +		dev_err(dev, "Missing CCKx clock for serial interface [%d] in LF_SPI mode\n", idx);
> +		return -EINVAL;
> +	}
> +
> +	sitf->id = idx;
> +	sitf->node = dev_fwnode(dev);
> +	spin_lock_init(&sitf->lock);
> +
> +	sitfcr |= MDF_SITFCR_SCKSRC(sitf->scksrc);
> +
> +	/* Configure SITF register */
> +	regmap_set_bits(sitf->regmap, MDF_SITFCR_REG, sitfcr);
> +
> +	dev_dbg(dev, "Serial interface [%d] registered\n", idx);

Drop, that's just probe success.

> +
> +	return 0;
> +}
> +
> +static int stm32_mdf_sitf_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct stm32_mdf_sitf *sitf;
> +	struct regmap *regmap;
> +	struct resource *res;
> +	void __iomem *base;
> +	int ret;
> +
> +	sitf = devm_kzalloc(&pdev->dev, sizeof(*sitf), GFP_KERNEL);
> +	if (!sitf)
> +		return -ENOMEM;
> +	sitf->dev = dev;
> +	sitf->mdf = dev_get_drvdata(dev->parent);
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	regmap = devm_regmap_init_mmio_clk(dev, "ker_ck", base, &stm32_sitf_regmap_cfg);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap), "Failed to init regmap\n");

And here quite different choice. Your code is just inconsistent.

> +	sitf->regmap = regmap;
> +
> +	ret = stm32_mdf_sitf_parse(pdev, sitf);
> +	if (ret < 0)
> +		return ret;

Best regards,
Krzysztof