Re: [PATCH v3 2/2] staging: iio: light: isl29018: use regmap forregister access

[Grant Grundler]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/