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

From: Joachim Eastwood
Date: Wed Feb 17 2016 - 17:03:02 EST


Hi Andrew,

On 17 February 2016 at 21:07, Andrew Lunn <andrew@xxxxxxx> wrote:
> Add a regmap for accessing the EEPROM, and then use that with the
> NVMEM framework. Enable backward compatibility in the MVMEM config

typo: MVMEM

> structure, so that the 'eeprom' file in sys is provided by the
> framework.
>
> Signed-off-by: Andrew Lunn <andrew@xxxxxxx>
> Acked-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> ---
> v5: Remove useless test.
> ---
> drivers/misc/eeprom/Kconfig | 2 +
> drivers/misc/eeprom/eeprom_93xx46.c | 121 ++++++++++++++++++++++++++++--------
> 2 files changed, 96 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index 8c43a222ae55..cfc493c2e30a 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -78,6 +78,8 @@ config EEPROM_93CX6
> config EEPROM_93XX46
> tristate "Microwire EEPROM 93XX46 support"
> depends on SPI && SYSFS
> + select REGMAP
> + select NVMEM
> 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..4dd9b5100f63 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;
> }
>
> +

Stray newline?

> 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;

Can be:
return eeprom_93xx46_read(eeprom_93xx46, val, offset, val_size);

Allows you to remove the 'err' variable also.

> +}
> +
> +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;

Same here.


regards,
Joachim Eastwood