| Message ID | 20200504111501.456925-2-acelan.kao@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | add 16-bit width registers support for EEPROM at24 device | expand |
On 04.05.20 13:15, AceLan Kao wrote: > BugLink: https://bugs.launchpad.net/bugs/1876699 > > This allows to access data with 16-bit width of registers > via i2c SMBus block functions. > > The multi-command sequence of the reading 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. > > Read performance: > 32768 bytes (33 kB, 32 KiB) copied, 11.4869 s, 2.9 kB/s > Write performance(with 1-byte page): > 32768 bytes (33 kB, 32 KiB) copied, 129.591 s, 0.3 kB/s > > The implementation is inspired by below commit > https://patchwork.ozlabs.org/patch/545292/ > > v2: add more descriptions about the issue that maybe introduced > by this commit > > Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > --- Somehow neither the commit message nor the justification in the bug report are explaining what benefit (what is fixed) of this change is. Why would we want to pick this up? If there is a better reason, this is now at least in linux-next: commit 82f25bd73c0bee4d29df47007a4f7290695b7db7 so it could be a proper cherry-pick. -Stefan > drivers/base/regmap/regmap-i2c.c | 61 ++++++++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c > index ac9b31c57967..7f924797be58 100644 > --- a/drivers/base/regmap/regmap-i2c.c > +++ b/drivers/base/regmap/regmap-i2c.c > @@ -246,6 +246,63 @@ static struct regmap_bus regmap_i2c_smbus_i2c_block = { > .max_raw_write = I2C_SMBUS_BLOCK_MAX, > }; > > +static int regmap_i2c_smbus_i2c_write_reg16(void *context, const void *data, > + size_t count) > +{ > + struct device *dev = context; > + struct i2c_client *i2c = to_i2c_client(dev); > + > + if (count < 2) > + return -EINVAL; > + > + count--; > + return i2c_smbus_write_i2c_block_data(i2c, ((u8 *)data)[0], count, > + (u8 *)data + 1); > +} > + > +static int regmap_i2c_smbus_i2c_read_reg16(void *context, const void *reg, > + size_t reg_size, void *val, > + size_t val_size) > +{ > + struct device *dev = context; > + struct i2c_client *i2c = to_i2c_client(dev); > + int ret, count, len = val_size; > + > + if (reg_size != 2) > + return -EINVAL; > + > + ret = i2c_smbus_write_byte_data(i2c, ((u16 *)reg)[0] & 0xff, > + ((u16 *)reg)[0] >> 8); > + if (ret < 0) > + return ret; > + > + count = 0; > + do { > + /* Current Address Read */ > + ret = i2c_smbus_read_byte(i2c); > + if (ret < 0) > + break; > + > + *((u8 *)val++) = ret; > + count++; > + len--; > + } while (len > 0); > + > + if (count == val_size) > + return 0; > + else if (ret < 0) > + return ret; > + else > + return -EIO; > +} > + > +static const struct regmap_bus regmap_i2c_smbus_i2c_block_reg16 = { > + .write = regmap_i2c_smbus_i2c_write_reg16, > + .read = regmap_i2c_smbus_i2c_read_reg16, > + .max_raw_read = I2C_SMBUS_BLOCK_MAX, > + .max_raw_write = I2C_SMBUS_BLOCK_MAX, > +}; > + > static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c, > const struct regmap_config *config) > { > @@ -255,6 +312,10 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c, > i2c_check_functionality(i2c->adapter, > I2C_FUNC_SMBUS_I2C_BLOCK)) > return ®map_i2c_smbus_i2c_block; > + else if (config->val_bits == 8 && config->reg_bits == 16 && > + i2c_check_functionality(i2c->adapter, > + I2C_FUNC_SMBUS_I2C_BLOCK)) > + return ®map_i2c_smbus_i2c_block_reg16; > else if (config->val_bits == 16 && config->reg_bits == 8 && > i2c_check_functionality(i2c->adapter, > I2C_FUNC_SMBUS_WORD_DATA)) >
Hi Stefan, Stefan Bader <stefan.bader@canonical.com> 於 2020年5月13日 週三 下午4:35寫道： > > On 04.05.20 13:15, AceLan Kao wrote: > > BugLink: https://bugs.launchpad.net/bugs/1876699 > > > > This allows to access data with 16-bit width of registers > > via i2c SMBus block functions. > > > > The multi-command sequence of the reading 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. > > > > Read performance: > > 32768 bytes (33 kB, 32 KiB) copied, 11.4869 s, 2.9 kB/s > > Write performance(with 1-byte page): > > 32768 bytes (33 kB, 32 KiB) copied, 129.591 s, 0.3 kB/s > > > > The implementation is inspired by below commit > > https://patchwork.ozlabs.org/patch/545292/ > > > > v2: add more descriptions about the issue that maybe introduced > > by this commit > > > > Signed-off-by: AceLan Kao <acelan.kao@canonical.com> > > --- > > Somehow neither the commit message nor the justification in the bug report are > explaining what benefit (what is fixed) of this change is. Why would we want to > pick this up? On cover letter, the impact section it says "Larger EEPROM devices that use 16-bit addresses couldn't be accessed." We need this because we have a project that comes with an EEPROM which uses 16-bit address to access the data, without this commit, we can read/write data on it. Do you need me add some more wordings on SRU justification? > > If there is a better reason, this is now at least in linux-next: > > commit 82f25bd73c0bee4d29df47007a4f7290695b7db7 > > so it could be a proper cherry-pick. Cool, I didn't notice this, I'll prepare v3 and cherry pick from linux-next > > -Stefan > > drivers/base/regmap/regmap-i2c.c | 61 ++++++++++++++++++++++++++++++++ > > 1 file changed, 61 insertions(+) > > > > diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c > > index ac9b31c57967..7f924797be58 100644 > > --- a/drivers/base/regmap/regmap-i2c.c > > +++ b/drivers/base/regmap/regmap-i2c.c > > @@ -246,6 +246,63 @@ static struct regmap_bus regmap_i2c_smbus_i2c_block = { > > .max_raw_write = I2C_SMBUS_BLOCK_MAX, > > }; > > > > +static int regmap_i2c_smbus_i2c_write_reg16(void *context, const void *data, > > + size_t count) > > +{ > > + struct device *dev = context; > > + struct i2c_client *i2c = to_i2c_client(dev); > > + > > + if (count < 2) > > + return -EINVAL; > > + > > + count--; > > + return i2c_smbus_write_i2c_block_data(i2c, ((u8 *)data)[0], count, > > + (u8 *)data + 1); > > +} > > + > > +static int regmap_i2c_smbus_i2c_read_reg16(void *context, const void *reg, > > + size_t reg_size, void *val, > > + size_t val_size) > > +{ > > + struct device *dev = context; > > + struct i2c_client *i2c = to_i2c_client(dev); > > + int ret, count, len = val_size; > > + > > + if (reg_size != 2) > > + return -EINVAL; > > + > > + ret = i2c_smbus_write_byte_data(i2c, ((u16 *)reg)[0] & 0xff, > > + ((u16 *)reg)[0] >> 8); > > + if (ret < 0) > > + return ret; > > + > > + count = 0; > > + do { > > + /* Current Address Read */ > > + ret = i2c_smbus_read_byte(i2c); > > + if (ret < 0) > > + break; > > + > > + *((u8 *)val++) = ret; > > + count++; > > + len--; > > + } while (len > 0); > > + > > + if (count == val_size) > > + return 0; > > + else if (ret < 0) > > + return ret; > > + else > > + return -EIO; > > +} > > + > > +static const struct regmap_bus regmap_i2c_smbus_i2c_block_reg16 = { > > + .write = regmap_i2c_smbus_i2c_write_reg16, > > + .read = regmap_i2c_smbus_i2c_read_reg16, > > + .max_raw_read = I2C_SMBUS_BLOCK_MAX, > > + .max_raw_write = I2C_SMBUS_BLOCK_MAX, > > +}; > > + > > static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c, > > const struct regmap_config *config) > > { > > @@ -255,6 +312,10 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c, > > i2c_check_functionality(i2c->adapter, > > I2C_FUNC_SMBUS_I2C_BLOCK)) > > return ®map_i2c_smbus_i2c_block; > > + else if (config->val_bits == 8 && config->reg_bits == 16 && > > + i2c_check_functionality(i2c->adapter, > > + I2C_FUNC_SMBUS_I2C_BLOCK)) > > + return ®map_i2c_smbus_i2c_block_reg16; > > else if (config->val_bits == 16 && config->reg_bits == 8 && > > i2c_check_functionality(i2c->adapter, > > I2C_FUNC_SMBUS_WORD_DATA)) > > > >
diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c index ac9b31c57967..7f924797be58 100644 --- a/drivers/base/regmap/regmap-i2c.c +++ b/drivers/base/regmap/regmap-i2c.c @@ -246,6 +246,63 @@ static struct regmap_bus regmap_i2c_smbus_i2c_block = { .max_raw_write = I2C_SMBUS_BLOCK_MAX, }; +static int regmap_i2c_smbus_i2c_write_reg16(void *context, const void *data, + size_t count) +{ + struct device *dev = context; + struct i2c_client *i2c = to_i2c_client(dev); + + if (count < 2) + return -EINVAL; + + count--; + return i2c_smbus_write_i2c_block_data(i2c, ((u8 *)data)[0], count, + (u8 *)data + 1); +} + +static int regmap_i2c_smbus_i2c_read_reg16(void *context, const void *reg, + size_t reg_size, void *val, + size_t val_size) +{ + struct device *dev = context; + struct i2c_client *i2c = to_i2c_client(dev); + int ret, count, len = val_size; + + if (reg_size != 2) + return -EINVAL; + + ret = i2c_smbus_write_byte_data(i2c, ((u16 *)reg)[0] & 0xff, + ((u16 *)reg)[0] >> 8); + if (ret < 0) + return ret; + + count = 0; + do { + /* Current Address Read */ + ret = i2c_smbus_read_byte(i2c); + if (ret < 0) + break; + + *((u8 *)val++) = ret; + count++; + len--; + } while (len > 0); + + if (count == val_size) + return 0; + else if (ret < 0) + return ret; + else + return -EIO; +} + +static const struct regmap_bus regmap_i2c_smbus_i2c_block_reg16 = { + .write = regmap_i2c_smbus_i2c_write_reg16, + .read = regmap_i2c_smbus_i2c_read_reg16, + .max_raw_read = I2C_SMBUS_BLOCK_MAX, + .max_raw_write = I2C_SMBUS_BLOCK_MAX, +}; + static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c, const struct regmap_config *config) { @@ -255,6 +312,10 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c, i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_I2C_BLOCK)) return ®map_i2c_smbus_i2c_block; + else if (config->val_bits == 8 && config->reg_bits == 16 && + i2c_check_functionality(i2c->adapter, + I2C_FUNC_SMBUS_I2C_BLOCK)) + return ®map_i2c_smbus_i2c_block_reg16; else if (config->val_bits == 16 && config->reg_bits == 8 && i2c_check_functionality(i2c->adapter, I2C_FUNC_SMBUS_WORD_DATA))
BugLink: https://bugs.launchpad.net/bugs/1876699 This allows to access data with 16-bit width of registers via i2c SMBus block functions. The multi-command sequence of the reading 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. Read performance: 32768 bytes (33 kB, 32 KiB) copied, 11.4869 s, 2.9 kB/s Write performance(with 1-byte page): 32768 bytes (33 kB, 32 KiB) copied, 129.591 s, 0.3 kB/s The implementation is inspired by below commit https://patchwork.ozlabs.org/patch/545292/ v2: add more descriptions about the issue that maybe introduced by this commit Signed-off-by: AceLan Kao <acelan.kao@canonical.com> --- drivers/base/regmap/regmap-i2c.c | 61 ++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)