Re: [PATCH 6/6] eeprom: 93xx46: extend driver to plug into the NVMEM framework

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 08/12/15 14:05, Andrew Lunn wrote:
> Add a regmap for accessing the EEPROM, and then use that with the
> NVMEM framework. Use it backward compatibility register function, so
> that the 'eeprom' file in sys is provided by the framework.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> ---
>   drivers/misc/eeprom/Kconfig         |   3 +
>   drivers/misc/eeprom/eeprom_93xx46.c | 121 ++++++++++++++++++++++++++++--------
>   2 files changed, 98 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index ff3e1dd751ec..5dfa2c2c2277 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -80,6 +80,9 @@ config EEPROM_93CX6
>   config EEPROM_93XX46
>   	tristate "Microwire EEPROM 93XX46 support"
>   	depends on SPI && SYSFS
> +	select REGMAP
> +	select NVMEM
> +	select NVMEM_COMPAT
>   	help
>   	  Driver for the microwire EEPROM chipsets 93xx46x. The driver
>   	  supports both read and write commands and also the command to
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index ff63f05edc76..aa75f59abfba 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -15,7 +15,8 @@
>   #include <linux/mutex.h>
>   #include <linux/slab.h>
>   #include <linux/spi/spi.h>
> -#include <linux/sysfs.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/regmap.h>
>   #include <linux/eeprom_93xx46.h>
>
>   #define OP_START	0x4
> @@ -28,25 +29,29 @@
>   struct eeprom_93xx46_dev {
>   	struct spi_device *spi;
>   	struct eeprom_93xx46_platform_data *pdata;
> -	struct bin_attribute bin;
>   	struct mutex lock;
> +	struct regmap_config regmap_config;
> +	struct nvmem_config nvmem_config;
> +	struct nvmem_device *nvmem;
>   	int addrlen;
> +	int size;
>   };
>
>   static ssize_t
> -eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
> -		       struct bin_attribute *bin_attr,
> -		       char *buf, loff_t off, size_t count)
> +eeprom_93xx46_read(struct eeprom_93xx46_dev *edev, char *buf,
> +		   unsigned off, size_t count)
>   {
> -	struct eeprom_93xx46_dev *edev;
> -	struct device *dev;
>   	struct spi_message m;
>   	struct spi_transfer t[2];
>   	int bits, ret;
>   	u16 cmd_addr;
>
> -	dev = container_of(kobj, struct device, kobj);
> -	edev = dev_get_drvdata(dev);
> +	if (unlikely(off >= edev->size))
> +		return 0;
> +	if ((off + count) > edev->size)
> +		count = edev->size - off;
> +	if (unlikely(!count))
> +		return count;
>
>   	cmd_addr = OP_READ << edev->addrlen;
>
> @@ -94,6 +99,7 @@ eeprom_93xx46_bin_read(struct file *filp, struct kobject *kobj,
>   	return ret ? : count;
>   }
>
> +
>   static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
>   {
>   	struct spi_message m;
> @@ -182,16 +188,17 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
>   }
>
>   static ssize_t
> -eeprom_93xx46_bin_write(struct file *filp, struct kobject *kobj,
> -			struct bin_attribute *bin_attr,
> -			char *buf, loff_t off, size_t count)
> +eeprom_93xx46_write(struct eeprom_93xx46_dev *edev, const char *buf,
> +		    loff_t off, size_t count)
>   {
> -	struct eeprom_93xx46_dev *edev;
> -	struct device *dev;
>   	int i, ret, step = 1;
>
> -	dev = container_of(kobj, struct device, kobj);
> -	edev = dev_get_drvdata(dev);
> +	if (unlikely(off >= edev->size))
> +		return -EFBIG;
> +	if ((off + count) > edev->size)
> +		count = edev->size - off;
> +	if (unlikely(!count))
> +		return count;
>
>   	/* only write even number of bytes on 16-bit devices */
>   	if (edev->addrlen == 6) {
> @@ -228,6 +235,49 @@ eeprom_93xx46_bin_write(struct file *filp, struct kobject *kobj,
>   	return ret ? : count;
>   }
>
> +/*
> + * Provide a regmap interface, which is registered with the NVMEM
> + * framework
> +*/
> +static int eeprom_93xx46_regmap_read(void *context, const void *reg,
> +				     size_t reg_size, void *val,
> +				     size_t val_size)
> +{
> +	struct eeprom_93xx46_dev *eeprom_93xx46 = context;
> +	off_t offset = *(u32 *)reg;
> +	int err;
> +
> +	err = eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);
> +	if (err)
> +		return err;
> +	return 0;
> +}
> +
> +static int eeprom_93xx46_regmap_write(void *context, const void *data,
> +				      size_t count)
> +{
> +	struct eeprom_93xx46_dev *eeprom_93xx46 = context;
> +	const char *buf;
> +	u32 offset;
> +	size_t len;
> +	int err;
> +
> +	memcpy(&offset, data, sizeof(offset));
> +	buf = (const char *)data + sizeof(offset);
> +	len = count - sizeof(offset);
> +
> +	err = eeprom_93xx46_write(eeprom_93xx46, buf, offset, len);
> +	if (err)
> +		return err;
> +	return 0;
> +}
> +
> +static const struct regmap_bus eeprom_93xx46_regmap_bus = {
> +	.read = eeprom_93xx46_regmap_read,
> +	.write = eeprom_93xx46_regmap_write,
> +	.reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
> +};
> +
>   static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
>   {
>   	struct eeprom_93xx46_platform_data *pd = edev->pdata;
> @@ -298,6 +348,7 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>   {
>   	struct eeprom_93xx46_platform_data *pd;
>   	struct eeprom_93xx46_dev *edev;
> +	struct regmap *regmap;
>   	int err;
>
>   	pd = spi->dev.platform_data;
> @@ -325,19 +376,36 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>   	edev->spi = spi_dev_get(spi);
>   	edev->pdata = pd;
>
> -	sysfs_bin_attr_init(&edev->bin);
> -	edev->bin.attr.name = "eeprom";
> -	edev->bin.attr.mode = S_IRUSR;
> -	edev->bin.read = eeprom_93xx46_bin_read;
> -	edev->bin.size = 128;
> +	edev->size = 128;
> +
>   	if (!(pd->flags & EE_READONLY)) {
> -		edev->bin.write = eeprom_93xx46_bin_write;
> -		edev->bin.attr.mode |= S_IWUSR;
>   	}
>
> -	err = sysfs_create_bin_file(&spi->dev.kobj, &edev->bin);
> -	if (err)
> +	edev->regmap_config.reg_bits = 32;
> +	edev->regmap_config.val_bits = 8;
> +	edev->regmap_config.reg_stride = 1;
> +	edev->regmap_config.max_register = edev->size - 1;
> +
> +	regmap = devm_regmap_init(&spi->dev, &eeprom_93xx46_regmap_bus, edev,
> +				  &edev->regmap_config);
> +	if (IS_ERR(regmap)) {
> +		dev_err(&spi->dev, "regmap init failed\n");
> +		err = PTR_ERR(regmap);
>   		goto fail;
> +	}
> +
> +	edev->nvmem_config.name = dev_name(&spi->dev);
> +	edev->nvmem_config.dev = &spi->dev;
> +	edev->nvmem_config.read_only = pd->flags & EE_READONLY;
> +	edev->nvmem_config.root_only = true;
> +	edev->nvmem_config.owner = THIS_MODULE;
> +
> +	edev->nvmem = nvmem_register_compat(&edev->nvmem_config,
> +					    &spi->dev);

Is there a reason for this driver to be using the old style?
I can understand the issues with at24/at25 but does this driver also 
suffer from such issues?

IMO, If its possible we should  discourage the compat api as much we can.

> +	if (IS_ERR(edev->nvmem)) {
> +		err = PTR_ERR(edev->nvmem);
> +		goto fail;
> +	}
>
>   	dev_info(&spi->dev, "%d-bit eeprom %s\n",
>   		(pd->flags & EE_ADDR8) ? 8 : 16,
> @@ -359,10 +427,11 @@ static int eeprom_93xx46_remove(struct spi_device *spi)
>   {
>   	struct eeprom_93xx46_dev *edev = spi_get_drvdata(spi);
>
> +	nvmem_unregister(edev->nvmem);
> +
>   	if (!(edev->pdata->flags & EE_READONLY))
>   		device_remove_file(&spi->dev, &dev_attr_erase);
>
> -	sysfs_remove_bin_file(&spi->dev.kobj, &edev->bin);
>   	kfree(edev);
>   	return 0;
>   }
>