Re: [PATCH] iio: adc: stm32-dfsdm: add id registers support

From: Jonathan Cameron
Date: Fri Dec 23 2022 - 09:51:01 EST

On Thu, 22 Dec 2022 10:08:06 +0100
Olivier Moysan <olivier.moysan@xxxxxxxxxxx> wrote:

> Add support of identification registers to STM32 DFSDM
> to allow hardware capabilities discovery and configuration check.
> The number of filters and channels, are read from registers,
> when they are available.
>
> Signed-off-by: Olivier Moysan <olivier.moysan@xxxxxxxxxxx>
Hi Olivier,

A few minor comments inline. The fact that this facility only exists
on some supported parts needs a little more documentation.

Thanks

Jonathan

> ---
> drivers/iio/adc/stm32-dfsdm-core.c | 93 +++++++++++++++++++++++++-----
> drivers/iio/adc/stm32-dfsdm.h | 69 ++++++++++++++++------
> 2 files changed, 127 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-dfsdm-core.c b/drivers/iio/adc/stm32-dfsdm-core.c
> index a3d4de6ba4c2..7f1e4767d4ff 100644
> --- a/drivers/iio/adc/stm32-dfsdm-core.c
> +++ b/drivers/iio/adc/stm32-dfsdm-core.c
> @@ -6,6 +6,7 @@
> * Author(s): Arnaud Pouliquen <arnaud.pouliquen@xxxxxx> for STMicroelectronics.
> */
>
> +#include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> @@ -20,6 +21,7 @@
> #include "stm32-dfsdm.h"
>

As we now have a situation where we 'either' set ipid or
num_filters + num_channels and that's non obvious, I'd like to see some
documentation for this structure to explain what is going on.

> struct stm32_dfsdm_dev_data {
> + u32 ipid;
> unsigned int num_filters;
> unsigned int num_channels;
> const struct regmap_config *regmap_cfg;
> @@ -27,8 +29,6 @@ struct stm32_dfsdm_dev_data {
>
> #define STM32H7_DFSDM_NUM_FILTERS 4
> #define STM32H7_DFSDM_NUM_CHANNELS 8
> -#define STM32MP1_DFSDM_NUM_FILTERS 6
> -#define STM32MP1_DFSDM_NUM_CHANNELS 8
>
> static bool stm32_dfsdm_volatile_reg(struct device *dev, unsigned int reg)
> {
> @@ -75,8 +75,7 @@ static const struct regmap_config stm32mp1_dfsdm_regmap_cfg = {
> };
>
> static const struct stm32_dfsdm_dev_data stm32mp1_dfsdm_data = {
> - .num_filters = STM32MP1_DFSDM_NUM_FILTERS,
> - .num_channels = STM32MP1_DFSDM_NUM_CHANNELS,
> + .ipid = STM32MP15_IPIDR_NUMBER,
> .regmap_cfg = &stm32mp1_dfsdm_regmap_cfg,
> };
>
> @@ -295,6 +294,66 @@ static const struct of_device_id stm32_dfsdm_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, stm32_dfsdm_of_match);
>
> +static int stm32_dfsdm_probe_identification(struct platform_device *pdev,
> + struct dfsdm_priv *priv,
> + const struct stm32_dfsdm_dev_data *dev_data)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device_node *child;
> + struct stm32_dfsdm *dfsdm = &priv->dfsdm;
> + const char *compat;
> + int ret, count = 0;
> + u32 id, val;
> +
> + if (!dev_data->ipid) {
> + dfsdm->num_fls = dev_data->num_filters;
> + dfsdm->num_chs = dev_data->num_channels;
> + return 0;
> + }
> +
> + ret = regmap_read(dfsdm->regmap, DFSDM_IPIDR, &val);
> + if (ret)
> + return ret;
> +
> + id = FIELD_GET(DFSDM_IPIDR_MASK, val);

As mentioned below. This is the whole register, so I would not bother
masking it.

> + if (id != dev_data->ipid) {
> + dev_err(&pdev->dev, "Unexpected IP version: 0x%x", id);
> + return -EINVAL;
> + }
> +
> + for_each_child_of_node(np, child) {
> + ret = of_property_read_string(child, "compatible", &compat);
> + if (ret)
> + continue;
> + /* Count only child nodes with dfsdm compatible */
> + if (strstr(compat, "dfsdm"))
> + count++;
> + }
> +
> + ret = regmap_read(dfsdm->regmap, DFSDM_HWCFGR, &val);
> + if (ret)
> + return ret;
> +
> + dfsdm->num_fls = FIELD_GET(DFSDM_HWCFGR_NBF_MASK, val);
> + dfsdm->num_chs = FIELD_GET(DFSDM_HWCFGR_NBT_MASK, val);
> +
> + if (count > dfsdm->num_fls) {
> + dev_err(&pdev->dev, "Unexpected child number: %d", count);
> + return -EINVAL;
> + }
> +
> + ret = regmap_read(dfsdm->regmap, DFSDM_VERR, &val);
> + if (ret)
> + return ret;
> +
> + dev_dbg(&pdev->dev, "DFSDM version: %lu.%lu. %d channels/%d filters\n",
> + FIELD_GET(DFSDM_VERR_MAJREV_MASK, val),
> + FIELD_GET(DFSDM_VERR_MINREV_MASK, val),
> + dfsdm->num_chs, dfsdm->num_fls);
> +
> + return 0;
> +}
> +

> diff --git a/drivers/iio/adc/stm32-dfsdm.h b/drivers/iio/adc/stm32-dfsdm.h
> index 4afc1f528b78..4f230e2a7692 100644
> --- a/drivers/iio/adc/stm32-dfsdm.h
> +++ b/drivers/iio/adc/stm32-dfsdm.h
> @@ -13,25 +13,28 @@
>
> /*
> * STM32 DFSDM - global register map
> - * ________________________________________________________
> - * | Offset | Registers block |
> - * --------------------------------------------------------
> - * | 0x000 | CHANNEL 0 + COMMON CHANNEL FIELDS |
> - * --------------------------------------------------------
> - * | 0x020 | CHANNEL 1 |
> - * --------------------------------------------------------
> - * | ... | ..... |
> - * --------------------------------------------------------
> - * | 0x0E0 | CHANNEL 7 |
> - * --------------------------------------------------------
> - * | 0x100 | FILTER 0 + COMMON FILTER FIELDs |
> - * --------------------------------------------------------
> - * | 0x200 | FILTER 1 |
> - * --------------------------------------------------------
> - * | 0x300 | FILTER 2 |
> - * --------------------------------------------------------
> - * | 0x400 | FILTER 3 |
> - * --------------------------------------------------------
> + * __________________________________________________________
> + * | Offset | Registers block |

Original alignment is odd. Maybe pull that "Registers block"
somewhere near central?

> + * ----------------------------------------------------------
> + * | 0x000 | CHANNEL 0 + COMMON CHANNEL FIELDS |
> + * ----------------------------------------------------------
> + * | 0x020 | CHANNEL 1 |
> + * ----------------------------------------------------------
> + * | ... | ..... |
> + * ----------------------------------------------------------
> + * | 0x20 x n | CHANNEL n |
> + * ----------------------------------------------------------
> + * | 0x100 | FILTER 0 + COMMON FILTER FIELDs |
> + * ----------------------------------------------------------
> + * | 0x200 | FILTER 1 |
> + * ----------------------------------------------------------
> + * | | ..... |
> + * ----------------------------------------------------------
> + * | 0x100 x m| FILTER m |
> + * ----------------------------------------------------------
> + * ----------------------------------------------------------
> + * | 0x7F0-7FC| Identification registers |
> + * ----------------------------------------------------------

Nicer to future proof it a little and add at least one or
two more spaces before the column separator.

> */
>
> /*
> @@ -231,6 +234,34 @@
> #define DFSDM_AWCFR_AWHTF_MASK GENMASK(15, 8)
> #define DFSDM_AWCFR_AWHTF(v) FIELD_PREP(DFSDM_AWCFR_AWHTF_MASK, v)
>
> +/*
> + * Identification register definitions
> + */
> +#define DFSDM_HWCFGR 0x7F0
> +#define DFSDM_VERR 0x7F4
> +#define DFSDM_IPIDR 0x7F8
> +#define DFSDM_SIDR 0x7FC
> +
> +/* HWCFGR: Hardware configuration register */
> +#define DFSDM_HWCFGR_NBT_SHIFT 0
> +#define DFSDM_HWCFGR_NBT_MASK GENMASK(7, 0)
> +#define DFSDM_HWCFGR_NBF_SHIFT 8
> +#define DFSDM_HWCFGR_NBF_MASK GENMASK(15, 8)
> +
> +/* VERR: Version register */
> +#define DFSDM_VERR_MINREV_SHIFT 0
> +#define DFSDM_VERR_MINREV_MASK GENMASK(3, 0)
> +#define DFSDM_VERR_MAJREV_SHIFT 4
> +#define DFSDM_VERR_MAJREV_MASK GENMASK(7, 4)
> +
> +/* IPDR: Identification register */
> +#define DFSDM_IPIDR_MASK GENMASK(31, 0)
Isn't this the whole register? Under those circumstances,
I don't see any point in having a mask.
> +
> +/* SIDR: Size identification register */
> +#define DFSDM_SIDR_MASK GENMASK(31, 0)
> +
> +#define STM32MP15_IPIDR_NUMBER 0x00110031
> +
> /* DFSDM filter order */
> enum stm32_dfsdm_sinc_order {
> DFSDM_FASTSINC_ORDER, /* FastSinc filter type */