Re: [PATCH] i2c: eeprom: at24: Provide an EEPROM framework interface

[Maxime Ripard]

On Thu, May 14, 2015 at 09:06:23AM +0300, Pantelis Antoniou wrote:
> 
> > On May 13, 2015, at 23:36 , Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> 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 <pantelis.antoniou@xxxxxxxxxxxx>
> > 
> > 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

Attachment: signature.asc
Description: Digital signature