Messages in this thread | | | Date | Thu, 19 Apr 2012 10:52:23 -0700 | Subject | Re: [PATCH v3 2/2] staging: iio: light: isl29018: use regmap for register access | From | Grant Grundler <> |
| |
On Thu, Apr 19, 2012 at 4:15 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote: > Using regmap for accessing register through i2c bus. This will > remove the code for caching registers, read-modify-write logics. > Also it will provide the debugfs feature to dump register > through regmap debugfs. > > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
Reviewed-by: Grant Grundler <grundler@chromium.org>
Laxman, Thanks for reposting this patch. I was talking with Bryan Freed and it looks like the caching of registers will change the usage of ADD_COMMAND1. More details below.
... > -static int isl29018_write_data(struct i2c_client *client, u8 reg, > - u8 val, u8 mask, u8 shift) > -{ > - u8 regval = val; > - int ret; > - struct isl29018_chip *chip = iio_priv(i2c_get_clientdata(client)); > - > - /* don't cache or mask REG_TEST */ > - if (reg < ISL29018_MAX_REGS) { > - regval = chip->reg_cache[reg]; > - regval &= ~mask; > - regval |= val << shift; > - }
Note the only REG_TEST isn't cached. Everything else updates the cache on write.
> - > - ret = i2c_smbus_write_byte_data(client, reg, regval); > - if (ret) { > - dev_err(&client->dev, "Write to device fails status %x\n", ret); > - } else { > - /* don't update cache on err */ > - if (reg < ISL29018_MAX_REGS) > - chip->reg_cache[reg] = regval; > - } > - > - return ret; > -}
... > -static int isl29018_read_sensor_input(struct i2c_client *client, int mode) > +static int isl29018_read_sensor_input(struct isl29018_chip *chip, int mode) > { > int status; > - int lsb; > - int msb; > + unsigned int lsb; > + unsigned int msb; > > /* Set mode */ > - status = isl29018_write_data(client, ISL29018_REG_ADD_COMMAND1, > - mode, COMMMAND1_OPMODE_MASK, COMMMAND1_OPMODE_SHIFT); > + status = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMAND1, > + COMMMAND1_OPMODE_MASK, mode << COMMMAND1_OPMODE_SHIFT);
The old code will do a single I2C write. The new code will do a read+write for ADD_COMMAND1 register. I reviewed the behaviors in drivers/base/regmap/regmap.c and regcache.c.
I can't judge if this is a "real" problem. Given the limited BW on I2C, someone with more I2C experience should carefully consider and share their opinion.
> if (status) { > - dev_err(&client->dev, "Error in setting operating mode\n"); > + dev_err(chip->dev, > + "Error in setting operating mode err %d\n", status); > return status; > } > msleep(CONVERSION_TIME_MS); > - lsb = i2c_smbus_read_byte_data(client, ISL29018_REG_ADD_DATA_LSB);
Old code for DATA_LSB and DATA_MSB ignored the "write cache" for reads - so marking these as volatile looks correct to me.
... > +static bool is_volatile_reg(struct device *dev, unsigned int reg) > +{ > + switch (reg) { > + case ISL29018_REG_ADD_DATA_LSB: > + case ISL29018_REG_ADD_DATA_MSB: > + case ISL29018_REG_ADD_COMMAND1: > + case ISL29018_REG_TEST:
Of these four, I think only ADD_COMMAND1 wasn't treated as volatile in the old code. Am I overlooking something?
My concern is only about the additional I2C read traffic this patch might generate. It's possible *some* bits in that register are volatile and we could previously ignore them.
cheers, grant -- 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/
| |