[PATCH] staging: iio: light: isl29018: use regmap for register access

[PATCH] staging: iio: light: isl29018: use regmap for register access

[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.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/staging/iio/light/Kconfig    |   19 ++--
 drivers/staging/iio/light/isl29018.c |  176 +++++++++++++++++-----------------
 2 files changed, 99 insertions(+), 96 deletions(-)

diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
index 53b49f7..fd39f72 100644
--- a/drivers/staging/iio/light/Kconfig
+++ b/drivers/staging/iio/light/Kconfig
@@ -4,15 +4,16 @@
 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
+	select REGMAP_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"
diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index 38ec52b..6682e84 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -26,9 +26,11 @@
 #include <linux/err.h>
 #include <linux/mutex.h>
 #include <linux/delay.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include "../iio.h"
 #include "../sysfs.h"
+
 #define CONVERSION_TIME_MS		100
 
 #define ISL29018_REG_ADD_COMMAND1	0x00
@@ -51,49 +53,22 @@
 
 #define ISL29018_REG_ADD_DATA_LSB	0x02
 #define ISL29018_REG_ADD_DATA_MSB	0x03
-#define ISL29018_MAX_REGS		(ISL29018_REG_ADD_DATA_MSB+1)
 
 #define ISL29018_REG_TEST		0x08
 #define ISL29018_TEST_SHIFT		0
 #define ISL29018_TEST_MASK		(0xFF << ISL29018_TEST_SHIFT)
 
 struct isl29018_chip {
-	struct i2c_client	*client;
+	struct device		*dev;
+	struct regmap		*regmap;
 	struct mutex		lock;
 	unsigned int		lux_scale;
 	unsigned int		range;
 	unsigned int		adc_bit;
 	int			prox_scheme;
-	u8			reg_cache[ISL29018_MAX_REGS];
 };
 
-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;
-	}
-
-	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_set_range(struct i2c_client *client, unsigned long range,
+static int isl29018_set_range(struct isl29018_chip *chip, unsigned long range,
 		unsigned int *new_range)
 {
 	static const unsigned long supp_ranges[] = {1000, 4000, 16000, 64000};
@@ -109,11 +84,11 @@ static int isl29018_set_range(struct i2c_client *client, unsigned long range,
 	if (i >= ARRAY_SIZE(supp_ranges))
 		return -EINVAL;
 
-	return isl29018_write_data(client, ISL29018_REG_ADD_COMMANDII,
-			i, COMMANDII_RANGE_MASK, COMMANDII_RANGE_SHIFT);
+	return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
+			COMMANDII_RANGE_MASK, i << COMMANDII_RANGE_SHIFT);
 }
 
-static int isl29018_set_resolution(struct i2c_client *client,
+static int isl29018_set_resolution(struct isl29018_chip *chip,
 			unsigned long adcbit, unsigned int *conf_adc_bit)
 {
 	static const unsigned long supp_adcbit[] = {16, 12, 8, 4};
@@ -129,48 +104,49 @@ static int isl29018_set_resolution(struct i2c_client *client,
 	if (i >= ARRAY_SIZE(supp_adcbit))
 		return -EINVAL;
 
-	return isl29018_write_data(client, ISL29018_REG_ADD_COMMANDII,
-			i, COMMANDII_RESOLUTION_MASK,
-			COMMANDII_RESOLUTION_SHIFT);
+	return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
+			COMMANDII_RESOLUTION_MASK,
+			i << COMMANDII_RESOLUTION_SHIFT);
 }
 
-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);
 	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);
-	if (lsb < 0) {
-		dev_err(&client->dev, "Error in reading LSB DATA\n");
-		return lsb;
+	status = regmap_read(chip->regmap, ISL29018_REG_ADD_DATA_LSB, &lsb);
+	if (status < 0) {
+		dev_err(chip->dev,
+			"Error in reading LSB DATA with err %d\n", status);
+		return status;
 	}
 
-	msb = i2c_smbus_read_byte_data(client, ISL29018_REG_ADD_DATA_MSB);
-	if (msb < 0) {
-		dev_err(&client->dev, "Error in reading MSB DATA\n");
+	status = regmap_read(chip->regmap, ISL29018_REG_ADD_DATA_MSB, &msb);
+	if (status < 0) {
+		dev_err(chip->dev,
+			"Error in reading MSB DATA with error %d\n", status);
 		return msb;
 	}
-	dev_vdbg(&client->dev, "MSB 0x%x and LSB 0x%x\n", msb, lsb);
+	dev_vdbg(chip->dev, "MSB 0x%x and LSB 0x%x\n", msb, lsb);
 
 	return (msb << 8) | lsb;
 }
 
-static int isl29018_read_lux(struct i2c_client *client, int *lux)
+static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
 {
 	int lux_data;
-	struct isl29018_chip *chip = iio_priv(i2c_get_clientdata(client));
 
-	lux_data = isl29018_read_sensor_input(client,
-				COMMMAND1_OPMODE_ALS_ONCE);
+	lux_data = isl29018_read_sensor_input(chip, COMMMAND1_OPMODE_ALS_ONCE);
 
 	if (lux_data < 0)
 		return lux_data;
@@ -180,11 +156,11 @@ static int isl29018_read_lux(struct i2c_client *client, int *lux)
 	return 0;
 }
 
-static int isl29018_read_ir(struct i2c_client *client, int *ir)
+static int isl29018_read_ir(struct isl29018_chip *chip, int *ir)
 {
 	int ir_data;
 
-	ir_data = isl29018_read_sensor_input(client, COMMMAND1_OPMODE_IR_ONCE);
+	ir_data = isl29018_read_sensor_input(chip, COMMMAND1_OPMODE_IR_ONCE);
 
 	if (ir_data < 0)
 		return ir_data;
@@ -194,7 +170,7 @@ static int isl29018_read_ir(struct i2c_client *client, int *ir)
 	return 0;
 }
 
-static int isl29018_read_proximity_ir(struct i2c_client *client, int scheme,
+static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
 		int *near_ir)
 {
 	int status;
@@ -202,14 +178,15 @@ static int isl29018_read_proximity_ir(struct i2c_client *client, int scheme,
 	int ir_data = -1;
 
 	/* Do proximity sensing with required scheme */
-	status = isl29018_write_data(client, ISL29018_REG_ADD_COMMANDII,
-			scheme, COMMANDII_SCHEME_MASK, COMMANDII_SCHEME_SHIFT);
+	status = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
+			COMMANDII_SCHEME_MASK,
+			scheme << COMMANDII_SCHEME_SHIFT);
 	if (status) {
-		dev_err(&client->dev, "Error in setting operating mode\n");
+		dev_err(chip->dev, "Error in setting operating mode\n");
 		return status;
 	}
 
-	prox_data = isl29018_read_sensor_input(client,
+	prox_data = isl29018_read_sensor_input(chip,
 					COMMMAND1_OPMODE_PROX_ONCE);
 	if (prox_data < 0)
 		return prox_data;
@@ -219,8 +196,7 @@ static int isl29018_read_proximity_ir(struct i2c_client *client, int scheme,
 		return 0;
 	}
 
-	ir_data = isl29018_read_sensor_input(client,
-				COMMMAND1_OPMODE_IR_ONCE);
+	ir_data = isl29018_read_sensor_input(chip, COMMMAND1_OPMODE_IR_ONCE);
 
 	if (ir_data < 0)
 		return ir_data;
@@ -249,7 +225,6 @@ static ssize_t store_range(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
-	struct i2c_client *client = chip->client;
 	int status;
 	unsigned long lval;
 	unsigned int new_range;
@@ -264,10 +239,11 @@ static ssize_t store_range(struct device *dev,
 	}
 
 	mutex_lock(&chip->lock);
-	status = isl29018_set_range(client, lval, &new_range);
+	status = isl29018_set_range(chip, lval, &new_range);
 	if (status < 0) {
 		mutex_unlock(&chip->lock);
-		dev_err(dev, "Error in setting max range\n");
+		dev_err(dev,
+			"Error in setting max range with err %d\n", status);
 		return status;
 	}
 	chip->range = new_range;
@@ -291,7 +267,6 @@ static ssize_t store_resolution(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
 	struct isl29018_chip *chip = iio_priv(indio_dev);
-	struct i2c_client *client = chip->client;
 	int status;
 	unsigned long lval;
 	unsigned int new_adc_bit;
@@ -304,7 +279,7 @@ static ssize_t store_resolution(struct device *dev,
 	}
 
 	mutex_lock(&chip->lock);
-	status = isl29018_set_resolution(client, lval, &new_adc_bit);
+	status = isl29018_set_resolution(chip, lval, &new_adc_bit);
 	if (status < 0) {
 		mutex_unlock(&chip->lock);
 		dev_err(dev, "Error in setting resolution\n");
@@ -379,20 +354,19 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
 {
 	int ret = -EINVAL;
 	struct isl29018_chip *chip = iio_priv(indio_dev);
-	struct i2c_client *client = chip->client;
 
 	mutex_lock(&chip->lock);
 	switch (mask) {
 	case 0:
 		switch (chan->type) {
 		case IIO_LIGHT:
-			ret = isl29018_read_lux(client, val);
+			ret = isl29018_read_lux(chip, val);
 			break;
 		case IIO_INTENSITY:
-			ret = isl29018_read_ir(client, val);
+			ret = isl29018_read_ir(chip, val);
 			break;
 		case IIO_PROXIMITY:
-			ret = isl29018_read_proximity_ir(client,
+			ret = isl29018_read_proximity_ir(chip,
 					chip->prox_scheme, val);
 			break;
 		default:
@@ -456,15 +430,12 @@ static const struct attribute_group isl29108_group = {
 	.attrs = isl29018_attributes,
 };
 
-static int isl29018_chip_init(struct i2c_client *client)
+static int isl29018_chip_init(struct isl29018_chip *chip)
 {
-	struct isl29018_chip *chip = iio_priv(i2c_get_clientdata(client));
 	int status;
 	int new_adc_bit;
 	unsigned int new_range;
 
-	memset(chip->reg_cache, 0, sizeof(chip->reg_cache));
-
 	/* Code added per Intersil Application Note 1534:
 	 *     When VDD sinks to approximately 1.8V or below, some of
 	 * the part's registers may change their state. When VDD
@@ -485,10 +456,9 @@ static int isl29018_chip_init(struct i2c_client *client)
 	 * the same thing EXCEPT the data sheet asks for a 1ms delay after
 	 * writing the CMD1 register.
 	 */
-	status = isl29018_write_data(client, ISL29018_REG_TEST, 0,
-				ISL29018_TEST_MASK, ISL29018_TEST_SHIFT);
+	status = regmap_write(chip->regmap, ISL29018_REG_TEST, 0x0);
 	if (status < 0) {
-		dev_err(&client->dev, "Failed to clear isl29018 TEST reg."
+		dev_err(chip->dev, "Failed to clear isl29018 TEST reg."
 					"(%d)\n", status);
 		return status;
 	}
@@ -497,10 +467,9 @@ static int isl29018_chip_init(struct i2c_client *client)
 	 * "Operating Mode" (COMMAND1) register is reprogrammed when
 	 * data is read from the device.
 	 */
-	status = isl29018_write_data(client, ISL29018_REG_ADD_COMMAND1, 0,
-				0xff, 0);
+	status = regmap_write(chip->regmap, ISL29018_REG_ADD_COMMAND1, 0);
 	if (status < 0) {
-		dev_err(&client->dev, "Failed to clear isl29018 CMD1 reg."
+		dev_err(chip->dev, "Failed to clear isl29018 CMD1 reg."
 					"(%d)\n", status);
 		return status;
 	}
@@ -508,13 +477,13 @@ static int isl29018_chip_init(struct i2c_client *client)
 	msleep(1);	/* per data sheet, page 10 */
 
 	/* set defaults */
-	status = isl29018_set_range(client, chip->range, &new_range);
+	status = isl29018_set_range(chip, chip->range, &new_range);
 	if (status < 0) {
-		dev_err(&client->dev, "Init of isl29018 fails\n");
+		dev_err(chip->dev, "Init of isl29018 fails\n");
 		return status;
 	}
 
-	status = isl29018_set_resolution(client, chip->adc_bit,
+	status = isl29018_set_resolution(chip, chip->adc_bit,
 						&new_adc_bit);
 
 	return 0;
@@ -527,6 +496,32 @@ static const struct iio_info isl29108_info = {
 	.write_raw = &isl29018_write_raw,
 };
 
+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:
+		return true;
+	default:
+		return false;
+	}
+}
+
+/*
+ * isl29018_regmap_config: regmap configuration.
+ * Use RBTREE mechanism for caching.
+ */
+static const struct regmap_config isl29018_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_reg = is_volatile_reg,
+	.max_register = ISL29018_REG_TEST,
+	.num_reg_defaults_raw = ISL29018_REG_TEST + 1,
+	.cache_type = REGCACHE_RBTREE,
+};
+
 static int __devinit isl29018_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -543,7 +538,7 @@ static int __devinit isl29018_probe(struct i2c_client *client,
 	chip = iio_priv(indio_dev);
 
 	i2c_set_clientdata(client, indio_dev);
-	chip->client = client;
+	chip->dev = &client->dev;
 
 	mutex_init(&chip->lock);
 
@@ -551,7 +546,14 @@ static int __devinit isl29018_probe(struct i2c_client *client,
 	chip->range = 1000;
 	chip->adc_bit = 16;
 
-	err = isl29018_chip_init(client);
+	chip->regmap = devm_regmap_init_i2c(client, &isl29018_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		err = PTR_ERR(chip->regmap);
+		dev_err(chip->dev, "regmap initialization failed: %d\n", err);
+		goto exit;
+	}
+
+	err = isl29018_chip_init(chip);
 	if (err)
 		goto exit_iio_free;
 
-- 
1.7.1.1

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

[Jonathan Cameron]

On 4/17/2012 9:50 AM, Laxman Dewangan 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.
I'd prefer the intial tab fixup for the kconfig file as a separate 
patch, but other than that
all looks good.

This will probably cause issues alongside the series I sent to Greg the 
other day though
so you may want to sit on it for a day or two and rebase.
>
> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
>   drivers/staging/iio/light/Kconfig    |   19 ++--
>   drivers/staging/iio/light/isl29018.c |  176 +++++++++++++++++-----------------
>   2 files changed, 99 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
> index 53b49f7..fd39f72 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig
> @@ -4,15 +4,16 @@
>   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
> +	select REGMAP_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.

Down to here is a valid but unconnected change.  Can you break this out 
to a separate patch?
>
>   config SENSORS_ISL29028
>   	tristate "Intersil ISL29028 Concurrent Light and Proximity Sensor"
> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
> index 38ec52b..6682e84 100644
> --- a/drivers/staging/iio/light/isl29018.c
> +++ b/drivers/staging/iio/light/isl29018.c
> @@ -26,9 +26,11 @@
>   #include<linux/err.h>
>   #include<linux/mutex.h>
>   #include<linux/delay.h>
> +#include<linux/regmap.h>
>   #include<linux/slab.h>
>   #include "../iio.h"
>   #include "../sysfs.h"
> +
>   #define CONVERSION_TIME_MS		100
>
>   #define ISL29018_REG_ADD_COMMAND1	0x00
> @@ -51,49 +53,22 @@
>
>   #define ISL29018_REG_ADD_DATA_LSB	0x02
>   #define ISL29018_REG_ADD_DATA_MSB	0x03
> -#define ISL29018_MAX_REGS		(ISL29018_REG_ADD_DATA_MSB+1)
>
>   #define ISL29018_REG_TEST		0x08
>   #define ISL29018_TEST_SHIFT		0
>   #define ISL29018_TEST_MASK		(0xFF<<  ISL29018_TEST_SHIFT)
>
>   struct isl29018_chip {
> -	struct i2c_client	*client;
> +	struct device		*dev;
> +	struct regmap		*regmap;
>   	struct mutex		lock;
>   	unsigned int		lux_scale;
>   	unsigned int		range;
>   	unsigned int		adc_bit;
>   	int			prox_scheme;
> -	u8			reg_cache[ISL29018_MAX_REGS];
>   };
>
> -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;
> -	}
> -
> -	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_set_range(struct i2c_client *client, unsigned long range,
> +static int isl29018_set_range(struct isl29018_chip *chip, unsigned long range,
>   		unsigned int *new_range)
>   {
>   	static const unsigned long supp_ranges[] = {1000, 4000, 16000, 64000};
> @@ -109,11 +84,11 @@ static int isl29018_set_range(struct i2c_client *client, unsigned long range,
>   	if (i>= ARRAY_SIZE(supp_ranges))
>   		return -EINVAL;
>
> -	return isl29018_write_data(client, ISL29018_REG_ADD_COMMANDII,
> -			i, COMMANDII_RANGE_MASK, COMMANDII_RANGE_SHIFT);
> +	return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
> +			COMMANDII_RANGE_MASK, i<<  COMMANDII_RANGE_SHIFT);
>   }
>
> -static int isl29018_set_resolution(struct i2c_client *client,
> +static int isl29018_set_resolution(struct isl29018_chip *chip,
>   			unsigned long adcbit, unsigned int *conf_adc_bit)
>   {
>   	static const unsigned long supp_adcbit[] = {16, 12, 8, 4};
> @@ -129,48 +104,49 @@ static int isl29018_set_resolution(struct i2c_client *client,
>   	if (i>= ARRAY_SIZE(supp_adcbit))
>   		return -EINVAL;
>
> -	return isl29018_write_data(client, ISL29018_REG_ADD_COMMANDII,
> -			i, COMMANDII_RESOLUTION_MASK,
> -			COMMANDII_RESOLUTION_SHIFT);
> +	return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
> +			COMMANDII_RESOLUTION_MASK,
> +			i<<  COMMANDII_RESOLUTION_SHIFT);
>   }
>
> -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);
>   	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);
> -	if (lsb<  0) {
> -		dev_err(&client->dev, "Error in reading LSB DATA\n");
> -		return lsb;
> +	status = regmap_read(chip->regmap, ISL29018_REG_ADD_DATA_LSB,&lsb);
> +	if (status<  0) {
> +		dev_err(chip->dev,
> +			"Error in reading LSB DATA with err %d\n", status);
> +		return status;
>   	}
>
> -	msb = i2c_smbus_read_byte_data(client, ISL29018_REG_ADD_DATA_MSB);
> -	if (msb<  0) {
> -		dev_err(&client->dev, "Error in reading MSB DATA\n");
> +	status = regmap_read(chip->regmap, ISL29018_REG_ADD_DATA_MSB,&msb);
> +	if (status<  0) {
> +		dev_err(chip->dev,
> +			"Error in reading MSB DATA with error %d\n", status);
>   		return msb;
>   	}
> -	dev_vdbg(&client->dev, "MSB 0x%x and LSB 0x%x\n", msb, lsb);
> +	dev_vdbg(chip->dev, "MSB 0x%x and LSB 0x%x\n", msb, lsb);
>
>   	return (msb<<  8) | lsb;
>   }
>
> -static int isl29018_read_lux(struct i2c_client *client, int *lux)
> +static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
>   {
>   	int lux_data;
> -	struct isl29018_chip *chip = iio_priv(i2c_get_clientdata(client));
>
> -	lux_data = isl29018_read_sensor_input(client,
> -				COMMMAND1_OPMODE_ALS_ONCE);
> +	lux_data = isl29018_read_sensor_input(chip, COMMMAND1_OPMODE_ALS_ONCE);
>
>   	if (lux_data<  0)
>   		return lux_data;
> @@ -180,11 +156,11 @@ static int isl29018_read_lux(struct i2c_client *client, int *lux)
>   	return 0;
>   }
>
> -static int isl29018_read_ir(struct i2c_client *client, int *ir)
> +static int isl29018_read_ir(struct isl29018_chip *chip, int *ir)
>   {
>   	int ir_data;
>
> -	ir_data = isl29018_read_sensor_input(client, COMMMAND1_OPMODE_IR_ONCE);
> +	ir_data = isl29018_read_sensor_input(chip, COMMMAND1_OPMODE_IR_ONCE);
>
>   	if (ir_data<  0)
>   		return ir_data;
> @@ -194,7 +170,7 @@ static int isl29018_read_ir(struct i2c_client *client, int *ir)
>   	return 0;
>   }
>
> -static int isl29018_read_proximity_ir(struct i2c_client *client, int scheme,
> +static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
>   		int *near_ir)
>   {
>   	int status;
> @@ -202,14 +178,15 @@ static int isl29018_read_proximity_ir(struct i2c_client *client, int scheme,
>   	int ir_data = -1;
>
>   	/* Do proximity sensing with required scheme */
> -	status = isl29018_write_data(client, ISL29018_REG_ADD_COMMANDII,
> -			scheme, COMMANDII_SCHEME_MASK, COMMANDII_SCHEME_SHIFT);
> +	status = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
> +			COMMANDII_SCHEME_MASK,
> +			scheme<<  COMMANDII_SCHEME_SHIFT);
>   	if (status) {
> -		dev_err(&client->dev, "Error in setting operating mode\n");
> +		dev_err(chip->dev, "Error in setting operating mode\n");
>   		return status;
>   	}
>
> -	prox_data = isl29018_read_sensor_input(client,
> +	prox_data = isl29018_read_sensor_input(chip,
>   					COMMMAND1_OPMODE_PROX_ONCE);
>   	if (prox_data<  0)
>   		return prox_data;
> @@ -219,8 +196,7 @@ static int isl29018_read_proximity_ir(struct i2c_client *client, int scheme,
>   		return 0;
>   	}
>
> -	ir_data = isl29018_read_sensor_input(client,
> -				COMMMAND1_OPMODE_IR_ONCE);
> +	ir_data = isl29018_read_sensor_input(chip, COMMMAND1_OPMODE_IR_ONCE);
>
>   	if (ir_data<  0)
>   		return ir_data;
> @@ -249,7 +225,6 @@ static ssize_t store_range(struct device *dev,
>   {
>   	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>   	struct isl29018_chip *chip = iio_priv(indio_dev);
> -	struct i2c_client *client = chip->client;
>   	int status;
>   	unsigned long lval;
>   	unsigned int new_range;
> @@ -264,10 +239,11 @@ static ssize_t store_range(struct device *dev,
>   	}
>
>   	mutex_lock(&chip->lock);
> -	status = isl29018_set_range(client, lval,&new_range);
> +	status = isl29018_set_range(chip, lval,&new_range);
>   	if (status<  0) {
>   		mutex_unlock(&chip->lock);
> -		dev_err(dev, "Error in setting max range\n");
> +		dev_err(dev,
> +			"Error in setting max range with err %d\n", status);
>   		return status;
>   	}
>   	chip->range = new_range;
> @@ -291,7 +267,6 @@ static ssize_t store_resolution(struct device *dev,
>   {
>   	struct iio_dev *indio_dev = dev_get_drvdata(dev);
>   	struct isl29018_chip *chip = iio_priv(indio_dev);
> -	struct i2c_client *client = chip->client;
>   	int status;
>   	unsigned long lval;
>   	unsigned int new_adc_bit;
> @@ -304,7 +279,7 @@ static ssize_t store_resolution(struct device *dev,
>   	}
>
>   	mutex_lock(&chip->lock);
> -	status = isl29018_set_resolution(client, lval,&new_adc_bit);
> +	status = isl29018_set_resolution(chip, lval,&new_adc_bit);
>   	if (status<  0) {
>   		mutex_unlock(&chip->lock);
>   		dev_err(dev, "Error in setting resolution\n");
> @@ -379,20 +354,19 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
>   {
>   	int ret = -EINVAL;
>   	struct isl29018_chip *chip = iio_priv(indio_dev);
> -	struct i2c_client *client = chip->client;
>
>   	mutex_lock(&chip->lock);
>   	switch (mask) {
>   	case 0:
>   		switch (chan->type) {
>   		case IIO_LIGHT:
> -			ret = isl29018_read_lux(client, val);
> +			ret = isl29018_read_lux(chip, val);
>   			break;
>   		case IIO_INTENSITY:
> -			ret = isl29018_read_ir(client, val);
> +			ret = isl29018_read_ir(chip, val);
>   			break;
>   		case IIO_PROXIMITY:
> -			ret = isl29018_read_proximity_ir(client,
> +			ret = isl29018_read_proximity_ir(chip,
>   					chip->prox_scheme, val);
>   			break;
>   		default:
> @@ -456,15 +430,12 @@ static const struct attribute_group isl29108_group = {
>   	.attrs = isl29018_attributes,
>   };
>
> -static int isl29018_chip_init(struct i2c_client *client)
> +static int isl29018_chip_init(struct isl29018_chip *chip)
>   {
> -	struct isl29018_chip *chip = iio_priv(i2c_get_clientdata(client));
>   	int status;
>   	int new_adc_bit;
>   	unsigned int new_range;
>
> -	memset(chip->reg_cache, 0, sizeof(chip->reg_cache));
> -
>   	/* Code added per Intersil Application Note 1534:
>   	 *     When VDD sinks to approximately 1.8V or below, some of
>   	 * the part's registers may change their state. When VDD
> @@ -485,10 +456,9 @@ static int isl29018_chip_init(struct i2c_client *client)
>   	 * the same thing EXCEPT the data sheet asks for a 1ms delay after
>   	 * writing the CMD1 register.
>   	 */
> -	status = isl29018_write_data(client, ISL29018_REG_TEST, 0,
> -				ISL29018_TEST_MASK, ISL29018_TEST_SHIFT);
> +	status = regmap_write(chip->regmap, ISL29018_REG_TEST, 0x0);
>   	if (status<  0) {
> -		dev_err(&client->dev, "Failed to clear isl29018 TEST reg."
> +		dev_err(chip->dev, "Failed to clear isl29018 TEST reg."
>   					"(%d)\n", status);
>   		return status;
>   	}
> @@ -497,10 +467,9 @@ static int isl29018_chip_init(struct i2c_client *client)
>   	 * "Operating Mode" (COMMAND1) register is reprogrammed when
>   	 * data is read from the device.
>   	 */
> -	status = isl29018_write_data(client, ISL29018_REG_ADD_COMMAND1, 0,
> -				0xff, 0);
> +	status = regmap_write(chip->regmap, ISL29018_REG_ADD_COMMAND1, 0);
>   	if (status<  0) {
> -		dev_err(&client->dev, "Failed to clear isl29018 CMD1 reg."
> +		dev_err(chip->dev, "Failed to clear isl29018 CMD1 reg."
>   					"(%d)\n", status);
>   		return status;
>   	}
> @@ -508,13 +477,13 @@ static int isl29018_chip_init(struct i2c_client *client)
>   	msleep(1);	/* per data sheet, page 10 */
>
>   	/* set defaults */
> -	status = isl29018_set_range(client, chip->range,&new_range);
> +	status = isl29018_set_range(chip, chip->range,&new_range);
>   	if (status<  0) {
> -		dev_err(&client->dev, "Init of isl29018 fails\n");
> +		dev_err(chip->dev, "Init of isl29018 fails\n");
>   		return status;
>   	}
>
> -	status = isl29018_set_resolution(client, chip->adc_bit,
> +	status = isl29018_set_resolution(chip, chip->adc_bit,
>   						&new_adc_bit);
>
>   	return 0;
> @@ -527,6 +496,32 @@ static const struct iio_info isl29108_info = {
>   	.write_raw =&isl29018_write_raw,
>   };
>
> +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:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
> +/*
> + * isl29018_regmap_config: regmap configuration.
> + * Use RBTREE mechanism for caching.
> + */
> +static const struct regmap_config isl29018_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_reg = is_volatile_reg,
> +	.max_register = ISL29018_REG_TEST,
> +	.num_reg_defaults_raw = ISL29018_REG_TEST + 1,
> +	.cache_type = REGCACHE_RBTREE,
> +};
> +
>   static int __devinit isl29018_probe(struct i2c_client *client,
>   			 const struct i2c_device_id *id)
>   {
> @@ -543,7 +538,7 @@ static int __devinit isl29018_probe(struct i2c_client *client,
>   	chip = iio_priv(indio_dev);
>
>   	i2c_set_clientdata(client, indio_dev);
> -	chip->client = client;
> +	chip->dev =&client->dev;
>
>   	mutex_init(&chip->lock);
>
> @@ -551,7 +546,14 @@ static int __devinit isl29018_probe(struct i2c_client *client,
>   	chip->range = 1000;
>   	chip->adc_bit = 16;
>
> -	err = isl29018_chip_init(client);
> +	chip->regmap = devm_regmap_init_i2c(client,&isl29018_regmap_config);
> +	if (IS_ERR(chip->regmap)) {
> +		err = PTR_ERR(chip->regmap);
> +		dev_err(chip->dev, "regmap initialization failed: %d\n", err);
> +		goto exit;
> +	}
> +
> +	err = isl29018_chip_init(chip);
>   	if (err)
>   		goto exit_iio_free;
>

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

[Laxman Dewangan]

HI Jonathan,

Thanks for fast review.

On Tuesday 17 April 2012 05:03 PM, Jonathan Cameron wrote:
> On 4/17/2012 9:50 AM, Laxman Dewangan 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.
> I'd prefer the intial tab fixup for the kconfig file as a separate
> patch, but other than that
> all looks good.
>

Yes, this make sense. I will create fist patch for fixing this and then 
next patch for actual my change.

> This will probably cause issues alongside the series I sent to Greg the
> other day though
> so you may want to sit on it for a day or two and rebase.

I will wait for your change to be in linux-next and then I will create 
next patches.

>> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
> Acked-by: Jonathan Cameron<jic23@kernel.org>
>> ---
>>    drivers/staging/iio/light/Kconfig    |   19 ++--
>>    drivers/staging/iio/light/isl29018.c |  176 +++++++++++++++++-----------------
>>    2 files changed, 99 insertions(+), 96 deletions(-)
>>
>> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
>> index 53b49f7..fd39f72 100644
>> --- a/drivers/staging/iio/light/Kconfig
>> +++ b/drivers/staging/iio/light/Kconfig
>> @@ -4,15 +4,16 @@
>>    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
>> +     select REGMAP_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.
> Down to here is a valid but unconnected change.  Can you break this out
> to a separate patch?

Sure, I will do.

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

[Bryan Freed]

Hey Laxman, Jonathan.

The regmap is pretty new to me.  In looking at it, I see
_regmap_write() will regcache_write() before attempting to write to
the device.
This effectively updates the cached value even when the device write
fails, which is the opposite of the current isl29018 cache policy.

It looks like we tried to change the policy back in August and got
this response:
  > I did change one basic behavior that I think was also broken: cache
  > the value regardless of if the transaction completed successfully or
  > not.
  Don't do that.  That means userspace will get an invalid value if it read=
s
  in the meantime. If you have an error on a hardware bus - tell userspace =
about
  it and don't 'guess' what is in the register.
See http://driverdev.linuxdriverproject.org/pipermail/devel/2011-August/020=
256.html
for details.

The result is likely to be that after a failed write to one field of a
register, the next successful write to another field of that register
will update them both.

bryan.

On Tue, Apr 17, 2012 at 4:59 AM, Laxman Dewangan <ldewangan@nvidia.com> wro=
te:
>
> HI Jonathan,
>
> Thanks for fast review.
>
>
> On Tuesday 17 April 2012 05:03 PM, Jonathan Cameron wrote:
>>
>> On 4/17/2012 9:50 AM, Laxman Dewangan 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.
>>
>> I'd prefer the intial tab fixup for the kconfig file as a separate
>> patch, but other than that
>> all looks good.
>>
>
> Yes, this make sense. I will create fist patch for fixing this and then n=
ext
> patch for actual my change.
>
>
>> This will probably cause issues alongside the series I sent to Greg the
>> other day though
>> so you may want to sit on it for a day or two and rebase.
>
>
> I will wait for your change to be in linux-next and then I will create ne=
xt
> patches.
>
>
>>> Signed-off-by: Laxman Dewangan<ldewangan@nvidia.com>
>>
>> Acked-by: Jonathan Cameron<jic23@kernel.org>
>>>
>>> ---
>>> =A0 drivers/staging/iio/light/Kconfig =A0 =A0| =A0 19 ++--
>>> =A0 drivers/staging/iio/light/isl29018.c | =A0176
>>> +++++++++++++++++-----------------
>>> =A0 2 files changed, 99 insertions(+), 96 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/light/Kconfig
>>> b/drivers/staging/iio/light/Kconfig
>>> index 53b49f7..fd39f72 100644
>>> --- a/drivers/staging/iio/light/Kconfig
>>> +++ b/drivers/staging/iio/light/Kconfig
>>> @@ -4,15 +4,16 @@
>>> =A0 menu "Light sensors"
>>>
>>> =A0 config SENSORS_ISL29018
>>> - =A0 =A0 =A0 =A0tristate "ISL 29018 light and proximity sensor"
>>> - =A0 =A0 =A0 =A0depends on I2C
>>> - =A0 =A0 =A0 =A0default n
>>> - =A0 =A0 =A0 =A0help
>>> - =A0 =A0 =A0 =A0 If you say yes here you get support for ambient light=
 sensing
>>> and
>>> - =A0 =A0 =A0 =A0 proximity infrared sensing from Intersil ISL29018.
>>> - =A0 =A0 =A0 =A0 This driver will provide the measurements of ambient =
light
>>> intensity
>>> - =A0 =A0 =A0 =A0 in lux, proximity infrared sensing and normal infrare=
d sensing.
>>> - =A0 =A0 =A0 =A0 Data from sensor is accessible via sysfs.
>>> + =A0 =A0 tristate "ISL 29018 light and proximity sensor"
>>> + =A0 =A0 depends on I2C
>>> + =A0 =A0 select REGMAP_I2C
>>> + =A0 =A0 default n
>>> + =A0 =A0 help
>>> + =A0 =A0 =A0If you say yes here you get support for ambient light sens=
ing and
>>> + =A0 =A0 =A0proximity infrared sensing from Intersil ISL29018.
>>> + =A0 =A0 =A0This driver will provide the measurements of ambient light
>>> intensity
>>> + =A0 =A0 =A0in lux, proximity infrared sensing and normal infrared sen=
sing.
>>> + =A0 =A0 =A0Data from sensor is accessible via sysfs.
>>
>> Down to here is a valid but unconnected change. =A0Can you break this ou=
t
>> to a separate patch?
>
>
> Sure, I will do.

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

[Laxman Dewangan]

Including Mark here for more insight/comment.

On Tuesday 17 April 2012 11:26 PM, Bryan Freed wrote:
> Hey Laxman, Jonathan.
>
> The regmap is pretty new to me.  In looking at it, I see
> _regmap_write() will regcache_write() before attempting to write to
> the device.
> This effectively updates the cached value even when the device write
> fails, which is the opposite of the current isl29018 cache policy.
>
> It looks like we tried to change the policy back in August and got
> this response:
>    >  I did change one basic behavior that I think was also broken: cache
>    >  the value regardless of if the transaction completed successfully or
>    >  not.
>    Don't do that.  That means userspace will get an invalid value if it reads
>    in the meantime. If you have an error on a hardware bus - tell userspace about
>    it and don't 'guess' what is in the register.
> See http://driverdev.linuxdriverproject.org/pipermail/devel/2011-August/020256.html
> for details.
>
> The result is likely to be that after a failed write to one field of a
> register, the next successful write to another field of that register
> will update them both.

yes, the caching is done before actual transfer and it can make cache 
dirty in case of transfer failure.
I am not sure what should be correct handling here: make cache dirty or 
revert the value to original one?
But using the regmap will provide the common behavior across the driver 
and all driver will have same behavior rather than handling on 
differently across driver.
So if there is any other policy for this, it will be same for all driver.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1334 bytes --]

On Wed, Apr 18, 2012 at 11:49:41AM +0530, Laxman Dewangan wrote:

> yes, the caching is done before actual transfer and it can make
> cache dirty in case of transfer failure.
> I am not sure what should be correct handling here: make cache dirty
> or revert the value to original one?
> But using the regmap will provide the common behavior across the
> driver and all driver will have same behavior rather than handling
> on differently across driver.
> So if there is any other policy for this, it will be same for all driver.

I'd mark the cache as dirty I guess, though you'll then need to arrange
to resync it at some point (presumably after you've taken recovery
action) so that won't be enough by itself.  Or reset the device and
bring it back up manually if you think that might work.

regmap doesn't make much effort here since if physical I/O has started
failing you've generally lost the plot completely, 90% of the time it's
not going to be possible to reover the situation at all and if you can
then there's a couple of different strategies that might work and which
one is appropriate is going to depend on the device.  I'll do a patch to
flag the cache as dirty since I can't see how it'd make the situation
any worse but the driver will still need to do something to handle this
situation for that to take any effect.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]