Re: [PATCH v2] soc: at91: Add Atmel SFR SN (Serial Number) support

[<Tudor.Ambarus@microchip.com>]

Hi, Kamel,

On 10/03/2019 12:16 PM, Kamel Bouhara wrote:
> External E-Mail
> 
> 
> Add support to read SFR's read-only registers providing the SoC
> Serial Numbers (SN0+SN1) to userspace.
> 
> ~ #  hexdump -n 8  -e'"%d\n"' /sys/bus/nvmem/devices/atmel-sfr0/nvmem
> 959527243
> 371539274
> 
> Signed-off-by: Kamel Bouhara <kamel.bouhara@bootlin.com>
> ---
> Changes in v2:
>  - updated atmel-sfr.c to sfr.c in Makefile
>  - fixed missing drvdata struct definition
>  - added missing syscon header
> 
>  drivers/soc/atmel/Kconfig  |  11 ++++
>  drivers/soc/atmel/Makefile |   1 +
>  drivers/soc/atmel/sfr.c    | 116 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 128 insertions(+)
>  create mode 100644 drivers/soc/atmel/sfr.c
> 
> diff --git a/drivers/soc/atmel/Kconfig b/drivers/soc/atmel/Kconfig
> index 05528139b023..d61711cb9556 100644
> --- a/drivers/soc/atmel/Kconfig
> +++ b/drivers/soc/atmel/Kconfig
> @@ -5,3 +5,14 @@ config AT91_SOC_ID
>  	default ARCH_AT91
>  	help
>  	  Include support for the SoC bus on the Atmel ARM SoCs.
> +
> +config AT91_SOC_SFR
> +	tristate "Special Function Registers support"
> +	depends on ARCH_AT91 || COMPILE_TEST
> +	help
> +	  This is a driver for the Special Function Registers available on
> +	  Atmel SAMA5Dx SoCs, providing access to specific aspects of the
> +	  integrated memory, bridge implementations, proccessor etc.

typo: processor.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called sfr.
> diff --git a/drivers/soc/atmel/Makefile b/drivers/soc/atmel/Makefile
> index 7ca355d10553..d849a897cd77 100644
> --- a/drivers/soc/atmel/Makefile
> +++ b/drivers/soc/atmel/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_AT91_SOC_ID) += soc.o
> +obj-$(CONFIG_AT91_SOC_SFR) += sfr.o
> diff --git a/drivers/soc/atmel/sfr.c b/drivers/soc/atmel/sfr.c
> new file mode 100644
> index 000000000000..2be306157283
> --- /dev/null
> +++ b/drivers/soc/atmel/sfr.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sfr.c - driver for special function registers
> + *
> + * Copyright (C) 2019 Bootlin.
> + *
> + */
> +#include <linux/device.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define SFR_SN0		0x4c
> +
> +struct sfr_priv {

how about renaming this to struct atmel_sfr_priv?

> +	struct device			*dev;

this is not used, maybe you can drop it?

> +	struct regmap			*regmap;
> +	struct nvmem_config		*config;
> +	struct atmel_sfr_drvdata	*drvdata;

this is not used, maybe you can drop it?

> +};
> +
> +struct atmel_sfr_drvdata {
> +	unsigned int nregs;
> +};
> +
> +static int atmel_sfr_read(void *context, unsigned int offset,
> +			  void *buf, size_t bytes)
> +{
> +	struct sfr_priv *priv = context;
> +	struct atmel_sfr_drvdata *drvdata = priv->drvdata;

looks like you can drop the *drvdata local variable.
> +
> +	return regmap_bulk_read(priv->regmap, SFR_SN0 + offset,
> +				buf, bytes / 4);
> +}
> +
> +static struct nvmem_config atmel_sfr_nvmem_config = {
> +	.name = "atmel-sfr",
> +	.read_only = true,
> +	.word_size = 4,
> +	.stride = 4,
> +	.reg_read = atmel_sfr_read,
> +};
> +
> +static int atmel_sfr_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct sfr_priv *priv;
> +	struct nvmem_device *nvmem;
> +	const struct atmel_sfr_drvdata *drvdata;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);

zeroing should not be needed since you update all the priv fields below.

> +	if (!priv)
> +		return -ENOMEM;
> +
> +	drvdata = of_device_get_match_data(dev);
> +	if (!drvdata)
> +		return -EINVAL;
> +
> +	priv->regmap = syscon_node_to_regmap(np);
> +	if (IS_ERR(priv->regmap)) {
> +		dev_err(dev, "cannot get parent's regmap\n");
> +		return PTR_ERR(priv->regmap);
> +	}
> +
> +	priv->drvdata = drvdata;

not needed?

Cheers,
ta

> +
> +	atmel_sfr_nvmem_config.size = drvdata->nregs;
> +	atmel_sfr_nvmem_config.dev = dev;
> +	atmel_sfr_nvmem_config.priv = priv;
> +
> +	priv->config = &atmel_sfr_nvmem_config;
> +
> +	nvmem = devm_nvmem_register(dev, &atmel_sfr_nvmem_config);
> +	if (IS_ERR(nvmem)) {
> +		dev_err(dev, "error registering nvmem config\n");
> +		return PTR_ERR(nvmem);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct atmel_sfr_drvdata sama5d2_sama5d4_drvdata = {
> +	.nregs = 2 * 4,
> +};
> +
> +static const struct of_device_id atmel_sfr_dt_ids[] = {
> +	{
> +		.compatible = "atmel,sama5d2-sfr",
> +		.data = &sama5d2_sama5d4_drvdata,
> +	}, {
> +		.compatible = "atmel,sama5d4-sfr",
> +		.data = &sama5d2_sama5d4_drvdata,
> +	}, {
> +		/* sentinel */
> +	},
> +};
> +MODULE_DEVICE_TABLE(of, atmel_sfr_dt_ids);
> +
> +static struct platform_driver atmel_sfr_driver = {
> +	.probe = atmel_sfr_probe,
> +	.driver = {
> +		.name = "atmel-sfr",
> +		.of_match_table = atmel_sfr_dt_ids,
> +	},
> +};
> +module_platform_driver(atmel_sfr_driver);
> +
> +MODULE_AUTHOR("Kamel Bouhara <kamel.bouhara@bootlin.com>");
> +MODULE_DESCRIPTION("Atmel SFR SN driver for SAMA5D2/4 SoC family");
> +MODULE_LICENSE("GPL v2");
> +
> --
> 2.23.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel