For DT and in-kernel users there is no interface to the
at24 EEPROMs so provide an EEPROM framework interface.
This allows us to use AT24 based EEPROMs and reference them
from within the DT tree.
Signed-off-by: Pantelis Antoniou <[email protected]>
---
drivers/misc/eeprom/at24.c | 222 +++++++++++++++++++++++++++++++++++----------
1 file changed, 174 insertions(+), 48 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 2d3db81..afa719a 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -3,6 +3,7 @@
*
* Copyright (C) 2005-2007 David Brownell
* Copyright (C) 2008 Wolfram Sang, Pengutronix
+ * Copyright (C) 2015 Pantelis Antoniou, Konsulko Group
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
@@ -23,6 +24,9 @@
#include <linux/of.h>
#include <linux/i2c.h>
#include <linux/platform_data/at24.h>
+#include <linux/regmap.h>
+#include <linux/eeprom-provider.h>
+#include <linux/io.h>
/*
* I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
@@ -63,12 +67,16 @@ struct at24_data {
* but not from changes by other I2C masters.
*/
struct mutex lock;
- struct bin_attribute bin;
u8 *writebuf;
unsigned write_max;
unsigned num_addresses;
+ struct regmap_config *regmap_config;
+ struct regmap *regmap;
+ struct eeprom_config *eeprom_config;
+ struct eeprom_device *eeprom_dev;
+
/*
* Some chips tie up multiple I2C addresses; dummy devices reserve
* them for us, and we'll use them with SMBus calls.
@@ -131,6 +139,8 @@ static const struct i2c_device_id at24_ids[] = {
};
MODULE_DEVICE_TABLE(i2c, at24_ids);
+static DEFINE_IDA(at24_ida);
+
/*-------------------------------------------------------------------------*/
/*
@@ -301,17 +311,6 @@ static ssize_t at24_read(struct at24_data *at24,
return retval;
}
-static ssize_t at24_bin_read(struct file *filp, struct kobject *kobj,
- struct bin_attribute *attr,
- char *buf, loff_t off, size_t count)
-{
- struct at24_data *at24;
-
- at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
- return at24_read(at24, buf, off, count);
-}
-
-
/*
* Note that if the hardware write-protect pin is pulled high, the whole
* chip is normally write protected. But there are plenty of product
@@ -432,21 +431,10 @@ static ssize_t at24_write(struct at24_data *at24, const char *buf, loff_t off,
return retval;
}
-static ssize_t at24_bin_write(struct file *filp, struct kobject *kobj,
- struct bin_attribute *attr,
- char *buf, loff_t off, size_t count)
-{
- struct at24_data *at24;
-
- if (unlikely(off >= attr->size))
- return -EFBIG;
-
- at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
- return at24_write(at24, buf, off, count);
-}
-
/*-------------------------------------------------------------------------*/
+/* panto: using the EEPROM framework macc is superfluous */
+
/*
* This lets other kernel code access the eeprom data. For example, it
* might hold a board's Ethernet address, or board-specific calibration
@@ -492,6 +480,91 @@ static void at24_get_ofdata(struct i2c_client *client,
{ }
#endif /* CONFIG_OF */
+static int regmap_at24_read(void *context,
+ const void *reg, size_t reg_size,
+ void *val, size_t val_size)
+{
+ struct i2c_client *client = context;
+ struct at24_data *at24;
+ unsigned int offset;
+ int ret;
+
+ /* reg bits is hardcoded to 32 bits */
+ BUG_ON(reg_size != 4);
+ offset = __raw_readl(reg);
+
+ at24 = i2c_get_clientdata(client);
+ if (at24 == NULL)
+ return -ENODEV;
+
+ /* val bytes is always 1 */
+ BUG_ON(at24->regmap_config->val_bits != 8);
+
+ ret = at24_read(at24, val, offset, val_size);
+ if (ret < 0)
+ return ret;
+ if (ret != val_size)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int regmap_at24_gather_write(void *context,
+ const void *reg, size_t reg_size,
+ const void *val, size_t val_size)
+{
+ struct i2c_client *client = context;
+ struct at24_data *at24;
+ unsigned int offset;
+ int ret;
+
+ at24 = i2c_get_clientdata(client);
+ if (at24 == NULL)
+ return -ENODEV;
+
+ BUG_ON(reg_size != 4);
+ offset = __raw_readl(reg);
+
+ /* val bytes is always 1 */
+ BUG_ON(at24->regmap_config->val_bits != 8);
+
+ ret = at24_write(at24, val, offset, val_size);
+ if (ret < 0)
+ return ret;
+ if (ret != val_size)
+ return -EINVAL;
+
+ return 0;
+}
+
+static int regmap_at24_write(void *context, const void *data, size_t count)
+{
+ struct i2c_client *client = context;
+ struct at24_data *at24;
+ unsigned int reg_bytes, offset;
+
+ at24 = i2c_get_clientdata(client);
+ if (at24 == NULL)
+ return -ENODEV;
+
+ reg_bytes = at24->regmap_config->reg_bits / 8;
+ offset = reg_bytes;
+
+ BUG_ON(reg_bytes != 4);
+ BUG_ON(count <= offset);
+
+ return regmap_at24_gather_write(context, data, reg_bytes,
+ data + offset, count - offset);
+}
+
+static struct regmap_bus regmap_at24_bus = {
+ .read = regmap_at24_read,
+ .write = regmap_at24_write,
+ .gather_write = regmap_at24_gather_write,
+ .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
+ .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
+};
+
static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
struct at24_platform_data chip;
@@ -502,6 +575,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
int err;
unsigned i, num_addresses;
kernel_ulong_t magic;
+ struct regmap_config *regmap_config;
+ struct regmap *regmap;
+ struct eeprom_config *eeprom_config;
+ struct eeprom_device *eeprom_dev;
if (client->dev.platform_data) {
chip = *(struct at24_platform_data *)client->dev.platform_data;
@@ -569,16 +646,75 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
}
}
+ regmap_config = devm_kzalloc(&client->dev, sizeof(*regmap_config),
+ GFP_KERNEL);
+ if (IS_ERR(regmap_config)) {
+ err = PTR_ERR(regmap_config);
+ dev_err(&client->dev, "%s: regmap_config allocation failed (%d)\n",
+ __func__, err);
+ return err;
+ }
+
+ /* use 32 bits as registers, they don't appear on the wire anyway */
+ regmap_config->reg_bits = 32;
+ regmap_config->val_bits = 8;
+ regmap_config->cache_type = REGCACHE_NONE;
+ regmap_config->max_register = chip.byte_len;
+
if (chip.flags & AT24_FLAG_TAKE8ADDR)
num_addresses = 8;
else
num_addresses = DIV_ROUND_UP(chip.byte_len,
(chip.flags & AT24_FLAG_ADDR16) ? 65536 : 256);
+ /* we can't use devm_regmap_init_i2c due to the many i2c clients */
+ regmap = devm_regmap_init(&client->dev, ®map_at24_bus,
+ client, regmap_config);
+ if (IS_ERR(regmap)) {
+ err = PTR_ERR(regmap);
+ dev_err(&client->dev, "%s: regmap allocation failed (%d)\n",
+ __func__, err);
+ return err;
+ }
+
+ eeprom_config = devm_kzalloc(&client->dev, sizeof(*eeprom_config),
+ GFP_KERNEL);
+ if (IS_ERR(eeprom_config)) {
+ err = PTR_ERR(eeprom_config);
+ dev_err(&client->dev, "%s: eeprom_config allocation failed (%d)\n",
+ __func__, err);
+ return err;
+ }
+ eeprom_config->dev = &client->dev;
+ eeprom_config->name = "at24-";
+ eeprom_config->id = ida_simple_get(&at24_ida, 0, 0, GFP_KERNEL);
+ eeprom_config->owner = THIS_MODULE;
+ if (eeprom_config->id < 0) {
+ err = eeprom_config->id;
+ dev_err(&client->dev, "%s: eeprom id allocation failed (%d)\n",
+ __func__, err);
+ return err;
+ }
+
+ eeprom_dev = eeprom_register(eeprom_config);
+ if (IS_ERR(eeprom_dev)) {
+ err = PTR_ERR(eeprom_dev);
+ dev_err(&client->dev, "%s: eeprom_register failed (%d)\n",
+ __func__, err);
+ goto err_out;
+ }
+
at24 = devm_kzalloc(&client->dev, sizeof(struct at24_data) +
num_addresses * sizeof(struct i2c_client *), GFP_KERNEL);
- if (!at24)
- return -ENOMEM;
+ if (!at24) {
+ err = -ENOMEM;
+ goto err_out;
+ }
+
+ at24->regmap = regmap;
+ at24->regmap_config = regmap_config;
+ at24->eeprom_config = eeprom_config;
+ at24->eeprom_dev = eeprom_dev;
mutex_init(&at24->lock);
at24->use_smbus = use_smbus;
@@ -586,16 +722,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
at24->chip = chip;
at24->num_addresses = num_addresses;
- /*
- * Export the EEPROM bytes through sysfs, since that's convenient.
- * By default, only root should see the data (maybe passwords etc)
- */
- sysfs_bin_attr_init(&at24->bin);
- at24->bin.attr.name = "eeprom";
- at24->bin.attr.mode = chip.flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
- at24->bin.read = at24_bin_read;
- at24->bin.size = chip.byte_len;
-
at24->macc.read = at24_macc_read;
writable = !(chip.flags & AT24_FLAG_READONLY);
@@ -606,9 +732,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
at24->macc.write = at24_macc_write;
- at24->bin.write = at24_bin_write;
- at24->bin.attr.mode |= S_IWUSR;
-
if (write_max > io_limit)
write_max = io_limit;
if (use_smbus && write_max > I2C_SMBUS_BLOCK_MAX)
@@ -618,8 +741,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
/* buffer (data + address at the beginning) */
at24->writebuf = devm_kzalloc(&client->dev,
write_max + 2, GFP_KERNEL);
- if (!at24->writebuf)
- return -ENOMEM;
+ if (!at24->writebuf) {
+ err = -ENOMEM;
+ goto err_out;
+ }
} else {
dev_warn(&client->dev,
"cannot write due to controller restrictions.");
@@ -640,14 +765,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
}
}
- err = sysfs_create_bin_file(&client->dev.kobj, &at24->bin);
- if (err)
- goto err_clients;
-
i2c_set_clientdata(client, at24);
dev_info(&client->dev, "%zu byte %s EEPROM, %s, %u bytes/write\n",
- at24->bin.size, client->name,
+ at24->chip.byte_len, client->name,
writable ? "writable" : "read-only", at24->write_max);
if (use_smbus == I2C_SMBUS_WORD_DATA ||
use_smbus == I2C_SMBUS_BYTE_DATA) {
@@ -663,10 +784,14 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
return 0;
err_clients:
+
for (i = 1; i < num_addresses; i++)
if (at24->client[i])
i2c_unregister_device(at24->client[i]);
+err_out:
+ ida_simple_remove(&at24_ida, eeprom_config->id);
+
return err;
}
@@ -676,11 +801,12 @@ static int at24_remove(struct i2c_client *client)
int i;
at24 = i2c_get_clientdata(client);
- sysfs_remove_bin_file(&client->dev.kobj, &at24->bin);
for (i = 1; i < at24->num_addresses; i++)
i2c_unregister_device(at24->client[i]);
+ eeprom_unregister(at24->eeprom_dev);
+
return 0;
}
--
1.7.12
Hi Pantelis,
On Wed, May 13, 2015 at 10:38:17AM +0300, Pantelis Antoniou wrote:
> For DT and in-kernel users there is no interface to the
> at24 EEPROMs so provide an EEPROM framework interface.
>
> This allows us to use AT24 based EEPROMs and reference them
> from within the DT tree.
>
> Signed-off-by: Pantelis Antoniou <[email protected]>
You should probably mention that the EEPROM framework hasn't been
merged yet.
> ---
> drivers/misc/eeprom/at24.c | 222 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 174 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 2d3db81..afa719a 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -3,6 +3,7 @@
> *
> * Copyright (C) 2005-2007 David Brownell
> * Copyright (C) 2008 Wolfram Sang, Pengutronix
> + * Copyright (C) 2015 Pantelis Antoniou, Konsulko Group
> *
> * This program is free software; you can redistribute it and/or modify
> * it under the terms of the GNU General Public License as published by
> @@ -23,6 +24,9 @@
> #include <linux/of.h>
> #include <linux/i2c.h>
> #include <linux/platform_data/at24.h>
> +#include <linux/regmap.h>
> +#include <linux/eeprom-provider.h>
> +#include <linux/io.h>
>
> /*
> * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
> @@ -63,12 +67,16 @@ struct at24_data {
> * but not from changes by other I2C masters.
> */
> struct mutex lock;
> - struct bin_attribute bin;
>
> u8 *writebuf;
> unsigned write_max;
> unsigned num_addresses;
>
> + struct regmap_config *regmap_config;
> + struct regmap *regmap;
> + struct eeprom_config *eeprom_config;
> + struct eeprom_device *eeprom_dev;
> +
> /*
> * Some chips tie up multiple I2C addresses; dummy devices reserve
> * them for us, and we'll use them with SMBus calls.
> @@ -131,6 +139,8 @@ static const struct i2c_device_id at24_ids[] = {
> };
> MODULE_DEVICE_TABLE(i2c, at24_ids);
>
> +static DEFINE_IDA(at24_ida);
> +
> /*-------------------------------------------------------------------------*/
>
> /*
> @@ -301,17 +311,6 @@ static ssize_t at24_read(struct at24_data *at24,
> return retval;
> }
>
> -static ssize_t at24_bin_read(struct file *filp, struct kobject *kobj,
> - struct bin_attribute *attr,
> - char *buf, loff_t off, size_t count)
> -{
> - struct at24_data *at24;
> -
> - at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
> - return at24_read(at24, buf, off, count);
> -}
> -
> -
> /*
> * Note that if the hardware write-protect pin is pulled high, the whole
> * chip is normally write protected. But there are plenty of product
> @@ -432,21 +431,10 @@ static ssize_t at24_write(struct at24_data *at24, const char *buf, loff_t off,
> return retval;
> }
>
> -static ssize_t at24_bin_write(struct file *filp, struct kobject *kobj,
> - struct bin_attribute *attr,
> - char *buf, loff_t off, size_t count)
> -{
> - struct at24_data *at24;
> -
> - if (unlikely(off >= attr->size))
> - return -EFBIG;
> -
> - at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
> - return at24_write(at24, buf, off, count);
> -}
> -
> /*-------------------------------------------------------------------------*/
>
> +/* panto: using the EEPROM framework macc is superfluous */
> +
Is it? it was kind of one of the point to be able to remove the memory
accessors stuff (which is only used by that driver btw).
> /*
> * This lets other kernel code access the eeprom data. For example, it
> * might hold a board's Ethernet address, or board-specific calibration
> @@ -492,6 +480,91 @@ static void at24_get_ofdata(struct i2c_client *client,
> { }
> #endif /* CONFIG_OF */
>
> +static int regmap_at24_read(void *context,
> + const void *reg, size_t reg_size,
> + void *val, size_t val_size)
> +{
> + struct i2c_client *client = context;
> + struct at24_data *at24;
> + unsigned int offset;
> + int ret;
> +
> + /* reg bits is hardcoded to 32 bits */
> + BUG_ON(reg_size != 4);
> + offset = __raw_readl(reg);
> +
> + at24 = i2c_get_clientdata(client);
> + if (at24 == NULL)
> + return -ENODEV;
> +
> + /* val bytes is always 1 */
> + BUG_ON(at24->regmap_config->val_bits != 8);
> +
> + ret = at24_read(at24, val, offset, val_size);
> + if (ret < 0)
> + return ret;
> + if (ret != val_size)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int regmap_at24_gather_write(void *context,
> + const void *reg, size_t reg_size,
> + const void *val, size_t val_size)
> +{
> + struct i2c_client *client = context;
> + struct at24_data *at24;
> + unsigned int offset;
> + int ret;
> +
> + at24 = i2c_get_clientdata(client);
> + if (at24 == NULL)
> + return -ENODEV;
> +
> + BUG_ON(reg_size != 4);
> + offset = __raw_readl(reg);
> +
> + /* val bytes is always 1 */
> + BUG_ON(at24->regmap_config->val_bits != 8);
> +
> + ret = at24_write(at24, val, offset, val_size);
> + if (ret < 0)
> + return ret;
> + if (ret != val_size)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int regmap_at24_write(void *context, const void *data, size_t count)
> +{
> + struct i2c_client *client = context;
> + struct at24_data *at24;
> + unsigned int reg_bytes, offset;
> +
> + at24 = i2c_get_clientdata(client);
> + if (at24 == NULL)
> + return -ENODEV;
> +
> + reg_bytes = at24->regmap_config->reg_bits / 8;
> + offset = reg_bytes;
> +
> + BUG_ON(reg_bytes != 4);
> + BUG_ON(count <= offset);
> +
> + return regmap_at24_gather_write(context, data, reg_bytes,
> + data + offset, count - offset);
> +}
> +
> +static struct regmap_bus regmap_at24_bus = {
> + .read = regmap_at24_read,
> + .write = regmap_at24_write,
> + .gather_write = regmap_at24_gather_write,
> + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
> + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
I wouldn't expect the content of the EEPROM to change of endianness
when we change the kernel's.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
> On May 13, 2015, at 23:36 , Maxime Ripard <[email protected]> wrote:
>
> Hi Pantelis,
>
> On Wed, May 13, 2015 at 10:38:17AM +0300, Pantelis Antoniou wrote:
>> For DT and in-kernel users there is no interface to the
>> at24 EEPROMs so provide an EEPROM framework interface.
>>
>> This allows us to use AT24 based EEPROMs and reference them
>> from within the DT tree.
>>
>> Signed-off-by: Pantelis Antoniou <[email protected]>
>
> You should probably mention that the EEPROM framework hasn't been
> merged yet.
>
Well, that’s true. But it’s on version 4 and it does do what I need it
to do, namely provide a DT interface for in-kernel users.
>> ---
>> drivers/misc/eeprom/at24.c | 222 +++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 174 insertions(+), 48 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index 2d3db81..afa719a 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -3,6 +3,7 @@
>> *
>> * Copyright (C) 2005-2007 David Brownell
>> * Copyright (C) 2008 Wolfram Sang, Pengutronix
>> + * Copyright (C) 2015 Pantelis Antoniou, Konsulko Group
>> *
>> * This program is free software; you can redistribute it and/or modify
>> * it under the terms of the GNU General Public License as published by
>> @@ -23,6 +24,9 @@
>> #include <linux/of.h>
>> #include <linux/i2c.h>
>> #include <linux/platform_data/at24.h>
>> +#include <linux/regmap.h>
>> +#include <linux/eeprom-provider.h>
>> +#include <linux/io.h>
>>
>> /*
>> * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
>> @@ -63,12 +67,16 @@ struct at24_data {
>> * but not from changes by other I2C masters.
>> */
>> struct mutex lock;
>> - struct bin_attribute bin;
>>
>> u8 *writebuf;
>> unsigned write_max;
>> unsigned num_addresses;
>>
>> + struct regmap_config *regmap_config;
>> + struct regmap *regmap;
>> + struct eeprom_config *eeprom_config;
>> + struct eeprom_device *eeprom_dev;
>> +
>> /*
>> * Some chips tie up multiple I2C addresses; dummy devices reserve
>> * them for us, and we'll use them with SMBus calls.
>> @@ -131,6 +139,8 @@ static const struct i2c_device_id at24_ids[] = {
>> };
>> MODULE_DEVICE_TABLE(i2c, at24_ids);
>>
>> +static DEFINE_IDA(at24_ida);
>> +
>> /*-------------------------------------------------------------------------*/
>>
>> /*
>> @@ -301,17 +311,6 @@ static ssize_t at24_read(struct at24_data *at24,
>> return retval;
>> }
>>
>> -static ssize_t at24_bin_read(struct file *filp, struct kobject *kobj,
>> - struct bin_attribute *attr,
>> - char *buf, loff_t off, size_t count)
>> -{
>> - struct at24_data *at24;
>> -
>> - at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
>> - return at24_read(at24, buf, off, count);
>> -}
>> -
>> -
>> /*
>> * Note that if the hardware write-protect pin is pulled high, the whole
>> * chip is normally write protected. But there are plenty of product
>> @@ -432,21 +431,10 @@ static ssize_t at24_write(struct at24_data *at24, const char *buf, loff_t off,
>> return retval;
>> }
>>
>> -static ssize_t at24_bin_write(struct file *filp, struct kobject *kobj,
>> - struct bin_attribute *attr,
>> - char *buf, loff_t off, size_t count)
>> -{
>> - struct at24_data *at24;
>> -
>> - if (unlikely(off >= attr->size))
>> - return -EFBIG;
>> -
>> - at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
>> - return at24_write(at24, buf, off, count);
>> -}
>> -
>> /*-------------------------------------------------------------------------*/
>>
>> +/* panto: using the EEPROM framework macc is superfluous */
>> +
>
> Is it? it was kind of one of the point to be able to remove the memory
> accessors stuff (which is only used by that driver btw).
>
Yes it completely redundant now. But it’s not my call to remove it, that would
be Wolfram.
>> /*
>> * This lets other kernel code access the eeprom data. For example, it
>> * might hold a board's Ethernet address, or board-specific calibration
>> @@ -492,6 +480,91 @@ static void at24_get_ofdata(struct i2c_client *client,
>> { }
>> #endif /* CONFIG_OF */
>>
>> +static int regmap_at24_read(void *context,
>> + const void *reg, size_t reg_size,
>> + void *val, size_t val_size)
>> +{
>> + struct i2c_client *client = context;
>> + struct at24_data *at24;
>> + unsigned int offset;
>> + int ret;
>> +
>> + /* reg bits is hardcoded to 32 bits */
>> + BUG_ON(reg_size != 4);
>> + offset = __raw_readl(reg);
>> +
>> + at24 = i2c_get_clientdata(client);
>> + if (at24 == NULL)
>> + return -ENODEV;
>> +
>> + /* val bytes is always 1 */
>> + BUG_ON(at24->regmap_config->val_bits != 8);
>> +
>> + ret = at24_read(at24, val, offset, val_size);
>> + if (ret < 0)
>> + return ret;
>> + if (ret != val_size)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int regmap_at24_gather_write(void *context,
>> + const void *reg, size_t reg_size,
>> + const void *val, size_t val_size)
>> +{
>> + struct i2c_client *client = context;
>> + struct at24_data *at24;
>> + unsigned int offset;
>> + int ret;
>> +
>> + at24 = i2c_get_clientdata(client);
>> + if (at24 == NULL)
>> + return -ENODEV;
>> +
>> + BUG_ON(reg_size != 4);
>> + offset = __raw_readl(reg);
>> +
>> + /* val bytes is always 1 */
>> + BUG_ON(at24->regmap_config->val_bits != 8);
>> +
>> + ret = at24_write(at24, val, offset, val_size);
>> + if (ret < 0)
>> + return ret;
>> + if (ret != val_size)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int regmap_at24_write(void *context, const void *data, size_t count)
>> +{
>> + struct i2c_client *client = context;
>> + struct at24_data *at24;
>> + unsigned int reg_bytes, offset;
>> +
>> + at24 = i2c_get_clientdata(client);
>> + if (at24 == NULL)
>> + return -ENODEV;
>> +
>> + reg_bytes = at24->regmap_config->reg_bits / 8;
>> + offset = reg_bytes;
>> +
>> + BUG_ON(reg_bytes != 4);
>> + BUG_ON(count <= offset);
>> +
>> + return regmap_at24_gather_write(context, data, reg_bytes,
>> + data + offset, count - offset);
>> +}
>> +
>> +static struct regmap_bus regmap_at24_bus = {
>> + .read = regmap_at24_read,
>> + .write = regmap_at24_write,
>> + .gather_write = regmap_at24_gather_write,
>> + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
>> + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
>
> I wouldn't expect the content of the EEPROM to change of endianness
> when we change the kernel’s
It doesn’t matter really; these EEPROMs are byte accessible, and the register
values are completely internal to the driver.
The native format saves a few cycles converting in case you run on a wrong endian
architecture.
> .
>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
Regards
— Pantelis
On Thu, May 14, 2015 at 09:06:23AM +0300, Pantelis Antoniou wrote:
>
> > On May 13, 2015, at 23:36 , Maxime Ripard <[email protected]> wrote:
> >
> > Hi Pantelis,
> >
> > On Wed, May 13, 2015 at 10:38:17AM +0300, Pantelis Antoniou wrote:
> >> For DT and in-kernel users there is no interface to the
> >> at24 EEPROMs so provide an EEPROM framework interface.
> >>
> >> This allows us to use AT24 based EEPROMs and reference them
> >> from within the DT tree.
> >>
> >> Signed-off-by: Pantelis Antoniou <[email protected]>
> >
> > You should probably mention that the EEPROM framework hasn't been
> > merged yet.
> >
>
> Well, that’s true. But it’s on version 4 and it does do what I need it
> to do, namely provide a DT interface for in-kernel users.
I was just meaning that people not aware of the fact that the
framework was not merged might apply it, and that would result in an
instant breakage, that's all.
It's actually pretty nice that you convert this driver over to the
EEPROM framework.
>
> >> ---
> >> drivers/misc/eeprom/at24.c | 222 +++++++++++++++++++++++++++++++++++----------
> >> 1 file changed, 174 insertions(+), 48 deletions(-)
> >>
> >> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> >> index 2d3db81..afa719a 100644
> >> --- a/drivers/misc/eeprom/at24.c
> >> +++ b/drivers/misc/eeprom/at24.c
> >> @@ -3,6 +3,7 @@
> >> *
> >> * Copyright (C) 2005-2007 David Brownell
> >> * Copyright (C) 2008 Wolfram Sang, Pengutronix
> >> + * Copyright (C) 2015 Pantelis Antoniou, Konsulko Group
> >> *
> >> * This program is free software; you can redistribute it and/or modify
> >> * it under the terms of the GNU General Public License as published by
> >> @@ -23,6 +24,9 @@
> >> #include <linux/of.h>
> >> #include <linux/i2c.h>
> >> #include <linux/platform_data/at24.h>
> >> +#include <linux/regmap.h>
> >> +#include <linux/eeprom-provider.h>
> >> +#include <linux/io.h>
> >>
> >> /*
> >> * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
> >> @@ -63,12 +67,16 @@ struct at24_data {
> >> * but not from changes by other I2C masters.
> >> */
> >> struct mutex lock;
> >> - struct bin_attribute bin;
> >>
> >> u8 *writebuf;
> >> unsigned write_max;
> >> unsigned num_addresses;
> >>
> >> + struct regmap_config *regmap_config;
> >> + struct regmap *regmap;
> >> + struct eeprom_config *eeprom_config;
> >> + struct eeprom_device *eeprom_dev;
> >> +
> >> /*
> >> * Some chips tie up multiple I2C addresses; dummy devices reserve
> >> * them for us, and we'll use them with SMBus calls.
> >> @@ -131,6 +139,8 @@ static const struct i2c_device_id at24_ids[] = {
> >> };
> >> MODULE_DEVICE_TABLE(i2c, at24_ids);
> >>
> >> +static DEFINE_IDA(at24_ida);
> >> +
> >> /*-------------------------------------------------------------------------*/
> >>
> >> /*
> >> @@ -301,17 +311,6 @@ static ssize_t at24_read(struct at24_data *at24,
> >> return retval;
> >> }
> >>
> >> -static ssize_t at24_bin_read(struct file *filp, struct kobject *kobj,
> >> - struct bin_attribute *attr,
> >> - char *buf, loff_t off, size_t count)
> >> -{
> >> - struct at24_data *at24;
> >> -
> >> - at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
> >> - return at24_read(at24, buf, off, count);
> >> -}
> >> -
> >> -
> >> /*
> >> * Note that if the hardware write-protect pin is pulled high, the whole
> >> * chip is normally write protected. But there are plenty of product
> >> @@ -432,21 +431,10 @@ static ssize_t at24_write(struct at24_data *at24, const char *buf, loff_t off,
> >> return retval;
> >> }
> >>
> >> -static ssize_t at24_bin_write(struct file *filp, struct kobject *kobj,
> >> - struct bin_attribute *attr,
> >> - char *buf, loff_t off, size_t count)
> >> -{
> >> - struct at24_data *at24;
> >> -
> >> - if (unlikely(off >= attr->size))
> >> - return -EFBIG;
> >> -
> >> - at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
> >> - return at24_write(at24, buf, off, count);
> >> -}
> >> -
> >> /*-------------------------------------------------------------------------*/
> >>
> >> +/* panto: using the EEPROM framework macc is superfluous */
> >> +
> >
> > Is it? it was kind of one of the point to be able to remove the memory
> > accessors stuff (which is only used by that driver btw).
>
> Yes it completely redundant now. But it’s not my call to remove it, that would
> be Wolfram.
There's still a user of that stuff though, some davinci board iirc,
that would need to port that user to the EEPROM framework too.
> >> /*
> >> * This lets other kernel code access the eeprom data. For example, it
> >> * might hold a board's Ethernet address, or board-specific calibration
> >> @@ -492,6 +480,91 @@ static void at24_get_ofdata(struct i2c_client *client,
> >> { }
> >> #endif /* CONFIG_OF */
> >>
> >> +static int regmap_at24_read(void *context,
> >> + const void *reg, size_t reg_size,
> >> + void *val, size_t val_size)
> >> +{
> >> + struct i2c_client *client = context;
> >> + struct at24_data *at24;
> >> + unsigned int offset;
> >> + int ret;
> >> +
> >> + /* reg bits is hardcoded to 32 bits */
> >> + BUG_ON(reg_size != 4);
> >> + offset = __raw_readl(reg);
> >> +
> >> + at24 = i2c_get_clientdata(client);
> >> + if (at24 == NULL)
> >> + return -ENODEV;
> >> +
> >> + /* val bytes is always 1 */
> >> + BUG_ON(at24->regmap_config->val_bits != 8);
> >> +
> >> + ret = at24_read(at24, val, offset, val_size);
> >> + if (ret < 0)
> >> + return ret;
> >> + if (ret != val_size)
> >> + return -EINVAL;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int regmap_at24_gather_write(void *context,
> >> + const void *reg, size_t reg_size,
> >> + const void *val, size_t val_size)
> >> +{
> >> + struct i2c_client *client = context;
> >> + struct at24_data *at24;
> >> + unsigned int offset;
> >> + int ret;
> >> +
> >> + at24 = i2c_get_clientdata(client);
> >> + if (at24 == NULL)
> >> + return -ENODEV;
> >> +
> >> + BUG_ON(reg_size != 4);
> >> + offset = __raw_readl(reg);
> >> +
> >> + /* val bytes is always 1 */
> >> + BUG_ON(at24->regmap_config->val_bits != 8);
> >> +
> >> + ret = at24_write(at24, val, offset, val_size);
> >> + if (ret < 0)
> >> + return ret;
> >> + if (ret != val_size)
> >> + return -EINVAL;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int regmap_at24_write(void *context, const void *data, size_t count)
> >> +{
> >> + struct i2c_client *client = context;
> >> + struct at24_data *at24;
> >> + unsigned int reg_bytes, offset;
> >> +
> >> + at24 = i2c_get_clientdata(client);
> >> + if (at24 == NULL)
> >> + return -ENODEV;
> >> +
> >> + reg_bytes = at24->regmap_config->reg_bits / 8;
> >> + offset = reg_bytes;
> >> +
> >> + BUG_ON(reg_bytes != 4);
> >> + BUG_ON(count <= offset);
> >> +
> >> + return regmap_at24_gather_write(context, data, reg_bytes,
> >> + data + offset, count - offset);
> >> +}
> >> +
> >> +static struct regmap_bus regmap_at24_bus = {
> >> + .read = regmap_at24_read,
> >> + .write = regmap_at24_write,
> >> + .gather_write = regmap_at24_gather_write,
> >> + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
> >> + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
> >
> > I wouldn't expect the content of the EEPROM to change of endianness
> > when we change the kernel’s
>
> It doesn’t matter really; these EEPROMs are byte accessible, and the register
> values are completely internal to the driver.
Ah, right. My bad :)
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
Hi Maxime,
> On May 15, 2015, at 17:19 , Maxime Ripard <[email protected]> wrote:
>
> On Thu, May 14, 2015 at 09:06:23AM +0300, Pantelis Antoniou wrote:
>>
>>> On May 13, 2015, at 23:36 , Maxime Ripard <[email protected]> wrote:
>>>
>>> Hi Pantelis,
>>>
>>> On Wed, May 13, 2015 at 10:38:17AM +0300, Pantelis Antoniou wrote:
>>>> For DT and in-kernel users there is no interface to the
>>>> at24 EEPROMs so provide an EEPROM framework interface.
>>>>
>>>> This allows us to use AT24 based EEPROMs and reference them
>>>> from within the DT tree.
>>>>
>>>> Signed-off-by: Pantelis Antoniou <[email protected]>
>>>
>>> You should probably mention that the EEPROM framework hasn't been
>>> merged yet.
>>>
>>
>> Well, that’s true. But it’s on version 4 and it does do what I need it
>> to do, namely provide a DT interface for in-kernel users.
>
> I was just meaning that people not aware of the fact that the
> framework was not merged might apply it, and that would result in an
> instant breakage, that's all.
>
> It's actually pretty nice that you convert this driver over to the
> EEPROM framework.
>
Thanks. It’s really broadly used and the framework works fine.
>>
>>>> ---
>>>> drivers/misc/eeprom/at24.c | 222 +++++++++++++++++++++++++++++++++++----------
>>>> 1 file changed, 174 insertions(+), 48 deletions(-)
>>>>
>>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>>> index 2d3db81..afa719a 100644
>>>> --- a/drivers/misc/eeprom/at24.c
>>>> +++ b/drivers/misc/eeprom/at24.c
>>>> @@ -3,6 +3,7 @@
>>>> *
>>>> * Copyright (C) 2005-2007 David Brownell
>>>> * Copyright (C) 2008 Wolfram Sang, Pengutronix
>>>> + * Copyright (C) 2015 Pantelis Antoniou, Konsulko Group
>>>> *
>>>> * This program is free software; you can redistribute it and/or modify
>>>> * it under the terms of the GNU General Public License as published by
>>>> @@ -23,6 +24,9 @@
>>>> #include <linux/of.h>
>>>> #include <linux/i2c.h>
>>>> #include <linux/platform_data/at24.h>
>>>> +#include <linux/regmap.h>
>>>> +#include <linux/eeprom-provider.h>
>>>> +#include <linux/io.h>
>>>>
>>>> /*
>>>> * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
>>>> @@ -63,12 +67,16 @@ struct at24_data {
>>>> * but not from changes by other I2C masters.
>>>> */
>>>> struct mutex lock;
>>>> - struct bin_attribute bin;
>>>>
>>>> u8 *writebuf;
>>>> unsigned write_max;
>>>> unsigned num_addresses;
>>>>
>>>> + struct regmap_config *regmap_config;
>>>> + struct regmap *regmap;
>>>> + struct eeprom_config *eeprom_config;
>>>> + struct eeprom_device *eeprom_dev;
>>>> +
>>>> /*
>>>> * Some chips tie up multiple I2C addresses; dummy devices reserve
>>>> * them for us, and we'll use them with SMBus calls.
>>>> @@ -131,6 +139,8 @@ static const struct i2c_device_id at24_ids[] = {
>>>> };
>>>> MODULE_DEVICE_TABLE(i2c, at24_ids);
>>>>
>>>> +static DEFINE_IDA(at24_ida);
>>>> +
>>>> /*-------------------------------------------------------------------------*/
>>>>
>>>> /*
>>>> @@ -301,17 +311,6 @@ static ssize_t at24_read(struct at24_data *at24,
>>>> return retval;
>>>> }
>>>>
>>>> -static ssize_t at24_bin_read(struct file *filp, struct kobject *kobj,
>>>> - struct bin_attribute *attr,
>>>> - char *buf, loff_t off, size_t count)
>>>> -{
>>>> - struct at24_data *at24;
>>>> -
>>>> - at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
>>>> - return at24_read(at24, buf, off, count);
>>>> -}
>>>> -
>>>> -
>>>> /*
>>>> * Note that if the hardware write-protect pin is pulled high, the whole
>>>> * chip is normally write protected. But there are plenty of product
>>>> @@ -432,21 +431,10 @@ static ssize_t at24_write(struct at24_data *at24, const char *buf, loff_t off,
>>>> return retval;
>>>> }
>>>>
>>>> -static ssize_t at24_bin_write(struct file *filp, struct kobject *kobj,
>>>> - struct bin_attribute *attr,
>>>> - char *buf, loff_t off, size_t count)
>>>> -{
>>>> - struct at24_data *at24;
>>>> -
>>>> - if (unlikely(off >= attr->size))
>>>> - return -EFBIG;
>>>> -
>>>> - at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
>>>> - return at24_write(at24, buf, off, count);
>>>> -}
>>>> -
>>>> /*-------------------------------------------------------------------------*/
>>>>
>>>> +/* panto: using the EEPROM framework macc is superfluous */
>>>> +
>>>
>>> Is it? it was kind of one of the point to be able to remove the memory
>>> accessors stuff (which is only used by that driver btw).
>>
>> Yes it completely redundant now. But it’s not my call to remove it, that would
>> be Wolfram.
>
> There's still a user of that stuff though, some davinci board iirc,
> that would need to port that user to the EEPROM framework too.
>
>>>> /*
>>>> * This lets other kernel code access the eeprom data. For example, it
>>>> * might hold a board's Ethernet address, or board-specific calibration
>>>> @@ -492,6 +480,91 @@ static void at24_get_ofdata(struct i2c_client *client,
>>>> { }
>>>> #endif /* CONFIG_OF */
>>>>
>>>> +static int regmap_at24_read(void *context,
>>>> + const void *reg, size_t reg_size,
>>>> + void *val, size_t val_size)
>>>> +{
>>>> + struct i2c_client *client = context;
>>>> + struct at24_data *at24;
>>>> + unsigned int offset;
>>>> + int ret;
>>>> +
>>>> + /* reg bits is hardcoded to 32 bits */
>>>> + BUG_ON(reg_size != 4);
>>>> + offset = __raw_readl(reg);
>>>> +
>>>> + at24 = i2c_get_clientdata(client);
>>>> + if (at24 == NULL)
>>>> + return -ENODEV;
>>>> +
>>>> + /* val bytes is always 1 */
>>>> + BUG_ON(at24->regmap_config->val_bits != 8);
>>>> +
>>>> + ret = at24_read(at24, val, offset, val_size);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + if (ret != val_size)
>>>> + return -EINVAL;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int regmap_at24_gather_write(void *context,
>>>> + const void *reg, size_t reg_size,
>>>> + const void *val, size_t val_size)
>>>> +{
>>>> + struct i2c_client *client = context;
>>>> + struct at24_data *at24;
>>>> + unsigned int offset;
>>>> + int ret;
>>>> +
>>>> + at24 = i2c_get_clientdata(client);
>>>> + if (at24 == NULL)
>>>> + return -ENODEV;
>>>> +
>>>> + BUG_ON(reg_size != 4);
>>>> + offset = __raw_readl(reg);
>>>> +
>>>> + /* val bytes is always 1 */
>>>> + BUG_ON(at24->regmap_config->val_bits != 8);
>>>> +
>>>> + ret = at24_write(at24, val, offset, val_size);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> + if (ret != val_size)
>>>> + return -EINVAL;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int regmap_at24_write(void *context, const void *data, size_t count)
>>>> +{
>>>> + struct i2c_client *client = context;
>>>> + struct at24_data *at24;
>>>> + unsigned int reg_bytes, offset;
>>>> +
>>>> + at24 = i2c_get_clientdata(client);
>>>> + if (at24 == NULL)
>>>> + return -ENODEV;
>>>> +
>>>> + reg_bytes = at24->regmap_config->reg_bits / 8;
>>>> + offset = reg_bytes;
>>>> +
>>>> + BUG_ON(reg_bytes != 4);
>>>> + BUG_ON(count <= offset);
>>>> +
>>>> + return regmap_at24_gather_write(context, data, reg_bytes,
>>>> + data + offset, count - offset);
>>>> +}
>>>> +
>>>> +static struct regmap_bus regmap_at24_bus = {
>>>> + .read = regmap_at24_read,
>>>> + .write = regmap_at24_write,
>>>> + .gather_write = regmap_at24_gather_write,
>>>> + .reg_format_endian_default = REGMAP_ENDIAN_NATIVE,
>>>> + .val_format_endian_default = REGMAP_ENDIAN_NATIVE,
>>>
>>> I wouldn't expect the content of the EEPROM to change of endianness
>>> when we change the kernel’s
>>
>> It doesn’t matter really; these EEPROMs are byte accessible, and the register
>> values are completely internal to the driver.
>
> Ah, right. My bad :)
>
No worries :)
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
Regards
— Pantelis