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

[Jonathan Cameron <jic23@xxxxxxxxx>]

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@xxxxxxxxxx>
Acked-by: Jonathan Cameron <jic23@xxxxxxxxxx>
> ---
>   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;

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel