Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752312AbbERIXr (ORCPT ); Mon, 18 May 2015 04:23:47 -0400 Received: from li42-95.members.linode.com ([209.123.162.95]:48404 "EHLO li42-95.members.linode.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750949AbbERIXl convert rfc822-to-8bit (ORCPT ); Mon, 18 May 2015 04:23:41 -0400 Subject: Re: [PATCH] i2c: eeprom: at24: Provide an EEPROM framework interface Mime-Version: 1.0 (Mac OS X Mail 8.2 \(2098\)) Content-Type: text/plain; charset=utf-8 From: Pantelis Antoniou In-Reply-To: <20150515141909.GX4004@lukather> Date: Mon, 18 May 2015 11:23:35 +0300 Cc: Wolfram Sang , Matt Porter , Srinivas Kandagatla , linux-i2c@vger.kernel.org, Koen Kooi , linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8BIT Message-Id: <11E4087A-B7EF-4C4B-95FD-25D96A12AB31@antoniou-consulting.com> References: <1431502697-31333-1-git-send-email-pantelis.antoniou@konsulko.com> <20150513203646.GI4004@lukather> <20150515141909.GX4004@lukather> To: Maxime Ripard X-Mailer: Apple Mail (2.2098) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8115 Lines: 258 Hi Maxime, > On May 15, 2015, at 17:19 , Maxime Ripard wrote: > > On Thu, May 14, 2015 at 09:06:23AM +0300, Pantelis Antoniou wrote: >> >>> On May 13, 2015, at 23:36 , Maxime Ripard 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 >>> >>> 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 >>>> #include >>>> #include >>>> +#include >>>> +#include >>>> +#include >>>> >>>> /* >>>> * 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/