| Message ID | 20200514103834.932389-2-acelan.kao@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | add 16-bit width registers support for EEPROM at24 device | expand |
Hi AceLan- After a discussion with Stefan and Kleber, we are not comfortable with this version for bionic: 1. The significant difference from the upstream-accepted focal version is confusing. (Perhaps it would be better to just develop a separate UBUNTU SAUCE patch for bionic, instead of calling this a "port" of the upstream patch?) 2. We'd like to have the author's Signed-off-by. Please contact the author and ask for it. Thanks for your patience! -Kamal On Thu, May 14, 2020 at 06:38:34PM +0800, AceLan Kao wrote: > From: Pieri <pierluigi.driusso@eurotech.com> > > BugLink: https://bugs.launchpad.net/bugs/1876699 > > This allows to access EEPROM with 16-bit width of registers via i2c SMBus > block functions. > > This commit is derivated from below commit, and then modified and provided > by customer > https://patchwork.ozlabs.org/patch/545292/ > Unfortunately, this commit didn't get merged into mainline in the end. > > From the discussion in the patch thread and another lengthy discussion[1], > it looks like the maintainer is convinced to accept this patch[2]. > But it turns out that there is a new idea to test[3] and the result is > not good, and the origin patch has been forgotten and didn't get merged > in the end. > > In the discussion, the implementation to support access 16-bit address data > is not safe. The multi-command sequence of the read function is not safe > and may read the wrong data from other address if other commands are sent > in-between the SMBus commands in the read function. > > 1. https://lkml.org/lkml/2015/2/4/436 > 2. https://lkml.org/lkml/2015/3/25/619 > 3. https://lkml.org/lkml/2015/3/27/99 > > Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > --- > drivers/misc/eeprom/Kconfig | 5 +- > drivers/misc/eeprom/at24.c | 139 +++++++++++++++++++++++++++++++----- > 2 files changed, 127 insertions(+), 17 deletions(-) > > diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig > index 3f93e4564cab9..27b0e8ee04926 100644 > --- a/drivers/misc/eeprom/Kconfig > +++ b/drivers/misc/eeprom/Kconfig > @@ -23,7 +23,10 @@ config EEPROM_AT24 > > If you use this with an SMBus adapter instead of an I2C adapter, > full functionality is not available. Only smaller devices are > - supported (24c16 and below, max 4 kByte). > + supported via block reads (24c16 and below, max 4 kByte). > + Larger devices that use 16-bit addresses will only work with > + individual byte reads, which is very slow in general and is unsafe > + in multi-master SMBus topologies. > > This driver can also be built as a module. If so, the module > will be called at24. > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > index 7554b001b502f..614b5b2c5bbd9 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 Extreme Engineering Solutions, Inc. > * > * 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 > @@ -52,7 +53,7 @@ > * Other than binding model, current differences from "eeprom" driver are > * that this one handles write access and isn't restricted to 24c02 devices. > * It also handles larger devices (32 kbit and up) with two-byte addresses, > - * which won't work on pure SMBus systems. > + * which don't work without risks on pure SMBus systems. > */ > > struct at24_data { > @@ -246,6 +247,88 @@ MODULE_DEVICE_TABLE(acpi, at24_acpi_ids); > * one "eeprom" file not four, but larger reads would fail when > * they crossed certain pages. > */ > + > +/* > + * Write a byte to an AT24 device using SMBus cycles. > + */ > +static inline s32 at24_smbus_write_byte_data(struct at24_data *at24, > + struct i2c_client *client, u16 offset, u8 value) > +{ > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > + return i2c_smbus_write_byte_data(client, offset, value); > + > + /* > + * Emulate I2C multi-byte write by using SMBus "write word" > + * cycle. We split up the 16-bit offset among the "command" > + * byte and the first data byte. > + */ > + return i2c_smbus_write_word_data(client, offset >> 8, > + (value << 8) | (offset & 0xff)); > +} > + > +/* > + * Write block data to an AT24 device using SMBus cycles. > + */ > +static inline s32 at24_smbus_write_i2c_block_data(struct at24_data *at24, > + const struct i2c_client *client, u16 off, u8 len, const u8 *vals) > +{ > + s32 res; > + > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > + return i2c_smbus_write_i2c_block_data(client, off, len, vals); > + > + /* Insert extra address byte into data stream */ > + at24->writebuf[0] = off & 0xff; > + memcpy(&at24->writebuf[1], vals, len); > + > + res = i2c_smbus_write_i2c_block_data(client, off >> 8, len + 1, > + at24->writebuf); > + > + return res; > +} > + > +/* > + * Read block data from an AT24 device using SMBus cycles. > + */ > +static inline s32 at24_smbus_read_block_data(struct at24_data *at24, > + const struct i2c_client *client, u16 off, u8 len, u8 *vals) > +{ > + int count; > + s32 res; > + > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > + return i2c_smbus_read_i2c_block_data_or_emulated(client, off, > + len, vals); > + > + /* > + * Emulate I2C multi-byte read by using SMBus "write byte" and > + * "receive byte". This is slightly unsafe since there is an > + * additional STOP involved, which exposes the SMBus and (this > + * device!) to takeover by another bus master. However, it's the > + * only way to work on SMBus-only controllers when talking to > + * EEPROMs with multi-byte addresses. > + */ > + > + /* Address "dummy" write */ > + res = i2c_smbus_write_byte_data(client, off >> 8, off & 0xff); > + if (res < 0) > + return res; > + > + count = 0; > + do { > + /* Current Address Read */ > + res = i2c_smbus_read_byte(client); > + if (res < 0) > + break; > + > + *(vals++) = res; > + count++; > + len--; > + } while (len > 0); > + > + return count; > +} > + > static struct i2c_client *at24_translate_offset(struct at24_data *at24, > unsigned int *offset) > { > @@ -286,10 +369,8 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf, > */ > read_time = jiffies; > > - status = i2c_smbus_read_i2c_block_data_or_emulated(client, > - offset, > - count, buf); > - > + status = at24_smbus_read_block_data(at24, client, > + offset, count, buf); > dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n", > count, offset, status, jiffies); > > @@ -508,8 +589,8 @@ static ssize_t at24_eeprom_write_smbus_block(struct at24_data *at24, > */ > write_time = jiffies; > > - status = i2c_smbus_write_i2c_block_data(client, > - offset, count, buf); > + status = at24_smbus_write_i2c_block_data(at24, > + client, offset, count, buf); > if (status == 0) > status = count; > > @@ -543,7 +624,8 @@ static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24, > */ > write_time = jiffies; > > - status = i2c_smbus_write_byte_data(client, offset, buf[0]); > + status = at24_smbus_write_byte_data(at24, client, offset, > + buf[0]); > if (status == 0) > status = count; > > @@ -810,13 +892,32 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > > /* Use I2C operations unless we're stuck with SMBus extensions. */ > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > - if (chip.flags & AT24_FLAG_ADDR16) > - return -EPFNOSUPPORT; > - > - if (i2c_check_functionality(client->adapter, > + if ((chip.flags & AT24_FLAG_ADDR16) && > + i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_READ_BYTE | > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) { > + /* > + * We need SMBUS_WRITE_BYTE_DATA and SMBUS_READ_BYTE to > + * implement byte reads for 16-bit address devices. > + * This will be slow, but better than nothing (e.g. > + * read @ 3.6 KiB/s). It is also unsafe in a multi- > + * master topology. > + */ > + use_smbus = I2C_SMBUS_BYTE_DATA; > + } else if (i2c_check_functionality(client->adapter, > I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { > use_smbus = I2C_SMBUS_I2C_BLOCK_DATA; > - } else if (i2c_check_functionality(client->adapter, > + } else if ((chip.flags & AT24_FLAG_ADDR16) && > + i2c_check_functionality(client->adapter, > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) { > + /* > + * We need SMBUS_WRITE_WORD_DATA to implement > + * byte writes for 16-bit address devices. > + */ > + use_smbus_write = I2C_SMBUS_BYTE_DATA; > + chip.page_size = 1; > + } else if (!(chip.flags & AT24_FLAG_ADDR16) && > + i2c_check_functionality(client->adapter, > I2C_FUNC_SMBUS_READ_WORD_DATA)) { > use_smbus = I2C_SMBUS_WORD_DATA; > } else if (i2c_check_functionality(client->adapter, > @@ -836,6 +937,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > } > } > > + if (strcmp(client->name, "24c256") == 0) > + chip.page_size = 64; > + > if (chip.flags & AT24_FLAG_TAKE8ADDR) > num_addresses = 8; > else > @@ -881,12 +985,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > if (writable) { > if (!use_smbus || use_smbus_write) { > > - unsigned write_max = chip.page_size; > + unsigned int write_max = chip.page_size; > + unsigned int smbus_max = (chip.flags & AT24_FLAG_ADDR16) ? > + I2C_SMBUS_BLOCK_MAX - 1 : > + I2C_SMBUS_BLOCK_MAX; > > if (write_max > io_limit) > write_max = io_limit; > - if (use_smbus && write_max > I2C_SMBUS_BLOCK_MAX) > - write_max = I2C_SMBUS_BLOCK_MAX; > + if (use_smbus && write_max > smbus_max) > + write_max = smbus_max; > at24->write_max = write_max; > > /* buffer (data + address at the beginning) */ > -- > 2.25.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
I'm willing to fix this patch and I hope we can start to review patches earlier and not NACK it at last moment. This leads to projects don't have a working kernel for around 2 months. 1. I didn't call this is a port of upstream patch. I wrote this in the patch > This commit is derivated from below commit, and then modified and provided > by customer > https://patchwork.ozlabs.org/patch/545292/ And say > I migrate the commit to latest kernel and finally get it merged. 2. Okay, I just sent out an email to ask for the permission to add author's signed off Kamal Mostafa <kamal@canonical.com> 於 2020年5月15日 週五 上午12:18寫道： > > Hi AceLan- > > After a discussion with Stefan and Kleber, we are not comfortable with > this version for bionic: > > 1. The significant difference from the upstream-accepted focal version > is confusing. (Perhaps it would be better to just develop a separate > UBUNTU SAUCE patch for bionic, instead of calling this a "port" of the > upstream patch?) > > 2. We'd like to have the author's Signed-off-by. Please contact the > author and ask for it. > > Thanks for your patience! > > -Kamal > > On Thu, May 14, 2020 at 06:38:34PM +0800, AceLan Kao wrote: > > From: Pieri <pierluigi.driusso@eurotech.com> > > > > BugLink: https://bugs.launchpad.net/bugs/1876699 > > > > This allows to access EEPROM with 16-bit width of registers via i2c SMBus > > block functions. > > > > This commit is derivated from below commit, and then modified and provided > > by customer > > https://patchwork.ozlabs.org/patch/545292/ > > Unfortunately, this commit didn't get merged into mainline in the end. > > > > From the discussion in the patch thread and another lengthy discussion[1], > > it looks like the maintainer is convinced to accept this patch[2]. > > But it turns out that there is a new idea to test[3] and the result is > > not good, and the origin patch has been forgotten and didn't get merged > > in the end. > > > > In the discussion, the implementation to support access 16-bit address data > > is not safe. The multi-command sequence of the read function is not safe > > and may read the wrong data from other address if other commands are sent > > in-between the SMBus commands in the read function. > > > > 1. https://lkml.org/lkml/2015/2/4/436 > > 2. https://lkml.org/lkml/2015/3/25/619 > > 3. https://lkml.org/lkml/2015/3/27/99 > > > > Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > > --- > > drivers/misc/eeprom/Kconfig | 5 +- > > drivers/misc/eeprom/at24.c | 139 +++++++++++++++++++++++++++++++----- > > 2 files changed, 127 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig > > index 3f93e4564cab9..27b0e8ee04926 100644 > > --- a/drivers/misc/eeprom/Kconfig > > +++ b/drivers/misc/eeprom/Kconfig > > @@ -23,7 +23,10 @@ config EEPROM_AT24 > > > > If you use this with an SMBus adapter instead of an I2C adapter, > > full functionality is not available. Only smaller devices are > > - supported (24c16 and below, max 4 kByte). > > + supported via block reads (24c16 and below, max 4 kByte). > > + Larger devices that use 16-bit addresses will only work with > > + individual byte reads, which is very slow in general and is unsafe > > + in multi-master SMBus topologies. > > > > This driver can also be built as a module. If so, the module > > will be called at24. > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > > index 7554b001b502f..614b5b2c5bbd9 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 Extreme Engineering Solutions, Inc. > > * > > * 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 > > @@ -52,7 +53,7 @@ > > * Other than binding model, current differences from "eeprom" driver are > > * that this one handles write access and isn't restricted to 24c02 devices. > > * It also handles larger devices (32 kbit and up) with two-byte addresses, > > - * which won't work on pure SMBus systems. > > + * which don't work without risks on pure SMBus systems. > > */ > > > > struct at24_data { > > @@ -246,6 +247,88 @@ MODULE_DEVICE_TABLE(acpi, at24_acpi_ids); > > * one "eeprom" file not four, but larger reads would fail when > > * they crossed certain pages. > > */ > > + > > +/* > > + * Write a byte to an AT24 device using SMBus cycles. > > + */ > > +static inline s32 at24_smbus_write_byte_data(struct at24_data *at24, > > + struct i2c_client *client, u16 offset, u8 value) > > +{ > > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > > + return i2c_smbus_write_byte_data(client, offset, value); > > + > > + /* > > + * Emulate I2C multi-byte write by using SMBus "write word" > > + * cycle. We split up the 16-bit offset among the "command" > > + * byte and the first data byte. > > + */ > > + return i2c_smbus_write_word_data(client, offset >> 8, > > + (value << 8) | (offset & 0xff)); > > +} > > + > > +/* > > + * Write block data to an AT24 device using SMBus cycles. > > + */ > > +static inline s32 at24_smbus_write_i2c_block_data(struct at24_data *at24, > > + const struct i2c_client *client, u16 off, u8 len, const u8 *vals) > > +{ > > + s32 res; > > + > > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > > + return i2c_smbus_write_i2c_block_data(client, off, len, vals); > > + > > + /* Insert extra address byte into data stream */ > > + at24->writebuf[0] = off & 0xff; > > + memcpy(&at24->writebuf[1], vals, len); > > + > > + res = i2c_smbus_write_i2c_block_data(client, off >> 8, len + 1, > > + at24->writebuf); > > + > > + return res; > > +} > > + > > +/* > > + * Read block data from an AT24 device using SMBus cycles. > > + */ > > +static inline s32 at24_smbus_read_block_data(struct at24_data *at24, > > + const struct i2c_client *client, u16 off, u8 len, u8 *vals) > > +{ > > + int count; > > + s32 res; > > + > > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > > + return i2c_smbus_read_i2c_block_data_or_emulated(client, off, > > + len, vals); > > + > > + /* > > + * Emulate I2C multi-byte read by using SMBus "write byte" and > > + * "receive byte". This is slightly unsafe since there is an > > + * additional STOP involved, which exposes the SMBus and (this > > + * device!) to takeover by another bus master. However, it's the > > + * only way to work on SMBus-only controllers when talking to > > + * EEPROMs with multi-byte addresses. > > + */ > > + > > + /* Address "dummy" write */ > > + res = i2c_smbus_write_byte_data(client, off >> 8, off & 0xff); > > + if (res < 0) > > + return res; > > + > > + count = 0; > > + do { > > + /* Current Address Read */ > > + res = i2c_smbus_read_byte(client); > > + if (res < 0) > > + break; > > + > > + *(vals++) = res; > > + count++; > > + len--; > > + } while (len > 0); > > + > > + return count; > > +} > > + > > static struct i2c_client *at24_translate_offset(struct at24_data *at24, > > unsigned int *offset) > > { > > @@ -286,10 +369,8 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf, > > */ > > read_time = jiffies; > > > > - status = i2c_smbus_read_i2c_block_data_or_emulated(client, > > - offset, > > - count, buf); > > - > > + status = at24_smbus_read_block_data(at24, client, > > + offset, count, buf); > > dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n", > > count, offset, status, jiffies); > > > > @@ -508,8 +589,8 @@ static ssize_t at24_eeprom_write_smbus_block(struct at24_data *at24, > > */ > > write_time = jiffies; > > > > - status = i2c_smbus_write_i2c_block_data(client, > > - offset, count, buf); > > + status = at24_smbus_write_i2c_block_data(at24, > > + client, offset, count, buf); > > if (status == 0) > > status = count; > > > > @@ -543,7 +624,8 @@ static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24, > > */ > > write_time = jiffies; > > > > - status = i2c_smbus_write_byte_data(client, offset, buf[0]); > > + status = at24_smbus_write_byte_data(at24, client, offset, > > + buf[0]); > > if (status == 0) > > status = count; > > > > @@ -810,13 +892,32 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > > > > /* Use I2C operations unless we're stuck with SMBus extensions. */ > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > > - if (chip.flags & AT24_FLAG_ADDR16) > > - return -EPFNOSUPPORT; > > - > > - if (i2c_check_functionality(client->adapter, > > + if ((chip.flags & AT24_FLAG_ADDR16) && > > + i2c_check_functionality(client->adapter, > > + I2C_FUNC_SMBUS_READ_BYTE | > > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) { > > + /* > > + * We need SMBUS_WRITE_BYTE_DATA and SMBUS_READ_BYTE to > > + * implement byte reads for 16-bit address devices. > > + * This will be slow, but better than nothing (e.g. > > + * read @ 3.6 KiB/s). It is also unsafe in a multi- > > + * master topology. > > + */ > > + use_smbus = I2C_SMBUS_BYTE_DATA; > > + } else if (i2c_check_functionality(client->adapter, > > I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { > > use_smbus = I2C_SMBUS_I2C_BLOCK_DATA; > > - } else if (i2c_check_functionality(client->adapter, > > + } else if ((chip.flags & AT24_FLAG_ADDR16) && > > + i2c_check_functionality(client->adapter, > > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) { > > + /* > > + * We need SMBUS_WRITE_WORD_DATA to implement > > + * byte writes for 16-bit address devices. > > + */ > > + use_smbus_write = I2C_SMBUS_BYTE_DATA; > > + chip.page_size = 1; > > + } else if (!(chip.flags & AT24_FLAG_ADDR16) && > > + i2c_check_functionality(client->adapter, > > I2C_FUNC_SMBUS_READ_WORD_DATA)) { > > use_smbus = I2C_SMBUS_WORD_DATA; > > } else if (i2c_check_functionality(client->adapter, > > @@ -836,6 +937,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > > } > > } > > > > + if (strcmp(client->name, "24c256") == 0) > > + chip.page_size = 64; > > + > > if (chip.flags & AT24_FLAG_TAKE8ADDR) > > num_addresses = 8; > > else > > @@ -881,12 +985,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > > if (writable) { > > if (!use_smbus || use_smbus_write) { > > > > - unsigned write_max = chip.page_size; > > + unsigned int write_max = chip.page_size; > > + unsigned int smbus_max = (chip.flags & AT24_FLAG_ADDR16) ? > > + I2C_SMBUS_BLOCK_MAX - 1 : > > + I2C_SMBUS_BLOCK_MAX; > > > > if (write_max > io_limit) > > write_max = io_limit; > > - if (use_smbus && write_max > I2C_SMBUS_BLOCK_MAX) > > - write_max = I2C_SMBUS_BLOCK_MAX; > > + if (use_smbus && write_max > smbus_max) > > + write_max = smbus_max; > > at24->write_max = write_max; > > > > /* buffer (data + address at the beginning) */ > > -- > > 2.25.1 > > > > > > -- > > kernel-team mailing list > > kernel-team@lists.ubuntu.com > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
I granted the permission from the author to add his s-o-b Signed-off-by: Pieri <pierluigi.driusso@eurotech.com> And I explained where this patch comes from in the previous email, is there anything I could do to make this commit be included in bionic? AceLan Kao <acelan.kao@canonical.com> 於 2020年5月15日 週五 上午11:46寫道： > > I'm willing to fix this patch and I hope we can start to review > patches earlier and > not NACK it at last moment. This leads to projects don't have a > working kernel for around 2 months. > > 1. I didn't call this is a port of upstream patch. I wrote this in the patch > > This commit is derivated from below commit, and then modified and provided > > by customer > > https://patchwork.ozlabs.org/patch/545292/ > And say > > I migrate the commit to latest kernel and finally get it merged. > > 2. Okay, I just sent out an email to ask for the permission to add > author's signed off > > Kamal Mostafa <kamal@canonical.com> 於 2020年5月15日 週五 上午12:18寫道： > > > > > > Hi AceLan- > > > > After a discussion with Stefan and Kleber, we are not comfortable with > > this version for bionic: > > > > 1. The significant difference from the upstream-accepted focal version > > is confusing. (Perhaps it would be better to just develop a separate > > UBUNTU SAUCE patch for bionic, instead of calling this a "port" of the > > upstream patch?) > > > > 2. We'd like to have the author's Signed-off-by. Please contact the > > author and ask for it. > > > > Thanks for your patience! > > > > -Kamal > > > > On Thu, May 14, 2020 at 06:38:34PM +0800, AceLan Kao wrote: > > > From: Pieri <pierluigi.driusso@eurotech.com> > > > > > > BugLink: https://bugs.launchpad.net/bugs/1876699 > > > > > > This allows to access EEPROM with 16-bit width of registers via i2c SMBus > > > block functions. > > > > > > This commit is derivated from below commit, and then modified and provided > > > by customer > > > https://patchwork.ozlabs.org/patch/545292/ > > > Unfortunately, this commit didn't get merged into mainline in the end. > > > > > > From the discussion in the patch thread and another lengthy discussion[1], > > > it looks like the maintainer is convinced to accept this patch[2]. > > > But it turns out that there is a new idea to test[3] and the result is > > > not good, and the origin patch has been forgotten and didn't get merged > > > in the end. > > > > > > In the discussion, the implementation to support access 16-bit address data > > > is not safe. The multi-command sequence of the read function is not safe > > > and may read the wrong data from other address if other commands are sent > > > in-between the SMBus commands in the read function. > > > > > > 1. https://lkml.org/lkml/2015/2/4/436 > > > 2. https://lkml.org/lkml/2015/3/25/619 > > > 3. https://lkml.org/lkml/2015/3/27/99 > > > > > > Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > > > --- > > > drivers/misc/eeprom/Kconfig | 5 +- > > > drivers/misc/eeprom/at24.c | 139 +++++++++++++++++++++++++++++++----- > > > 2 files changed, 127 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig > > > index 3f93e4564cab9..27b0e8ee04926 100644 > > > --- a/drivers/misc/eeprom/Kconfig > > > +++ b/drivers/misc/eeprom/Kconfig > > > @@ -23,7 +23,10 @@ config EEPROM_AT24 > > > > > > If you use this with an SMBus adapter instead of an I2C adapter, > > > full functionality is not available. Only smaller devices are > > > - supported (24c16 and below, max 4 kByte). > > > + supported via block reads (24c16 and below, max 4 kByte). > > > + Larger devices that use 16-bit addresses will only work with > > > + individual byte reads, which is very slow in general and is unsafe > > > + in multi-master SMBus topologies. > > > > > > This driver can also be built as a module. If so, the module > > > will be called at24. > > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > > > index 7554b001b502f..614b5b2c5bbd9 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 Extreme Engineering Solutions, Inc. > > > * > > > * 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 > > > @@ -52,7 +53,7 @@ > > > * Other than binding model, current differences from "eeprom" driver are > > > * that this one handles write access and isn't restricted to 24c02 devices. > > > * It also handles larger devices (32 kbit and up) with two-byte addresses, > > > - * which won't work on pure SMBus systems. > > > + * which don't work without risks on pure SMBus systems. > > > */ > > > > > > struct at24_data { > > > @@ -246,6 +247,88 @@ MODULE_DEVICE_TABLE(acpi, at24_acpi_ids); > > > * one "eeprom" file not four, but larger reads would fail when > > > * they crossed certain pages. > > > */ > > > + > > > +/* > > > + * Write a byte to an AT24 device using SMBus cycles. > > > + */ > > > +static inline s32 at24_smbus_write_byte_data(struct at24_data *at24, > > > + struct i2c_client *client, u16 offset, u8 value) > > > +{ > > > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > > > + return i2c_smbus_write_byte_data(client, offset, value); > > > + > > > + /* > > > + * Emulate I2C multi-byte write by using SMBus "write word" > > > + * cycle. We split up the 16-bit offset among the "command" > > > + * byte and the first data byte. > > > + */ > > > + return i2c_smbus_write_word_data(client, offset >> 8, > > > + (value << 8) | (offset & 0xff)); > > > +} > > > + > > > +/* > > > + * Write block data to an AT24 device using SMBus cycles. > > > + */ > > > +static inline s32 at24_smbus_write_i2c_block_data(struct at24_data *at24, > > > + const struct i2c_client *client, u16 off, u8 len, const u8 *vals) > > > +{ > > > + s32 res; > > > + > > > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > > > + return i2c_smbus_write_i2c_block_data(client, off, len, vals); > > > + > > > + /* Insert extra address byte into data stream */ > > > + at24->writebuf[0] = off & 0xff; > > > + memcpy(&at24->writebuf[1], vals, len); > > > + > > > + res = i2c_smbus_write_i2c_block_data(client, off >> 8, len + 1, > > > + at24->writebuf); > > > + > > > + return res; > > > +} > > > + > > > +/* > > > + * Read block data from an AT24 device using SMBus cycles. > > > + */ > > > +static inline s32 at24_smbus_read_block_data(struct at24_data *at24, > > > + const struct i2c_client *client, u16 off, u8 len, u8 *vals) > > > +{ > > > + int count; > > > + s32 res; > > > + > > > + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) > > > + return i2c_smbus_read_i2c_block_data_or_emulated(client, off, > > > + len, vals); > > > + > > > + /* > > > + * Emulate I2C multi-byte read by using SMBus "write byte" and > > > + * "receive byte". This is slightly unsafe since there is an > > > + * additional STOP involved, which exposes the SMBus and (this > > > + * device!) to takeover by another bus master. However, it's the > > > + * only way to work on SMBus-only controllers when talking to > > > + * EEPROMs with multi-byte addresses. > > > + */ > > > + > > > + /* Address "dummy" write */ > > > + res = i2c_smbus_write_byte_data(client, off >> 8, off & 0xff); > > > + if (res < 0) > > > + return res; > > > + > > > + count = 0; > > > + do { > > > + /* Current Address Read */ > > > + res = i2c_smbus_read_byte(client); > > > + if (res < 0) > > > + break; > > > + > > > + *(vals++) = res; > > > + count++; > > > + len--; > > > + } while (len > 0); > > > + > > > + return count; > > > +} > > > + > > > static struct i2c_client *at24_translate_offset(struct at24_data *at24, > > > unsigned int *offset) > > > { > > > @@ -286,10 +369,8 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf, > > > */ > > > read_time = jiffies; > > > > > > - status = i2c_smbus_read_i2c_block_data_or_emulated(client, > > > - offset, > > > - count, buf); > > > - > > > + status = at24_smbus_read_block_data(at24, client, > > > + offset, count, buf); > > > dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n", > > > count, offset, status, jiffies); > > > > > > @@ -508,8 +589,8 @@ static ssize_t at24_eeprom_write_smbus_block(struct at24_data *at24, > > > */ > > > write_time = jiffies; > > > > > > - status = i2c_smbus_write_i2c_block_data(client, > > > - offset, count, buf); > > > + status = at24_smbus_write_i2c_block_data(at24, > > > + client, offset, count, buf); > > > if (status == 0) > > > status = count; > > > > > > @@ -543,7 +624,8 @@ static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24, > > > */ > > > write_time = jiffies; > > > > > > - status = i2c_smbus_write_byte_data(client, offset, buf[0]); > > > + status = at24_smbus_write_byte_data(at24, client, offset, > > > + buf[0]); > > > if (status == 0) > > > status = count; > > > > > > @@ -810,13 +892,32 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > > > > > > /* Use I2C operations unless we're stuck with SMBus extensions. */ > > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > > > - if (chip.flags & AT24_FLAG_ADDR16) > > > - return -EPFNOSUPPORT; > > > - > > > - if (i2c_check_functionality(client->adapter, > > > + if ((chip.flags & AT24_FLAG_ADDR16) && > > > + i2c_check_functionality(client->adapter, > > > + I2C_FUNC_SMBUS_READ_BYTE | > > > + I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) { > > > + /* > > > + * We need SMBUS_WRITE_BYTE_DATA and SMBUS_READ_BYTE to > > > + * implement byte reads for 16-bit address devices. > > > + * This will be slow, but better than nothing (e.g. > > > + * read @ 3.6 KiB/s). It is also unsafe in a multi- > > > + * master topology. > > > + */ > > > + use_smbus = I2C_SMBUS_BYTE_DATA; > > > + } else if (i2c_check_functionality(client->adapter, > > > I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { > > > use_smbus = I2C_SMBUS_I2C_BLOCK_DATA; > > > - } else if (i2c_check_functionality(client->adapter, > > > + } else if ((chip.flags & AT24_FLAG_ADDR16) && > > > + i2c_check_functionality(client->adapter, > > > + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) { > > > + /* > > > + * We need SMBUS_WRITE_WORD_DATA to implement > > > + * byte writes for 16-bit address devices. > > > + */ > > > + use_smbus_write = I2C_SMBUS_BYTE_DATA; > > > + chip.page_size = 1; > > > + } else if (!(chip.flags & AT24_FLAG_ADDR16) && > > > + i2c_check_functionality(client->adapter, > > > I2C_FUNC_SMBUS_READ_WORD_DATA)) { > > > use_smbus = I2C_SMBUS_WORD_DATA; > > > } else if (i2c_check_functionality(client->adapter, > > > @@ -836,6 +937,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > > > } > > > } > > > > > > + if (strcmp(client->name, "24c256") == 0) > > > + chip.page_size = 64; > > > + > > > if (chip.flags & AT24_FLAG_TAKE8ADDR) > > > num_addresses = 8; > > > else > > > @@ -881,12 +985,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) > > > if (writable) { > > > if (!use_smbus || use_smbus_write) { > > > > > > - unsigned write_max = chip.page_size; > > > + unsigned int write_max = chip.page_size; > > > + unsigned int smbus_max = (chip.flags & AT24_FLAG_ADDR16) ? > > > + I2C_SMBUS_BLOCK_MAX - 1 : > > > + I2C_SMBUS_BLOCK_MAX; > > > > > > if (write_max > io_limit) > > > write_max = io_limit; > > > - if (use_smbus && write_max > I2C_SMBUS_BLOCK_MAX) > > > - write_max = I2C_SMBUS_BLOCK_MAX; > > > + if (use_smbus && write_max > smbus_max) > > > + write_max = smbus_max; > > > at24->write_max = write_max; > > > > > > /* buffer (data + address at the beginning) */ > > > -- > > > 2.25.1 > > > > > > > > > -- > > > kernel-team mailing list > > > kernel-team@lists.ubuntu.com > > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig index 3f93e4564cab9..27b0e8ee04926 100644 --- a/drivers/misc/eeprom/Kconfig +++ b/drivers/misc/eeprom/Kconfig @@ -23,7 +23,10 @@ config EEPROM_AT24 If you use this with an SMBus adapter instead of an I2C adapter, full functionality is not available. Only smaller devices are - supported (24c16 and below, max 4 kByte). + supported via block reads (24c16 and below, max 4 kByte). + Larger devices that use 16-bit addresses will only work with + individual byte reads, which is very slow in general and is unsafe + in multi-master SMBus topologies. This driver can also be built as a module. If so, the module will be called at24. diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c index 7554b001b502f..614b5b2c5bbd9 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 Extreme Engineering Solutions, Inc. * * 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 @@ -52,7 +53,7 @@ * Other than binding model, current differences from "eeprom" driver are * that this one handles write access and isn't restricted to 24c02 devices. * It also handles larger devices (32 kbit and up) with two-byte addresses, - * which won't work on pure SMBus systems. + * which don't work without risks on pure SMBus systems. */ struct at24_data { @@ -246,6 +247,88 @@ MODULE_DEVICE_TABLE(acpi, at24_acpi_ids); * one "eeprom" file not four, but larger reads would fail when * they crossed certain pages. */ + +/* + * Write a byte to an AT24 device using SMBus cycles. + */ +static inline s32 at24_smbus_write_byte_data(struct at24_data *at24, + struct i2c_client *client, u16 offset, u8 value) +{ + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) + return i2c_smbus_write_byte_data(client, offset, value); + + /* + * Emulate I2C multi-byte write by using SMBus "write word" + * cycle. We split up the 16-bit offset among the "command" + * byte and the first data byte. + */ + return i2c_smbus_write_word_data(client, offset >> 8, + (value << 8) | (offset & 0xff)); +} + +/* + * Write block data to an AT24 device using SMBus cycles. + */ +static inline s32 at24_smbus_write_i2c_block_data(struct at24_data *at24, + const struct i2c_client *client, u16 off, u8 len, const u8 *vals) +{ + s32 res; + + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) + return i2c_smbus_write_i2c_block_data(client, off, len, vals); + + /* Insert extra address byte into data stream */ + at24->writebuf[0] = off & 0xff; + memcpy(&at24->writebuf[1], vals, len); + + res = i2c_smbus_write_i2c_block_data(client, off >> 8, len + 1, + at24->writebuf); + + return res; +} + +/* + * Read block data from an AT24 device using SMBus cycles. + */ +static inline s32 at24_smbus_read_block_data(struct at24_data *at24, + const struct i2c_client *client, u16 off, u8 len, u8 *vals) +{ + int count; + s32 res; + + if (!(at24->chip.flags & AT24_FLAG_ADDR16)) + return i2c_smbus_read_i2c_block_data_or_emulated(client, off, + len, vals); + + /* + * Emulate I2C multi-byte read by using SMBus "write byte" and + * "receive byte". This is slightly unsafe since there is an + * additional STOP involved, which exposes the SMBus and (this + * device!) to takeover by another bus master. However, it's the + * only way to work on SMBus-only controllers when talking to + * EEPROMs with multi-byte addresses. + */ + + /* Address "dummy" write */ + res = i2c_smbus_write_byte_data(client, off >> 8, off & 0xff); + if (res < 0) + return res; + + count = 0; + do { + /* Current Address Read */ + res = i2c_smbus_read_byte(client); + if (res < 0) + break; + + *(vals++) = res; + count++; + len--; + } while (len > 0); + + return count; +} + static struct i2c_client *at24_translate_offset(struct at24_data *at24, unsigned int *offset) { @@ -286,10 +369,8 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf, */ read_time = jiffies; - status = i2c_smbus_read_i2c_block_data_or_emulated(client, - offset, - count, buf); - + status = at24_smbus_read_block_data(at24, client, + offset, count, buf); dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n", count, offset, status, jiffies); @@ -508,8 +589,8 @@ static ssize_t at24_eeprom_write_smbus_block(struct at24_data *at24, */ write_time = jiffies; - status = i2c_smbus_write_i2c_block_data(client, - offset, count, buf); + status = at24_smbus_write_i2c_block_data(at24, + client, offset, count, buf); if (status == 0) status = count; @@ -543,7 +624,8 @@ static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24, */ write_time = jiffies; - status = i2c_smbus_write_byte_data(client, offset, buf[0]); + status = at24_smbus_write_byte_data(at24, client, offset, + buf[0]); if (status == 0) status = count; @@ -810,13 +892,32 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) /* Use I2C operations unless we're stuck with SMBus extensions. */ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { - if (chip.flags & AT24_FLAG_ADDR16) - return -EPFNOSUPPORT; - - if (i2c_check_functionality(client->adapter, + if ((chip.flags & AT24_FLAG_ADDR16) && + i2c_check_functionality(client->adapter, + I2C_FUNC_SMBUS_READ_BYTE | + I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) { + /* + * We need SMBUS_WRITE_BYTE_DATA and SMBUS_READ_BYTE to + * implement byte reads for 16-bit address devices. + * This will be slow, but better than nothing (e.g. + * read @ 3.6 KiB/s). It is also unsafe in a multi- + * master topology. + */ + use_smbus = I2C_SMBUS_BYTE_DATA; + } else if (i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_I2C_BLOCK)) { use_smbus = I2C_SMBUS_I2C_BLOCK_DATA; - } else if (i2c_check_functionality(client->adapter, + } else if ((chip.flags & AT24_FLAG_ADDR16) && + i2c_check_functionality(client->adapter, + I2C_FUNC_SMBUS_WRITE_WORD_DATA)) { + /* + * We need SMBUS_WRITE_WORD_DATA to implement + * byte writes for 16-bit address devices. + */ + use_smbus_write = I2C_SMBUS_BYTE_DATA; + chip.page_size = 1; + } else if (!(chip.flags & AT24_FLAG_ADDR16) && + i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_READ_WORD_DATA)) { use_smbus = I2C_SMBUS_WORD_DATA; } else if (i2c_check_functionality(client->adapter, @@ -836,6 +937,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) } } + if (strcmp(client->name, "24c256") == 0) + chip.page_size = 64; + if (chip.flags & AT24_FLAG_TAKE8ADDR) num_addresses = 8; else @@ -881,12 +985,15 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id) if (writable) { if (!use_smbus || use_smbus_write) { - unsigned write_max = chip.page_size; + unsigned int write_max = chip.page_size; + unsigned int smbus_max = (chip.flags & AT24_FLAG_ADDR16) ? + I2C_SMBUS_BLOCK_MAX - 1 : + I2C_SMBUS_BLOCK_MAX; if (write_max > io_limit) write_max = io_limit; - if (use_smbus && write_max > I2C_SMBUS_BLOCK_MAX) - write_max = I2C_SMBUS_BLOCK_MAX; + if (use_smbus && write_max > smbus_max) + write_max = smbus_max; at24->write_max = write_max; /* buffer (data + address at the beginning) */