NAK: [PATCH v2 1/1][SRU][Bionic] UBUNTU: SAUCE: at24: add 16-bit width registers support
AceLan Kao
acelan.kao at canonical.com
Mon May 18 07:05:34 UTC 2020
I granted the permission from the author to add his s-o-b
Signed-off-by: Pieri <pierluigi.driusso at 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 at 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 at 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 at 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 at 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 at lists.ubuntu.com
> > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
More information about the kernel-team
mailing list