Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 2/2] nvmem: iomap: new driver exposing NVMEM accessible
 using I/O mapping
To:     =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <zajec5@gmail.com>,
        Rob Herring <robh+dt@kernel.org>
Cc:     devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mips@vger.kernel.org,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vivek Unune <npcomplete13@gmail.com>,
        bcm-kernel-feedback-list@broadcom.com,
        linux-kernel@vger.kernel.org,
        =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= <rafal@milecki.pl>
References: <20210304144132.24098-1-zajec5@gmail.com>
 <20210304144132.24098-2-zajec5@gmail.com>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <047bced8-6c20-4a0a-c7ea-e0ad83318461@linaro.org>
Date:   Fri, 5 Mar 2021 10:02:25 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20210304144132.24098-2-zajec5@gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 04/03/2021 14:41, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> This is a generic NVMEM access method used e.g. by Broadcom for their
> NVRAM on MIPS and Northstar devices.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>   drivers/nvmem/Kconfig  |  7 +++
>   drivers/nvmem/Makefile |  2 +
>   drivers/nvmem/iomap.c  | 99 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 108 insertions(+)
>   create mode 100644 drivers/nvmem/iomap.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 75d2594c16e1..3d5c5684685d 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -278,4 +278,11 @@ config NVMEM_RMEM
>   
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nvmem-rmem.
> +
> +config NVMEM_IOMAP
> +	tristate "I/O mapped NVMEM support"
> +	depends on HAS_IOMEM
> +	help
> +	  This driver supports NVMEM that can be accessed using I/O mapping.
> +
>   endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 5376b8e0dae5..88a3b6979c53 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -57,3 +57,5 @@ obj-$(CONFIG_SPRD_EFUSE)	+= nvmem_sprd_efuse.o
>   nvmem_sprd_efuse-y		:= sprd-efuse.o
>   obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
>   nvmem-rmem-y			:= rmem.o
> +obj-$(CONFIG_NVMEM_IOMAP)	+= nvmem_iomap.o
> +nvmem_iomap-y			:= iomap.o
> diff --git a/drivers/nvmem/iomap.c b/drivers/nvmem/iomap.c
> new file mode 100644
> index 000000000000..ab6b40858a64
> --- /dev/null
> +++ b/drivers/nvmem/iomap.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2021 Rafał Miłecki <rafal@milecki.pl>
> + */
> +
> +#include <linux/io.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +
> +struct iomap {
> +	struct device *dev;
> +	void __iomem *base;
> +};
> +
> +static int iomap_read(void *context, unsigned int offset, void *val,
> +		      size_t bytes)
> +{
> +	struct iomap *priv = context;
> +	u8 *src = priv->base + offset;
> +	u8 *dst = val;
> +	size_t tmp;
> +
> +	tmp = offset % 4;
> +	memcpy_fromio(dst, src, tmp);
> +	dst += tmp;
> +	src += tmp;
> +	bytes -= tmp;
> +
> +	tmp = rounddown(bytes, 4);
> +	__ioread32_copy(dst, src, tmp / 4);
> +	dst += tmp;
> +	src += tmp;
> +	bytes -= tmp;
> +
> +	memcpy_fromio(dst, src, bytes);
> +


You could just do this!

	while (bytes--)
		*val++ = readb(priv->base + offset + i++);


> +	return 0;
> +}
> +
> +static int iomap_probe(struct platform_device *pdev)
> +{
> +	struct nvmem_config config = {
> +		.name = "iomap",
> +		.reg_read = iomap_read,
> +	};
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct iomap *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->dev = dev;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);


> +	if (!res) {
> +		dev_err(dev, "Failed to get resource\n");
> +		return -ENODEV;
> +	}

This is redundant check and error message!

You can just do :
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
priv->base = devm_ioremap_resource(dev, res);


> +
> +	priv->base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->base)) {
> +		dev_err(dev, "Failed to map resource: %ld\n", PTR_ERR(priv->base));
This message is redundant!

> +		return PTR_ERR(priv->base);
> +	}


> +
> +	config.dev = dev;
> +	config.priv = priv;
> +	config.size = resource_size(res);
> +
> +	return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
> +}
> +
> +static const struct of_device_id iomap_of_match_table[] = {
> +	{ .compatible = "brcm,nvram", },
> +	{ .compatible = "nvmem-iomap", },

Generic compatible does not really makes sense at all, you can probably 
consider adding some generic functions rather than having a compatible, 
in case you want to reduce code duplication!


> +	{},
> +};
> +
> +static struct platform_driver iomap_driver = {
> +	.probe = iomap_probe,
> +	.driver = {
> +		.name = "nvmem_iomap",
> +		.of_match_table = iomap_of_match_table,
> +	},
> +};
> +
> +static int __init iomap_init(void)
> +{
> +	return platform_driver_register(&iomap_driver);
> +}
> +
> +subsys_initcall_sync(iomap_init);
> +
> +MODULE_AUTHOR("Rafał Miłecki");
> +MODULE_LICENSE("GPL v2");

it should be
MODULE_LICENSE("GPL");


> +MODULE_DEVICE_TABLE(of, iomap_of_match_table);
>