All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V4 0/2] staging: iio: light: use regmap in isl29018 for register access
@ 2012-04-20  7:27 Laxman Dewangan
  2012-04-20  7:27 ` [PATCH V4 1/2] staging: iio: light: convert multiple spaces to tab Laxman Dewangan
  2012-04-20  7:27 ` [PATCH V4 2/2] staging: iio: light: isl29018: use regmap for register access Laxman Dewangan
  0 siblings, 2 replies; 6+ messages in thread
From: Laxman Dewangan @ 2012-04-20  7:27 UTC (permalink / raw)
  To: jic23, gregkh, max, jbrenner, bfreed, lars, grundler
  Cc: linux-iio, devel, linux-kernel, Laxman Dewangan


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.

Changes from V3:
Taken care of Lars's and Grant's comment about COMMAND1 register access.
-Use regmap_write() for ADD_COMMAND1 and keeping this as volatile.


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 |  178 +++++++++++++++++-----------------
 2 files changed, 100 insertions(+), 97 deletions(-)


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH V4 1/2] staging: iio: light: convert multiple spaces to tab
  2012-04-20  7:27 [PATCH V4 0/2] staging: iio: light: use regmap in isl29018 for register access Laxman Dewangan
@ 2012-04-20  7:27 ` Laxman Dewangan
  2012-04-20 11:22   ` Lars-Peter Clausen
  2012-04-20  7:27 ` [PATCH V4 2/2] staging: iio: light: isl29018: use regmap for register access Laxman Dewangan
  1 sibling, 1 reply; 6+ messages in thread
From: Laxman Dewangan @ 2012-04-20  7:27 UTC (permalink / raw)
  To: jic23, gregkh, max, jbrenner, bfreed, lars, grundler
  Cc: linux-iio, devel, linux-kernel, Laxman Dewangan

Using tab inplace of multiple spaces for indenting.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
Changes from V1:
As per Jonathan's feedback, creating this patch for only
indenting cleanups.
No change from V2
No Change from V3

This is already acked by Jonathan.

 drivers/staging/iio/light/Kconfig |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
index 53b49f7..bb633f6 100644
--- a/drivers/staging/iio/light/Kconfig
+++ b/drivers/staging/iio/light/Kconfig
@@ -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"
-- 
1.7.1.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH V4 2/2] staging: iio: light: isl29018: use regmap for register access
  2012-04-20  7:27 [PATCH V4 0/2] staging: iio: light: use regmap in isl29018 for register access Laxman Dewangan
  2012-04-20  7:27 ` [PATCH V4 1/2] staging: iio: light: convert multiple spaces to tab Laxman Dewangan
@ 2012-04-20  7:27 ` Laxman Dewangan
  2012-04-20 15:04   ` Grant Grundler
  1 sibling, 1 reply; 6+ messages in thread
From: Laxman Dewangan @ 2012-04-20  7:27 UTC (permalink / raw)
  To: jic23, gregkh, max, jbrenner, bfreed, lars, grundler
  Cc: linux-iio, devel, linux-kernel, 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>
---
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 about wrong return.

Changes from V3:
Taken care of Lars's and Grant's comment about COMMAND1 register access.
-Use regmap_write() for ADD_COMMAND1 and keeping this as volatile.

 drivers/staging/iio/light/Kconfig    |    1 +
 drivers/staging/iio/light/isl29018.c |  178 +++++++++++++++++-----------------
 2 files changed, 91 insertions(+), 88 deletions(-)

diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
index bb633f6..fd39f72 100644
--- a/drivers/staging/iio/light/Kconfig
+++ b/drivers/staging/iio/light/Kconfig
@@ -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
diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index 073e899..de079ae 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_write(chip->regmap, ISL29018_REG_ADD_COMMAND1,
+			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");
-		return msb;
+	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 status;
 	}
-	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,7 +354,6 @@ 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) {
@@ -387,13 +361,13 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_PROCESSED:
 		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:
@@ -459,15 +433,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
@@ -488,10 +459,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;
 	}
@@ -500,10 +470,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;
 	}
@@ -511,13 +480,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;
@@ -530,6 +499,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)
 {
@@ -546,7 +541,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);
 
@@ -554,7 +549,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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH V4 1/2] staging: iio: light: convert multiple spaces to tab
  2012-04-20  7:27 ` [PATCH V4 1/2] staging: iio: light: convert multiple spaces to tab Laxman Dewangan
@ 2012-04-20 11:22   ` Lars-Peter Clausen
  2012-04-20 15:52     ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Lars-Peter Clausen @ 2012-04-20 11:22 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: jic23, gregkh, max, jbrenner, bfreed, grundler, linux-iio, devel,
	linux-kernel

On 04/20/2012 09:27 AM, Laxman Dewangan wrote:
> Using tab inplace of multiple spaces for indenting.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> ---
> Changes from V1:
> As per Jonathan's feedback, creating this patch for only
> indenting cleanups.
> No change from V2
> No Change from V3
> 
> This is already acked by Jonathan.

When resending a patch that has already been acked or reviewed, include the
Acked-by or Reviewed-by line in the patch under your Signed-off-by.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH V4 2/2] staging: iio: light: isl29018: use regmap for register access
  2012-04-20  7:27 ` [PATCH V4 2/2] staging: iio: light: isl29018: use regmap for register access Laxman Dewangan
@ 2012-04-20 15:04   ` Grant Grundler
  0 siblings, 0 replies; 6+ messages in thread
From: Grant Grundler @ 2012-04-20 15:04 UTC (permalink / raw)
  To: Laxman Dewangan
  Cc: jic23, gregkh, max, jbrenner, bfreed, lars, linux-iio, devel,
	linux-kernel

On Fri, Apr 20, 2012 at 12:27 AM, Laxman Dewangan <ldewangan@nvidia.com> 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@nvidia.com>

Reviewed-by: Grant Grundler <grundler@chromium.org>

Thanks for explaining the COMMAND1 register usage and volatile bits. LGTM.

cheers,
grant

> ---
> 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 about wrong return.
>
> Changes from V3:
> Taken care of Lars's and Grant's comment about COMMAND1 register access.
> -Use regmap_write() for ADD_COMMAND1 and keeping this as volatile.
>
>  drivers/staging/iio/light/Kconfig    |    1 +
>  drivers/staging/iio/light/isl29018.c |  178 +++++++++++++++++-----------------
>  2 files changed, 91 insertions(+), 88 deletions(-)
>
> diff --git a/drivers/staging/iio/light/Kconfig b/drivers/staging/iio/light/Kconfig
> index bb633f6..fd39f72 100644
> --- a/drivers/staging/iio/light/Kconfig
> +++ b/drivers/staging/iio/light/Kconfig
> @@ -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
> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
> index 073e899..de079ae 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_write(chip->regmap, ISL29018_REG_ADD_COMMAND1,
> +                       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");
> -               return msb;
> +       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 status;
>        }
> -       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,7 +354,6 @@ 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) {
> @@ -387,13 +361,13 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
>        case IIO_CHAN_INFO_PROCESSED:
>                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:
> @@ -459,15 +433,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
> @@ -488,10 +459,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;
>        }
> @@ -500,10 +470,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;
>        }
> @@ -511,13 +480,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;
> @@ -530,6 +499,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)
>  {
> @@ -546,7 +541,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);
>
> @@ -554,7 +549,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
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH V4 1/2] staging: iio: light: convert multiple spaces to tab
  2012-04-20 11:22   ` Lars-Peter Clausen
@ 2012-04-20 15:52     ` Greg KH
  0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2012-04-20 15:52 UTC (permalink / raw)
  To: Lars-Peter Clausen
  Cc: Laxman Dewangan, devel, max, jbrenner, grundler, linux-kernel,
	bfreed, linux-iio, jic23

On Fri, Apr 20, 2012 at 01:22:00PM +0200, Lars-Peter Clausen wrote:
> On 04/20/2012 09:27 AM, Laxman Dewangan wrote:
> > Using tab inplace of multiple spaces for indenting.
> > 
> > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
> > ---
> > Changes from V1:
> > As per Jonathan's feedback, creating this patch for only
> > indenting cleanups.
> > No change from V2
> > No Change from V3
> > 
> > This is already acked by Jonathan.
> 
> When resending a patch that has already been acked or reviewed, include the
> Acked-by or Reviewed-by line in the patch under your Signed-off-by.

I've now applied this patch, so it no longer needs to be resent.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2012-04-20 15:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-20  7:27 [PATCH V4 0/2] staging: iio: light: use regmap in isl29018 for register access Laxman Dewangan
2012-04-20  7:27 ` [PATCH V4 1/2] staging: iio: light: convert multiple spaces to tab Laxman Dewangan
2012-04-20 11:22   ` Lars-Peter Clausen
2012-04-20 15:52     ` Greg KH
2012-04-20  7:27 ` [PATCH V4 2/2] staging: iio: light: isl29018: use regmap for register access Laxman Dewangan
2012-04-20 15:04   ` Grant Grundler

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.