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

From: Srinivas Kandagatla
Date: Tue Dec 15 2015 - 05:06:17 EST



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@xxxxxxx>
---
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;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/