Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754984AbbENGGc (ORCPT ); Thu, 14 May 2015 02:06:32 -0400 Received: from li42-95.members.linode.com ([209.123.162.95]:51252 "EHLO li42-95.members.linode.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754851AbbENGGb convert rfc822-to-8bit (ORCPT ); Thu, 14 May 2015 02:06:31 -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: <20150513203646.GI4004@lukather> Date: Thu, 14 May 2015 09:06:23 +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: References: <1431502697-31333-1-git-send-email-pantelis.antoniou@konsulko.com> <20150513203646.GI4004@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: 7106 Lines: 240 > 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. >> --- >> 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. >> /* >> * 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 -- 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/