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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux