staging: iio: light: use regmap in isl29018 for register access
Subsystems (3)
v2 cover staging: iio: light: use regmap in isl29018 for register access
This is patch series for using regmap framework for accessing device register. THe first patch is for using tab for ndenting and second patch is for using regmap apis for register accesses.
Changes from V2:
Taken care of Lars's review comment.
Laxman Dewangan (2):
staging: iio: light: convert multiple spaces to tab
staging: iio: light: isl29018: use regmap for register access
drivers/staging/iio/light/Kconfig | 19 ++-- drivers/staging/iio/light/isl29018.c | 176 +++++++++++++++++----------------- 2 files changed, 99 insertions(+), 96 deletions(-)
PATCH v3 1/2 staging: iio: light: convert multiple spaces to tab
Using tab inplace of multiple spaces for indenting.
---
As per Jonathan's feedback, creating this patch for only indenting cleanups.
No changes from V2.
drivers/staging/iio/light/Kconfig | 18 +++++++++--------- 1 files changed, 9 insertions(+), 9 deletions(-)
drivers/staging/iio/light/Kconfig +9 -9
@@ -4,15 +4,15 @@ menu "Light sensors" config SENSORS_ISL29018 - tristate "ISL 29018 light and proximity sensor" - depends on I2C - default n - help - If you say yes here you get support for ambient light sensing and - proximity infrared sensing from Intersil ISL29018. - This driver will provide the measurements of ambient light intensity - in lux, proximity infrared sensing and normal infrared sensing. - Data from sensor is accessible via sysfs. + tristate "ISL 29018 light and proximity sensor" + depends on I2C + default n + help + If you say yes here you get support for ambient light sensing and + proximity infrared sensing from Intersil ISL29018. + This driver will provide the measurements of ambient light intensity + in lux, proximity infrared sensing and normal infrared sensing. + Data from sensor is accessible via sysfs. config SENSORS_ISL29028 tristate "Intersil ISL29028 Concurrent Light and Proximity Sensor"
Signature · 1 line
1.7.1.1
PATCH v3 2/2 staging: iio: light: isl29018: use regmap for register access
In reply to Laxman DewanganUsing 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.
---
Change from V1:
- Spliting the changes in two patches.
- Separate patch for indenting cleanups.
- Rebased the code on top of -next which have Jonathan's recent changes.
Changes from V2:
- Taken care of Lars's review comment.
drivers/staging/iio/light/Kconfig | 1 + drivers/staging/iio/light/isl29018.c | 178 +++++++++++++++++----------------- 2 files changed, 91 insertions(+), 88 deletions(-)
drivers/staging/iio/light/Kconfig +1 -0
@@ -6,6 +6,7 @@ menu "Light sensors" config SENSORS_ISL29018 tristate "ISL 29018 light and proximity sensor" depends on I2C + select REGMAP_I2C default n help If you say yes here you get support for ambient light sensing and
drivers/staging/iio/light/isl29018.c +90 -88
366 lines not on this page · raw message
Signature · 1 line
1.7.1.1
Re: [PATCH v3 2/2] staging: iio: light: isl29018: use regmap for register access
On Thu, Apr 19, 2012 at 4:15 AM, Laxman Dewangan <[email protected]> wro= te:
Quoted 6 lines
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 <[email protected]>
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.
...
Quoted 2 lines
-static int isl29018_write_data(struct
i2c_client*client, u8 reg,
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 u8 val, u8 mask, u8 shift)
Quoted 4 lines
-{
- =C2=A0 =C2=A0 =C2=A0 u8 regval =3D val;
- =C2=A0 =C2=A0 =C2=A0 int ret;
- =C2=A0 =C2=A0 =C2=A0 structisl29018_chip*chip =3D iio_priv(i2c_get_cl=
ientdata(client));
Quoted 4 lines
-
- =C2=A0 =C2=A0 =C2=A0 /* don't cache or maskREG_TEST*/
- =C2=A0 =C2=A0 =C2=A0 if (reg <ISL29018_MAX_REGS) {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 regval =3D chip->reg_c=
ache[reg];
Quoted 2 lines
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 regval &=3D ~mask;
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 regval |=3D val << shi=
ft;
Quoted 1 line
- =C2=A0 =C2=A0 =C2=A0 }
Note the only REG_TEST isn't cached. Everything else updates the cache on write.
Quoted 2 lines
-
- =C2=A0 =C2=A0 =C2=A0 ret =3D i2c_smbus_write_byte_data(client, reg, reg=
val);
Quoted 2 lines
- =C2=A0 =C2=A0 =C2=A0 if (ret) {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_err(&client->dev, =
"Write to device fails status %x\n", ret);
Quoted 2 lines
- =C2=A0 =C2=A0 =C2=A0 } else {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* don't update cache =
on err */
Quoted 1 line
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (reg < ISL29018_MAX=
_REGS)
Quoted 1 line
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 chip->reg_cache[reg] =3D regval;
Quoted 4 lines
- =C2=A0 =C2=A0 =C2=A0 }
drivers/staging/iio/light/isl29018.c +0
-3- - =C2=A0 =C2=A0 =C2=A0 return ret; -}
...
Quoted 1 line
-static int isl29018_read_sensor_input(struct
i2c_client*client, int mod=
e)
Quoted 1 line
+static int isl29018_read_sensor_input(struct
isl29018_chip*chip, int mo=
de)
Quoted 9 lines
=C2=A0{
=C2=A0 =C2=A0 =C2=A0 =C2=A0int status;
- =C2=A0 =C2=A0 =C2=A0 int lsb;
- =C2=A0 =C2=A0 =C2=A0 int msb;
+ =C2=A0 =C2=A0 =C2=A0 unsigned int lsb;
+ =C2=A0 =C2=A0 =C2=A0 unsigned int msb;=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Set mode */
- =C2=A0 =C2=A0 =C2=A0 status =3D isl29018_write_data(client, ISL29018_RE=
G_ADD_COMMAND1,
Quoted 1 line
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 mode, COMMMAND1_OPMODE_MASK, COMMMAND1_OPMODE_SHIFT);
Quoted 1 line
+ =C2=A0 =C2=A0 =C2=A0 status =3D regmap_update_bits(chip->regmap, ISL290=
18_REG_ADD_COMMAND1,
Quoted 1 line
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 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.
Quoted 2 lines
=C2=A0 =C2=A0 =C2=A0 =C2=A0if (status) {
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_err(&client->dev, =
"Error in setting operating mode\n");
Quoted 2 lines
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 dev_err(chip->dev,
+ =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =
=C2=A0 "Error in setting operating mode err %d\n", status);
Quoted 4 lines
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return status; =C2=A0 =C2=A0 =C2=A0 =C2=A0}
=C2=A0 =C2=A0 =C2=A0 =C2=A0msleep(CONVERSION_TIME_MS);
- =C2=A0 =C2=A0 =C2=A0 lsb =3D 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.
...
Review
Quoted 7 lines
(quoted diff) +7
-0+static bool is_volatile_reg(struct device *dev, unsigned int reg) +{ + =C2=A0 =C2=A0 =C2=A0 switch (reg) { + =C2=A0 =C2=A0 =C2=A0 case ISL29018_REG_ADD_DATA_LSB: + =C2=A0 =C2=A0 =C2=A0 case ISL29018_REG_ADD_DATA_MSB: + =C2=A0 =C2=A0 =C2=A0 case ISL29018_REG_ADD_COMMAND1: + =C2=A0 =C2=A0 =C2=A0 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
Re: [PATCH v3 2/2] staging: iio: light: isl29018: use regmap for register access
Quoted 6 lines · Grant Grundler
Quoted 5 lines · Laxman Dewangan
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.
Quoted 8 lines
Quoted 1 line
Signed-off-by Laxman Dewangan<[email protected]>Reviewed-by Grant Grundler<[email protected]>Laxman,
Thanks for reposting this patch. I was talking with Bryan Freed and it looks like the caching of registers will change the usage ofADD_COMMAND1. More details below.
Thanks for review. ADD_COMMAND1 have the intrrupt flag bit. More details below.
Quoted 10 lines
drivers/staging/iio/light/isl29018.c static const struct iio_info isl29108_info = { · line 508
Quoted 7 lines
drivers/staging/iio/light/isl29018.c +7
-0+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_COMMAND1wasn't treated as volatile in the old code. Am I overlooking something?
Quoted 4 lines
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.
Register ADD_COMMAND1, bit 2 is interrupt flag bit which shows the interrupt status and hence we can not cache it. The ISL29018 datasheet says:
Interrupt flag; Bit 2. This is the status bit of the interrupt.
The bit is set to logic high when the interrupt thresholds
have been triggered, and logic low when not yet triggered.
Once triggered, INT pin stays low and the status bit stays
high. Both interrupt pin and the status bit are automatically
cleared at the end of Command Register I transfer.
Re: [PATCH v3 2/2] staging: iio: light: isl29018: use regmap for register access
Quoted 34 lines · Laxman Dewangan
Quoted 7 lines · Grant Grundler
Quoted 5 lines · Laxman Dewangan
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.
Quoted 8 lines
Quoted 1 line
Signed-off-by Laxman Dewangan<[email protected]>Reviewed-by Grant Grundler<[email protected]>Laxman,
Thanks for reposting this patch. I was talking with Bryan Freed and it looks like the caching of registers will change the usage ofADD_COMMAND1. More details below.Thanks for review.
ADD_COMMAND1have the intrrupt flag bit. More details below.Quoted 10 lines
drivers/staging/iio/light/isl29018.c static const struct iio_info isl29108_info = { · line 508
Quoted 7 lines
drivers/staging/iio/light/isl29018.c +7
-0+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_COMMAND1wasn't treated as volatile in the old code. Am I overlooking something?Quoted 4 lines
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.
Quoted 9 lines
Register
ADD_COMMAND1, bit 2 is interrupt flag bit which shows the interrupt status and hence we can not cache it.
The ISL29018 datasheet says:
Interrupt flag; Bit 2. This is the status bit of the interrupt. The bit is set to logic high when the interrupt thresholds have been triggered, and logic low when not yet triggered. Once triggered, INT pin stays low and the status bit stays high. Both interrupt pin and the status bit are automatically cleared at the end of Command Register I transfer.
If the bit is cleared when reading the register I suppose it is not being worth much to mark the register as volatile since the bit will be cleared whenever you update the register. If there is only opmode and the irq bit in that register I'd keep the register volatile, but use regmap_write instead of regmap_update_bits.
Re: [PATCH v3 2/2] staging: iio: light: isl29018: use regmap for register access
Quoted 27 lines · Lars-Peter Clausen
Quoted 17 lines · Laxman Dewangan
Quoted 12 lines · Grant Grundler
drivers/staging/iio/light/isl29018.c static const struct iio_info isl29108_info = { · line 508
Quoted 7 lines · Laxman Dewangan
drivers/staging/iio/light/isl29018.c +7
-0+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_COMMAND1wasn't treated as volatile in the old code. Am I overlooking something?Quoted 4 lines
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.
Quoted 9 lines
Register
ADD_COMMAND1, bit 2 is interrupt flag bit which shows the interrupt status and hence we can not cache it.
The ISL29018 datasheet says:
Interrupt flag; Bit 2. This is the status bit of the interrupt. The bit is set to logic high when the interrupt thresholds have been triggered, and logic low when not yet triggered. Once triggered, INT pin stays low and the status bit stays high. Both interrupt pin and the status bit are automatically cleared at the end of Command Register I transfer.
Quoted 5 lines
If the bit is cleared when reading the register I suppose it is not being worth much to mark the register as volatile since the bit will be cleared whenever you update the register. If there is only opmode and the irq bit in that register I'd keep the register volatile, but use
regmap_writeinstead ofregmap_update_bits.
There is 2-bits in ADD_COMMAND1 for interrupt persistence.
The interrupt pin and the interrupt flag is triggered/set when the data sensor reading is out of the interrupt threshold window after m consecutive number of integration cycles.
The driver is not supporting interrupt now. I think we can use regmap_write() in place of regmap_update_bits() keeping this volatile as you suggested. When we add interrupt support on this driver, we will write the register after ORing with persistence bits. For that also it will not require to do caching of register.