On Thu, Apr 19, 2012 at 4:15 AM, Laxman Dewangan <ldewangan@xxxxxxxxxx> 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@xxxxxxxxxx>
Reviewed-by: Grant Grundler <grundler@xxxxxxxxxxxx>
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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
