Hi Maxime,
> On May 15, 2015, at 17:19 , Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx> wrote:
>
> 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.
>
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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
