Re: [PATCH v2 2/3] nvmem: syscon: Add syscon backed nvmem driver

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 17/05/2023 17:25, Marek Vasut wrote:
> Add trivial driver which permits exposing syscon backed register
> to userspace. This is useful e.g. to expose U-Boot boot counter
> on various platforms where the boot counter is stored in random
> volatile register, like STM32MP15xx TAMP_BKPxR register.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Alexandre Torgue <alexandre.torgue@foss.st.com>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Cc: devicetree@vger.kernel.org
> Cc: kernel@dh-electronics.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-stm32@st-md-mailman.stormreply.com
> ---
> V2: No change
> ---
>  drivers/nvmem/Kconfig        |  10 ++++
>  drivers/nvmem/Makefile       |   2 +
>  drivers/nvmem/nvmem-syscon.c | 105 +++++++++++++++++++++++++++++++++++
>  3 files changed, 117 insertions(+)
>  create mode 100644 drivers/nvmem/nvmem-syscon.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index b291b27048c76..4e4aecdd9c293 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -303,6 +303,16 @@ config NVMEM_STM32_BSEC_OPTEE_TA
>  	  This library is a used by stm32-romem driver or included in the module
>  	  called nvmem-stm32-romem.
>  
> +config NVMEM_SYSCON
> +	tristate "Generic syscon backed nvmem"
> +	help
> +	  This is a driver for generic syscon backed nvmem. This can be
> +	  used to expose arbitrary syscon backed register to user space
> +	  via nvmem, like the U-Boot boot counter.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called nvmem-syscon.
> +
>  config NVMEM_STM32_ROMEM
>  	tristate "STMicroelectronics STM32 factory-programmed memory support"
>  	depends on ARCH_STM32 || COMPILE_TEST
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index f82431ec8aef0..d10e478e6a6c9 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -60,6 +60,8 @@ obj-$(CONFIG_NVMEM_SPMI_SDAM)		+= nvmem_qcom-spmi-sdam.o
>  nvmem_qcom-spmi-sdam-y			+= qcom-spmi-sdam.o
>  obj-$(CONFIG_NVMEM_SPRD_EFUSE)		+= nvmem_sprd_efuse.o
>  nvmem_sprd_efuse-y			:= sprd-efuse.o
> +obj-$(CONFIG_NVMEM_SYSCON)		+= nvmem_syscon.o
> +nvmem_syscon-y				:= nvmem-syscon.o
>  obj-$(CONFIG_NVMEM_STM32_ROMEM)		+= nvmem_stm32_romem.o
>  nvmem_stm32_romem-y 			:= stm32-romem.o
>  nvmem_stm32_romem-$(CONFIG_NVMEM_STM32_BSEC_OPTEE_TA) += stm32-bsec-optee-ta.o
> diff --git a/drivers/nvmem/nvmem-syscon.c b/drivers/nvmem/nvmem-syscon.c
> new file mode 100644
> index 0000000000000..e0aa5af0300d3
> --- /dev/null
> +++ b/drivers/nvmem/nvmem-syscon.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Marek Vasut <marex@denx.de>
> + *
> + * Based on snvs_lpgpr.c .
> + */
> +
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of_device.h>
> +#include <linux/regmap.h>
> +
> +struct nvmem_syscon_priv {
> +	struct device_d		*dev;
> +	struct regmap		*regmap;
> +	struct nvmem_config	cfg;
> +	unsigned int		off;
> +};
> +
> +static int nvmem_syscon_write(void *context, unsigned int offset, void *val,
> +			      size_t bytes)
> +{
> +	struct nvmem_syscon_priv *priv = context;
> +
> +	return regmap_bulk_write(priv->regmap, priv->off + offset,
> +				 val, bytes / 4);
> +}
> +
> +static int nvmem_syscon_read(void *context, unsigned int offset, void *val,
> +			     size_t bytes)
> +{
> +	struct nvmem_syscon_priv *priv = context;
> +
> +	return regmap_bulk_read(priv->regmap, priv->off + offset,
> +				val, bytes / 4);
> +}
> +
> +static int nvmem_syscon_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *node = dev->of_node;
> +	struct device_node *syscon_node;
> +	struct nvmem_syscon_priv *priv;
> +	struct nvmem_device *nvmem;
> +	struct nvmem_config *cfg;
> +	int ret;
> +
> +	if (!node)
> +		return -ENOENT;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32_index(node, "reg", 0, &priv->off);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u32_index(node, "reg", 1, &priv->cfg.size);
> +	if (ret)
> +		return ret;
> +
> +	syscon_node = of_get_parent(node);

This does not look correct. You hard-code dependency that it must be a
child of syscon node. This is weird requirement and not explained in the
bindings.

Why this cannot be then generic MMIO node? Why it has to be a child of
syscon?


> +	if (!syscon_node)
> +		return -ENODEV;
> +
> +	priv->regmap = syscon_node_to_regmap(syscon_node);
> +	of_node_put(syscon_node);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
Best regards,
Krzysztof