[PATCH 0/9] staging: iio: tsl2583: i2c cleanups

[PATCH 0/9] staging: iio: tsl2583: i2c cleanups

[Brian Masney]

This is my next round of staging cleanups to the tsl2581 light driver
in IIO staging. It mostly contains cleanups to the i2c code. I verified
that the driver still functions correctly using a TSL2581 hooked up to
a Raspberry Pi 2.

This patch set lays the foundation for moving to the regmap API.

Brian Masney (9):
  staging: iio: tsl2583: i2c_smbus_write_byte() / i2c_smbus_read_byte()
    migration
  staging: iio: tsl2583: removed unused code from device probing
  staging: iio: tsl2583: fixed ordering of comments
  staging: iio: tsl2583: remove redundant power off sequence in
    taos_chip_on()
  staging: iio: tsl2583: don't shutdown chip when updating the lux table
  staging: iio: tsl2583: remove redudant i2c call in
    taos_als_calibrate()
  staging: iio: tsl2583: fix issue with changes to calibscale and
    int_time not being set on the chip
  staging: iio: tsl2583: check if chip is suspended in
    in_illuminance_calibrate_store
  staging: iio: tsl2583: remove redundant write to the control register
    in taos_probe()

 drivers/staging/iio/light/tsl2583.c | 352 +++++++++++++-----------------------
 1 file changed, 126 insertions(+), 226 deletions(-)

-- 
2.5.5

[PATCH 1/9] staging: iio: tsl2583: i2c_smbus_write_byte() / i2c_smbus_read_byte() migration

[Brian Masney]

There were several places where the driver would first call
i2c_smbus_write_byte() to select the register on the device, and then
call i2c_smbus_read_byte() to get the contents of that register. The
code would look roughly like:

/* Select register */
i2c_smbus_write_byte(client, REGISTER);

/* Read the the last register that was written to */
int data = i2c_smbus_read_byte(client);

Rewrite this to use i2c_smbus_read_byte_data() to combine the two
calls into one:

int data = i2c_smbus_read_byte_data(chip->client, REGISTER);

Verified that the driver still functions correctly using a TSL2581
hooked up to a Raspberry Pi 2.

This fixes the following warnings that were found by the
kbuild test robot that were introduced by commit 8ba355cce3c6 ("staging:
iio: tsl2583: check for error code from i2c_smbus_read_byte()").

drivers/staging/iio/light/tsl2583.c:365:5-12: WARNING: Unsigned
expression compared with zero: reg_val < 0

drivers/staging/iio/light/tsl2583.c:388:5-12: WARNING: Unsigned
expression compared with zero: reg_val < 0

This also removes the need for the taos_i2c_read() function since all
callers were only calling the function with a length of 1.

Signed-off-by: Brian Masney <masneyb@onstation.org>
Cc: Julia Lawall <julia.lawall@lip6.fr>
---
 drivers/staging/iio/light/tsl2583.c | 85 +++++++------------------------------
 1 file changed, 16 insertions(+), 69 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 49b19f5..a3a9095 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -155,39 +155,6 @@ static void taos_defaults(struct tsl2583_chip *chip)
 }
 
 /*
- * Read a number of bytes starting at register (reg) location.
- * Return 0, or i2c_smbus_write_byte ERROR code.
- */
-static int
-taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
-{
-	int i, ret;
-
-	for (i = 0; i < len; i++) {
-		/* select register to write */
-		ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | reg));
-		if (ret < 0) {
-			dev_err(&client->dev,
-				"taos_i2c_read failed to write register %x\n",
-				reg);
-			return ret;
-		}
-		/* read the data */
-		ret = i2c_smbus_read_byte(client);
-		if (ret < 0) {
-			dev_err(&client->dev,
-				"%s failed to read byte after writing to register %x\n",
-				__func__, reg);
-			return ret;
-		}
-		*val = ret;
-		val++;
-		reg++;
-	}
-	return 0;
-}
-
-/*
  * Reads and calculates current lux value.
  * The raw ch0 and ch1 values of the ambient light sensed in the last
  * integration cycle are read from the device.
@@ -220,13 +187,14 @@ static int taos_get_lux(struct iio_dev *indio_dev)
 		goto done;
 	}
 
-	ret = taos_i2c_read(chip->client, (TSL258X_CMD_REG), &buf[0], 1);
+	ret = i2c_smbus_read_byte_data(chip->client, TSL258X_CMD_REG);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "taos_get_lux failed to read CMD_REG\n");
 		goto done;
 	}
+
 	/* is data new & valid */
-	if (!(buf[0] & TSL258X_STA_ADC_INTR)) {
+	if (!(ret & TSL258X_STA_ADC_INTR)) {
 		dev_err(&chip->client->dev, "taos_get_lux data not valid\n");
 		ret = chip->als_cur_info.lux; /* return LAST VALUE */
 		goto done;
@@ -235,13 +203,14 @@ static int taos_get_lux(struct iio_dev *indio_dev)
 	for (i = 0; i < 4; i++) {
 		int reg = TSL258X_CMD_REG | (TSL258X_ALS_CHAN0LO + i);
 
-		ret = taos_i2c_read(chip->client, reg, &buf[i], 1);
+		ret = i2c_smbus_read_byte_data(chip->client, reg);
 		if (ret < 0) {
 			dev_err(&chip->client->dev,
 				"taos_get_lux failed to read register %x\n",
 				reg);
 			goto done;
 		}
+		buf[i] = ret;
 	}
 
 	/*
@@ -343,52 +312,36 @@ static int taos_get_lux(struct iio_dev *indio_dev)
 static int taos_als_calibrate(struct iio_dev *indio_dev)
 {
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
-	u8 reg_val;
 	unsigned int gain_trim_val;
 	int ret;
 	int lux_val;
 
-	ret = i2c_smbus_write_byte(chip->client,
-				   (TSL258X_CMD_REG | TSL258X_CNTRL));
+	ret = i2c_smbus_read_byte_data(chip->client,
+				       TSL258X_CMD_REG | TSL258X_CNTRL);
 	if (ret < 0) {
 		dev_err(&chip->client->dev,
-			"taos_als_calibrate failed to reach the CNTRL register, ret=%d\n",
-			ret);
-		return ret;
-	}
-
-	reg_val = i2c_smbus_read_byte(chip->client);
-	if (reg_val < 0) {
-		dev_err(&chip->client->dev,
-			"%s failed to read after writing to the CNTRL register\n",
+			"%s failed to read from the CNTRL register\n",
 			__func__);
 		return ret;
 	}
 
-	if ((reg_val & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON))
+	if ((ret & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON))
 			!= (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON)) {
 		dev_err(&chip->client->dev,
 			"taos_als_calibrate failed: device not powered on with ADC enabled\n");
 		return -EINVAL;
 	}
 
-	ret = i2c_smbus_write_byte(chip->client,
-				   (TSL258X_CMD_REG | TSL258X_CNTRL));
+	ret = i2c_smbus_read_byte_data(chip->client,
+				       TSL258X_CMD_REG | TSL258X_CNTRL);
 	if (ret < 0) {
 		dev_err(&chip->client->dev,
-			"taos_als_calibrate failed to reach the STATUS register, ret=%d\n",
-			ret);
-		return ret;
-	}
-	reg_val = i2c_smbus_read_byte(chip->client);
-	if (reg_val < 0) {
-		dev_err(&chip->client->dev,
-			"%s failed to read after writing to the STATUS register\n",
+			"%s failed to read from the CNTRL register\n",
 			__func__);
 		return ret;
 	}
 
-	if ((reg_val & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
+	if ((ret & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
 		dev_err(&chip->client->dev,
 			"taos_als_calibrate failed: STATUS - ADC not valid.\n");
 		return -ENODATA;
@@ -847,15 +800,9 @@ static int taos_probe(struct i2c_client *clientp,
 	memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
 
 	for (i = 0; i < TSL258X_MAX_DEVICE_REGS; i++) {
-		ret = i2c_smbus_write_byte(clientp,
-				(TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
-		if (ret < 0) {
-			dev_err(&clientp->dev,
-				"i2c_smbus_write_byte to cmd reg failed in taos_probe(), err = %d\n",
-				ret);
-			return ret;
-		}
-		ret = i2c_smbus_read_byte(clientp);
+		ret = i2c_smbus_read_byte_data(clientp,
+					       (TSL258X_CMD_REG |
+						(TSL258X_CNTRL + i)));
 		if (ret < 0) {
 			dev_err(&clientp->dev,
 				"i2c_smbus_read_byte from reg failed in taos_probe(), err = %d\n",
-- 
2.5.5

Re: [PATCH 1/9] staging: iio: tsl2583: i2c_smbus_write_byte() / i2c_smbus_read_byte() migration

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> There were several places where the driver would first call
> i2c_smbus_write_byte() to select the register on the device, and then
> call i2c_smbus_read_byte() to get the contents of that register. The
> code would look roughly like:
> 
> /* Select register */
> i2c_smbus_write_byte(client, REGISTER);
> 
> /* Read the the last register that was written to */
> int data = i2c_smbus_read_byte(client);
> 
> Rewrite this to use i2c_smbus_read_byte_data() to combine the two
> calls into one:
> 
> int data = i2c_smbus_read_byte_data(chip->client, REGISTER);
> 
> Verified that the driver still functions correctly using a TSL2581
> hooked up to a Raspberry Pi 2.
> 
> This fixes the following warnings that were found by the
> kbuild test robot that were introduced by commit 8ba355cce3c6 ("staging:
> iio: tsl2583: check for error code from i2c_smbus_read_byte()").
> 
> drivers/staging/iio/light/tsl2583.c:365:5-12: WARNING: Unsigned
> expression compared with zero: reg_val < 0
> 
> drivers/staging/iio/light/tsl2583.c:388:5-12: WARNING: Unsigned
> expression compared with zero: reg_val < 0
> 
> This also removes the need for the taos_i2c_read() function since all
> callers were only calling the function with a length of 1.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
One comment in line.  Good to get rid of the warning that the
autobuilders have been pinging at me ever time I do a push!

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 85 +++++++------------------------------
>  1 file changed, 16 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 49b19f5..a3a9095 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -155,39 +155,6 @@ static void taos_defaults(struct tsl2583_chip *chip)
>  }
>  
>  /*
> - * Read a number of bytes starting at register (reg) location.
> - * Return 0, or i2c_smbus_write_byte ERROR code.
> - */
> -static int
> -taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
> -{
> -	int i, ret;
> -
> -	for (i = 0; i < len; i++) {
> -		/* select register to write */
> -		ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | reg));
> -		if (ret < 0) {
> -			dev_err(&client->dev,
> -				"taos_i2c_read failed to write register %x\n",
> -				reg);
> -			return ret;
> -		}
> -		/* read the data */
> -		ret = i2c_smbus_read_byte(client);
> -		if (ret < 0) {
> -			dev_err(&client->dev,
> -				"%s failed to read byte after writing to register %x\n",
> -				__func__, reg);
> -			return ret;
> -		}
> -		*val = ret;
> -		val++;
> -		reg++;
> -	}
> -	return 0;
> -}
> -
> -/*
>   * Reads and calculates current lux value.
>   * The raw ch0 and ch1 values of the ambient light sensed in the last
>   * integration cycle are read from the device.
> @@ -220,13 +187,14 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  		goto done;
>  	}
>  
> -	ret = taos_i2c_read(chip->client, (TSL258X_CMD_REG), &buf[0], 1);
> +	ret = i2c_smbus_read_byte_data(chip->client, TSL258X_CMD_REG);
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev, "taos_get_lux failed to read CMD_REG\n");
>  		goto done;
>  	}
> +
>  	/* is data new & valid */
> -	if (!(buf[0] & TSL258X_STA_ADC_INTR)) {
> +	if (!(ret & TSL258X_STA_ADC_INTR)) {
>  		dev_err(&chip->client->dev, "taos_get_lux data not valid\n");
>  		ret = chip->als_cur_info.lux; /* return LAST VALUE */
>  		goto done;
> @@ -235,13 +203,14 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  	for (i = 0; i < 4; i++) {
>  		int reg = TSL258X_CMD_REG | (TSL258X_ALS_CHAN0LO + i);
>  
> -		ret = taos_i2c_read(chip->client, reg, &buf[i], 1);
> +		ret = i2c_smbus_read_byte_data(chip->client, reg);
>  		if (ret < 0) {
>  			dev_err(&chip->client->dev,
>  				"taos_get_lux failed to read register %x\n",
>  				reg);
>  			goto done;
>  		}
> +		buf[i] = ret;
>  	}
>  
>  	/*
> @@ -343,52 +312,36 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  static int taos_als_calibrate(struct iio_dev *indio_dev)
>  {
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> -	u8 reg_val;
>  	unsigned int gain_trim_val;
>  	int ret;
>  	int lux_val;
>  
> -	ret = i2c_smbus_write_byte(chip->client,
> -				   (TSL258X_CMD_REG | TSL258X_CNTRL));
> +	ret = i2c_smbus_read_byte_data(chip->client,
> +				       TSL258X_CMD_REG | TSL258X_CNTRL);
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev,
> -			"taos_als_calibrate failed to reach the CNTRL register, ret=%d\n",
> -			ret);
> -		return ret;
> -	}
> -
> -	reg_val = i2c_smbus_read_byte(chip->client);
> -	if (reg_val < 0) {
> -		dev_err(&chip->client->dev,
> -			"%s failed to read after writing to the CNTRL register\n",
> +			"%s failed to read from the CNTRL register\n",
>  			__func__);
>  		return ret;
>  	}
>  
> -	if ((reg_val & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON))
> +	if ((ret & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON))
>  			!= (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON)) {
>  		dev_err(&chip->client->dev,
>  			"taos_als_calibrate failed: device not powered on with ADC enabled\n");
>  		return -EINVAL;
>  	}
>  
> -	ret = i2c_smbus_write_byte(chip->client,
> -				   (TSL258X_CMD_REG | TSL258X_CNTRL));
> +	ret = i2c_smbus_read_byte_data(chip->client,
> +				       TSL258X_CMD_REG | TSL258X_CNTRL);
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev,
> -			"taos_als_calibrate failed to reach the STATUS register, ret=%d\n",
> -			ret);
> -		return ret;
> -	}
> -	reg_val = i2c_smbus_read_byte(chip->client);
> -	if (reg_val < 0) {
> -		dev_err(&chip->client->dev,
> -			"%s failed to read after writing to the STATUS register\n",
> +			"%s failed to read from the CNTRL register\n",
>  			__func__);
>  		return ret;
>  	}
>  
> -	if ((reg_val & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
> +	if ((ret & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
>  		dev_err(&chip->client->dev,
>  			"taos_als_calibrate failed: STATUS - ADC not valid.\n");
>  		return -ENODATA;
> @@ -847,15 +800,9 @@ static int taos_probe(struct i2c_client *clientp,
>  	memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
>  
>  	for (i = 0; i < TSL258X_MAX_DEVICE_REGS; i++) {
> -		ret = i2c_smbus_write_byte(clientp,
> -				(TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
> -		if (ret < 0) {
> -			dev_err(&clientp->dev,
> -				"i2c_smbus_write_byte to cmd reg failed in taos_probe(), err = %d\n",
> -				ret);
> -			return ret;
> -		}
> -		ret = i2c_smbus_read_byte(clientp);
> +		ret = i2c_smbus_read_byte_data(clientp,
> +					       (TSL258X_CMD_REG |
> +						(TSL258X_CNTRL + i)));
Overkill on the brackets here. Clearly that was from the original code
though so maybe a follow up cleanup to get rid of the extra set around the
whole statement?
>  		if (ret < 0) {
>  			dev_err(&clientp->dev,
>  				"i2c_smbus_read_byte from reg failed in taos_probe(), err = %d\n",
> 

Re: [PATCH 1/9] staging: iio: tsl2583: i2c_smbus_write_byte() / i2c_smbus_read_byte() migration

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> There were several places where the driver would first call
> i2c_smbus_write_byte() to select the register on the device, and then
> call i2c_smbus_read_byte() to get the contents of that register. The
> code would look roughly like:
> 
> /* Select register */
> i2c_smbus_write_byte(client, REGISTER);
> 
> /* Read the the last register that was written to */
> int data = i2c_smbus_read_byte(client);
> 
> Rewrite this to use i2c_smbus_read_byte_data() to combine the two
> calls into one:
> 
> int data = i2c_smbus_read_byte_data(chip->client, REGISTER);
> 
> Verified that the driver still functions correctly using a TSL2581
> hooked up to a Raspberry Pi 2.
> 
> This fixes the following warnings that were found by the
> kbuild test robot that were introduced by commit 8ba355cce3c6 ("staging:
> iio: tsl2583: check for error code from i2c_smbus_read_byte()").
> 
> drivers/staging/iio/light/tsl2583.c:365:5-12: WARNING: Unsigned
> expression compared with zero: reg_val < 0
> 
> drivers/staging/iio/light/tsl2583.c:388:5-12: WARNING: Unsigned
> expression compared with zero: reg_val < 0
> 
> This also removes the need for the taos_i2c_read() function since all
> callers were only calling the function with a length of 1.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> Cc: Julia Lawall <julia.lawall@lip6.fr>
Forgot to say - applied to the togreg branch of iio.git and pushed
out as testing to see what we've missed!

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 85 +++++++------------------------------
>  1 file changed, 16 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 49b19f5..a3a9095 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -155,39 +155,6 @@ static void taos_defaults(struct tsl2583_chip *chip)
>  }
>  
>  /*
> - * Read a number of bytes starting at register (reg) location.
> - * Return 0, or i2c_smbus_write_byte ERROR code.
> - */
> -static int
> -taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
> -{
> -	int i, ret;
> -
> -	for (i = 0; i < len; i++) {
> -		/* select register to write */
> -		ret = i2c_smbus_write_byte(client, (TSL258X_CMD_REG | reg));
> -		if (ret < 0) {
> -			dev_err(&client->dev,
> -				"taos_i2c_read failed to write register %x\n",
> -				reg);
> -			return ret;
> -		}
> -		/* read the data */
> -		ret = i2c_smbus_read_byte(client);
> -		if (ret < 0) {
> -			dev_err(&client->dev,
> -				"%s failed to read byte after writing to register %x\n",
> -				__func__, reg);
> -			return ret;
> -		}
> -		*val = ret;
> -		val++;
> -		reg++;
> -	}
> -	return 0;
> -}
> -
> -/*
>   * Reads and calculates current lux value.
>   * The raw ch0 and ch1 values of the ambient light sensed in the last
>   * integration cycle are read from the device.
> @@ -220,13 +187,14 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  		goto done;
>  	}
>  
> -	ret = taos_i2c_read(chip->client, (TSL258X_CMD_REG), &buf[0], 1);
> +	ret = i2c_smbus_read_byte_data(chip->client, TSL258X_CMD_REG);
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev, "taos_get_lux failed to read CMD_REG\n");
>  		goto done;
>  	}
> +
>  	/* is data new & valid */
> -	if (!(buf[0] & TSL258X_STA_ADC_INTR)) {
> +	if (!(ret & TSL258X_STA_ADC_INTR)) {
>  		dev_err(&chip->client->dev, "taos_get_lux data not valid\n");
>  		ret = chip->als_cur_info.lux; /* return LAST VALUE */
>  		goto done;
> @@ -235,13 +203,14 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  	for (i = 0; i < 4; i++) {
>  		int reg = TSL258X_CMD_REG | (TSL258X_ALS_CHAN0LO + i);
>  
> -		ret = taos_i2c_read(chip->client, reg, &buf[i], 1);
> +		ret = i2c_smbus_read_byte_data(chip->client, reg);
>  		if (ret < 0) {
>  			dev_err(&chip->client->dev,
>  				"taos_get_lux failed to read register %x\n",
>  				reg);
>  			goto done;
>  		}
> +		buf[i] = ret;
>  	}
>  
>  	/*
> @@ -343,52 +312,36 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  static int taos_als_calibrate(struct iio_dev *indio_dev)
>  {
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> -	u8 reg_val;
>  	unsigned int gain_trim_val;
>  	int ret;
>  	int lux_val;
>  
> -	ret = i2c_smbus_write_byte(chip->client,
> -				   (TSL258X_CMD_REG | TSL258X_CNTRL));
> +	ret = i2c_smbus_read_byte_data(chip->client,
> +				       TSL258X_CMD_REG | TSL258X_CNTRL);
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev,
> -			"taos_als_calibrate failed to reach the CNTRL register, ret=%d\n",
> -			ret);
> -		return ret;
> -	}
> -
> -	reg_val = i2c_smbus_read_byte(chip->client);
> -	if (reg_val < 0) {
> -		dev_err(&chip->client->dev,
> -			"%s failed to read after writing to the CNTRL register\n",
> +			"%s failed to read from the CNTRL register\n",
>  			__func__);
>  		return ret;
>  	}
>  
> -	if ((reg_val & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON))
> +	if ((ret & (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON))
>  			!= (TSL258X_CNTL_ADC_ENBL | TSL258X_CNTL_PWR_ON)) {
>  		dev_err(&chip->client->dev,
>  			"taos_als_calibrate failed: device not powered on with ADC enabled\n");
>  		return -EINVAL;
>  	}
>  
> -	ret = i2c_smbus_write_byte(chip->client,
> -				   (TSL258X_CMD_REG | TSL258X_CNTRL));
> +	ret = i2c_smbus_read_byte_data(chip->client,
> +				       TSL258X_CMD_REG | TSL258X_CNTRL);
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev,
> -			"taos_als_calibrate failed to reach the STATUS register, ret=%d\n",
> -			ret);
> -		return ret;
> -	}
> -	reg_val = i2c_smbus_read_byte(chip->client);
> -	if (reg_val < 0) {
> -		dev_err(&chip->client->dev,
> -			"%s failed to read after writing to the STATUS register\n",
> +			"%s failed to read from the CNTRL register\n",
>  			__func__);
>  		return ret;
>  	}
>  
> -	if ((reg_val & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
> +	if ((ret & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
>  		dev_err(&chip->client->dev,
>  			"taos_als_calibrate failed: STATUS - ADC not valid.\n");
>  		return -ENODATA;
> @@ -847,15 +800,9 @@ static int taos_probe(struct i2c_client *clientp,
>  	memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
>  
>  	for (i = 0; i < TSL258X_MAX_DEVICE_REGS; i++) {
> -		ret = i2c_smbus_write_byte(clientp,
> -				(TSL258X_CMD_REG | (TSL258X_CNTRL + i)));
> -		if (ret < 0) {
> -			dev_err(&clientp->dev,
> -				"i2c_smbus_write_byte to cmd reg failed in taos_probe(), err = %d\n",
> -				ret);
> -			return ret;
> -		}
> -		ret = i2c_smbus_read_byte(clientp);
> +		ret = i2c_smbus_read_byte_data(clientp,
> +					       (TSL258X_CMD_REG |
> +						(TSL258X_CNTRL + i)));
>  		if (ret < 0) {
>  			dev_err(&clientp->dev,
>  				"i2c_smbus_read_byte from reg failed in taos_probe(), err = %d\n",
> 

[PATCH 2/9] staging: iio: tsl2583: removed unused code from device probing

[Brian Masney]

taos_probe() queries the all of the sensor's registers and loads all of
the values into a buffer stored on the stack. Only the chip ID register
was actually used. Change the probe function to just query the chip ID
register on the device.

Verified that the driver still functions correctly using a TSL2581
hooked up to a Raspberry Pi 2.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/tsl2583.c | 37 +++++++++++++------------------------
 1 file changed, 13 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index a3a9095..388440b 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -31,8 +31,6 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-#define TSL258X_MAX_DEVICE_REGS		32
-
 /* Triton register offsets */
 #define	TSL258X_REG_MAX		8
 
@@ -66,6 +64,9 @@
 /* Lux calculation constants */
 #define	TSL258X_LUX_CALC_OVER_FLOW		65535
 
+#define TSL2583_CHIP_ID			0x90
+#define TSL2583_CHIP_ID_MASK		0xf0
+
 enum {
 	TSL258X_CHIP_UNKNOWN = 0,
 	TSL258X_CHIP_WORKING = 1,
@@ -607,12 +608,6 @@ static const struct attribute_group tsl2583_attribute_group = {
 	.attrs = sysfs_attrs_ctrl,
 };
 
-/* Use the default register values to identify the Taos device */
-static int taos_tsl258x_device(unsigned char *bufp)
-{
-	return ((bufp[TSL258X_CHIPID] & 0xf0) == 0x90);
-}
-
 static const struct iio_chan_spec tsl2583_channels[] = {
 	{
 		.type = IIO_LIGHT,
@@ -777,8 +772,7 @@ static const struct iio_info tsl2583_info = {
 static int taos_probe(struct i2c_client *clientp,
 		      const struct i2c_device_id *idp)
 {
-	int i, ret;
-	unsigned char buf[TSL258X_MAX_DEVICE_REGS];
+	int ret;
 	struct tsl2583_chip *chip;
 	struct iio_dev *indio_dev;
 
@@ -799,22 +793,17 @@ static int taos_probe(struct i2c_client *clientp,
 	chip->taos_chip_status = TSL258X_CHIP_UNKNOWN;
 	memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
 
-	for (i = 0; i < TSL258X_MAX_DEVICE_REGS; i++) {
-		ret = i2c_smbus_read_byte_data(clientp,
-					       (TSL258X_CMD_REG |
-						(TSL258X_CNTRL + i)));
-		if (ret < 0) {
-			dev_err(&clientp->dev,
-				"i2c_smbus_read_byte from reg failed in taos_probe(), err = %d\n",
-				ret);
-			return ret;
-		}
-		buf[i] = ret;
+	ret = i2c_smbus_read_byte_data(clientp,
+				       TSL258X_CMD_REG | TSL258X_CHIPID);
+	if (ret < 0) {
+		dev_err(&clientp->dev,
+			"%s failed to read the chip ID register\n", __func__);
+		return ret;
 	}
 
-	if (!taos_tsl258x_device(buf)) {
-		dev_info(&clientp->dev,
-			 "i2c device found but does not match expected id in taos_probe()\n");
+	if ((ret & TSL2583_CHIP_ID_MASK) != TSL2583_CHIP_ID) {
+		dev_info(&clientp->dev, "%s received an unknown chip ID %x\n",
+			 __func__, ret);
 		return -EINVAL;
 	}
 
-- 
2.5.5

Re: [PATCH 2/9] staging: iio: tsl2583: removed unused code from device probing

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> taos_probe() queries the all of the sensor's registers and loads all of
> the values into a buffer stored on the stack. Only the chip ID register
> was actually used. Change the probe function to just query the chip ID
> register on the device.
> 
> Verified that the driver still functions correctly using a TSL2581
> hooked up to a Raspberry Pi 2.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Huh. I'm embarrassed that I didn't notice this bit of silliness in the
original reviews :)

Applied to the togreg branch of iio.git

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 37 +++++++++++++------------------------
>  1 file changed, 13 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index a3a9095..388440b 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -31,8 +31,6 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> -#define TSL258X_MAX_DEVICE_REGS		32
> -
>  /* Triton register offsets */
>  #define	TSL258X_REG_MAX		8
>  
> @@ -66,6 +64,9 @@
>  /* Lux calculation constants */
>  #define	TSL258X_LUX_CALC_OVER_FLOW		65535
>  
> +#define TSL2583_CHIP_ID			0x90
> +#define TSL2583_CHIP_ID_MASK		0xf0
> +
>  enum {
>  	TSL258X_CHIP_UNKNOWN = 0,
>  	TSL258X_CHIP_WORKING = 1,
> @@ -607,12 +608,6 @@ static const struct attribute_group tsl2583_attribute_group = {
>  	.attrs = sysfs_attrs_ctrl,
>  };
>  
> -/* Use the default register values to identify the Taos device */
> -static int taos_tsl258x_device(unsigned char *bufp)
> -{
> -	return ((bufp[TSL258X_CHIPID] & 0xf0) == 0x90);
> -}
> -
>  static const struct iio_chan_spec tsl2583_channels[] = {
>  	{
>  		.type = IIO_LIGHT,
> @@ -777,8 +772,7 @@ static const struct iio_info tsl2583_info = {
>  static int taos_probe(struct i2c_client *clientp,
>  		      const struct i2c_device_id *idp)
>  {
> -	int i, ret;
> -	unsigned char buf[TSL258X_MAX_DEVICE_REGS];
> +	int ret;
>  	struct tsl2583_chip *chip;
>  	struct iio_dev *indio_dev;
>  
> @@ -799,22 +793,17 @@ static int taos_probe(struct i2c_client *clientp,
>  	chip->taos_chip_status = TSL258X_CHIP_UNKNOWN;
>  	memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
>  
> -	for (i = 0; i < TSL258X_MAX_DEVICE_REGS; i++) {
> -		ret = i2c_smbus_read_byte_data(clientp,
> -					       (TSL258X_CMD_REG |
> -						(TSL258X_CNTRL + i)));
> -		if (ret < 0) {
> -			dev_err(&clientp->dev,
> -				"i2c_smbus_read_byte from reg failed in taos_probe(), err = %d\n",
> -				ret);
> -			return ret;
> -		}
> -		buf[i] = ret;
> +	ret = i2c_smbus_read_byte_data(clientp,
> +				       TSL258X_CMD_REG | TSL258X_CHIPID);
> +	if (ret < 0) {
> +		dev_err(&clientp->dev,
> +			"%s failed to read the chip ID register\n", __func__);
> +		return ret;
>  	}
>  
> -	if (!taos_tsl258x_device(buf)) {
> -		dev_info(&clientp->dev,
> -			 "i2c device found but does not match expected id in taos_probe()\n");
> +	if ((ret & TSL2583_CHIP_ID_MASK) != TSL2583_CHIP_ID) {
> +		dev_info(&clientp->dev, "%s received an unknown chip ID %x\n",
> +			 __func__, ret);
>  		return -EINVAL;
>  	}
>  
> 

[PATCH 3/9] staging: iio: tsl2583: fixed ordering of comments

[Brian Masney]

in taos_defaults()

The comments in taos_defaults() appear after the line of code
that they apply to. This patch moves the comments so that they appear
before the code. Some of the comments were updated to be more
informative.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/tsl2583.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 388440b..709f446 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -143,16 +143,23 @@ static const struct gainadj gainadj[] = {
  */
 static void taos_defaults(struct tsl2583_chip *chip)
 {
-	/* Operational parameters */
+	/*
+	 * The integration time must be a multiple of 50ms and within the
+	 * range [50, 600] ms.
+	 */
 	chip->taos_settings.als_time = 100;
-	/* must be a multiple of 50mS */
+
+	/*
+	 * This is an index into the gainadj table. Assume clear glass as the
+	 * default.
+	 */
 	chip->taos_settings.als_gain = 0;
-	/* this is actually an index into the gain table */
-	/* assume clear glass as default */
+
+	/* Default gain trim to account for aperture effects */
 	chip->taos_settings.als_gain_trim = 1000;
-	/* default gain trim to account for aperture effects */
-	chip->taos_settings.als_cal_target = 130;
+
 	/* Known external ALS reading used for calibration */
+	chip->taos_settings.als_cal_target = 130;
 }
 
 /*
-- 
2.5.5

Re: [PATCH 3/9] staging: iio: tsl2583: fixed ordering of comments

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> in taos_defaults()
> 
> The comments in taos_defaults() appear after the line of code
> that they apply to. This patch moves the comments so that they appear
> before the code. Some of the comments were updated to be more
> informative.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Nice tidying up.

Applied to the togreg branch of iio.git - initially pushed out
as testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 388440b..709f446 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -143,16 +143,23 @@ static const struct gainadj gainadj[] = {
>   */
>  static void taos_defaults(struct tsl2583_chip *chip)
>  {
> -	/* Operational parameters */
> +	/*
> +	 * The integration time must be a multiple of 50ms and within the
> +	 * range [50, 600] ms.
> +	 */
>  	chip->taos_settings.als_time = 100;
> -	/* must be a multiple of 50mS */
> +
> +	/*
> +	 * This is an index into the gainadj table. Assume clear glass as the
> +	 * default.
> +	 */
>  	chip->taos_settings.als_gain = 0;
> -	/* this is actually an index into the gain table */
> -	/* assume clear glass as default */
> +
> +	/* Default gain trim to account for aperture effects */
>  	chip->taos_settings.als_gain_trim = 1000;
> -	/* default gain trim to account for aperture effects */
> -	chip->taos_settings.als_cal_target = 130;
> +
>  	/* Known external ALS reading used for calibration */
> +	chip->taos_settings.als_cal_target = 130;
>  }
>  
>  /*
> 

[PATCH 4/9] staging: iio: tsl2583: remove redundant power off sequence in taos_chip_on()

[Brian Masney]

taos_chip_on() explicitly turns the sensor power on and then writes the
8 registers that are stored in taos_config. The first register in
taos_config is the CONTROL register and the configuration is set to
turn the power off. The existing state sequence in taos_chip_on() is:

- Turn device power on
- Turn device power off (via taos_config)
- Configure other 7 registers (via taos_config)
- Turn device power on, enable ADC

This patch changes the code so that the device is not powered off via
taos_config.

Verified that the driver still functions correctly using a TSL2581
hooked up to a Raspberry Pi 2.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/tsl2583.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 709f446..7fb09c6 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -410,17 +410,8 @@ static int taos_chip_on(struct iio_dev *indio_dev)
 	chip->als_saturation = als_count * 922; /* 90% of full scale */
 	chip->als_time_scale = (als_time + 25) / 50;
 
-	/*
-	 * TSL258x Specific power-on / adc enable sequence
-	 * Power on the device 1st.
-	 */
-	utmp = TSL258X_CNTL_PWR_ON;
-	ret = i2c_smbus_write_byte_data(chip->client,
-					TSL258X_CMD_REG | TSL258X_CNTRL, utmp);
-	if (ret < 0) {
-		dev_err(&chip->client->dev, "taos_chip_on failed on CNTRL reg.\n");
-		return ret;
-	}
+	/* Power on the device; ADC off. */
+	chip->taos_config[TSL258X_CNTRL] = TSL258X_CNTL_PWR_ON;
 
 	/*
 	 * Use the following shadow copy for our delay before enabling ADC.
-- 
2.5.5

Re: [PATCH 4/9] staging: iio: tsl2583: remove redundant power off sequence in taos_chip_on()

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> taos_chip_on() explicitly turns the sensor power on and then writes the
> 8 registers that are stored in taos_config. The first register in
> taos_config is the CONTROL register and the configuration is set to
> turn the power off. The existing state sequence in taos_chip_on() is:
> 
> - Turn device power on
> - Turn device power off (via taos_config)
> - Configure other 7 registers (via taos_config)
> - Turn device power on, enable ADC
> 
> This patch changes the code so that the device is not powered off via
> taos_config.
> 
> Verified that the driver still functions correctly using a TSL2581
> hooked up to a Raspberry Pi 2.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Seems sensible.

Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 709f446..7fb09c6 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -410,17 +410,8 @@ static int taos_chip_on(struct iio_dev *indio_dev)
>  	chip->als_saturation = als_count * 922; /* 90% of full scale */
>  	chip->als_time_scale = (als_time + 25) / 50;
>  
> -	/*
> -	 * TSL258x Specific power-on / adc enable sequence
> -	 * Power on the device 1st.
> -	 */
> -	utmp = TSL258X_CNTL_PWR_ON;
> -	ret = i2c_smbus_write_byte_data(chip->client,
> -					TSL258X_CMD_REG | TSL258X_CNTRL, utmp);
> -	if (ret < 0) {
> -		dev_err(&chip->client->dev, "taos_chip_on failed on CNTRL reg.\n");
> -		return ret;
> -	}
> +	/* Power on the device; ADC off. */
> +	chip->taos_config[TSL258X_CNTRL] = TSL258X_CNTL_PWR_ON;
>  
>  	/*
>  	 * Use the following shadow copy for our delay before enabling ADC.
> 

[PATCH 5/9] staging: iio: tsl2583: don't shutdown chip when updating the lux table

[Brian Masney]

in_illuminance_lux_table_store() shuts down the chip, updates the
contents of the lux table, and then turns the chip back on. The values
in lux table are not used by the chip and are only used internally by
the driver. It is not necessary to change the power state on the chip.
This patch removes the calls to taos_chip_off() and taos_chip_on()
in in_illuminance_lux_table_store().

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/tsl2583.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 7fb09c6..af1cf9b 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -564,20 +564,10 @@ static ssize_t in_illuminance_lux_table_store(struct device *dev,
 		goto done;
 	}
 
-	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
-		ret = taos_chip_off(indio_dev);
-		if (ret < 0)
-			goto done;
-	}
-
 	/* Zero out the table */
 	memset(taos_device_lux, 0, sizeof(taos_device_lux));
 	memcpy(taos_device_lux, &value[1], (value[0] * 4));
 
-	ret = taos_chip_on(indio_dev);
-	if (ret < 0)
-		goto done;
-
 	ret = len;
 
 done:
-- 
2.5.5

Re: [PATCH 5/9] staging: iio: tsl2583: don't shutdown chip when updating the lux table

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> in_illuminance_lux_table_store() shuts down the chip, updates the
> contents of the lux table, and then turns the chip back on. The values
> in lux table are not used by the chip and are only used internally by
> the driver. It is not necessary to change the power state on the chip.
> This patch removes the calls to taos_chip_off() and taos_chip_on()
> in in_illuminance_lux_table_store().
That is rather mystifying...  Maybe a really odd way of locking?
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 10 ----------
>  1 file changed, 10 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 7fb09c6..af1cf9b 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -564,20 +564,10 @@ static ssize_t in_illuminance_lux_table_store(struct device *dev,
>  		goto done;
>  	}
>  
> -	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
> -		ret = taos_chip_off(indio_dev);
> -		if (ret < 0)
> -			goto done;
> -	}
> -
>  	/* Zero out the table */
>  	memset(taos_device_lux, 0, sizeof(taos_device_lux));
>  	memcpy(taos_device_lux, &value[1], (value[0] * 4));
>  
> -	ret = taos_chip_on(indio_dev);
> -	if (ret < 0)
> -		goto done;
> -
>  	ret = len;
>  
>  done:
> 

[PATCH 6/9] staging: iio: tsl2583: remove redudant i2c call in taos_als_calibrate()

[Brian Masney]

taos_als_calibrate() queries the control register to determine if the
unit is powered on and has the ADC enabled. It then queries the same
register a second time to determine if the ADC reading is valid. This
patch removes the redundant i2c_smbus_read_byte_data() call.

Verified that the driver still functions correctly using a TSL2581
hooked up to a Raspberry Pi 2.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/tsl2583.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index af1cf9b..7eab17f 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -338,18 +338,7 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
 		dev_err(&chip->client->dev,
 			"taos_als_calibrate failed: device not powered on with ADC enabled\n");
 		return -EINVAL;
-	}
-
-	ret = i2c_smbus_read_byte_data(chip->client,
-				       TSL258X_CMD_REG | TSL258X_CNTRL);
-	if (ret < 0) {
-		dev_err(&chip->client->dev,
-			"%s failed to read from the CNTRL register\n",
-			__func__);
-		return ret;
-	}
-
-	if ((ret & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
+	} else if ((ret & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
 		dev_err(&chip->client->dev,
 			"taos_als_calibrate failed: STATUS - ADC not valid.\n");
 		return -ENODATA;
-- 
2.5.5

Re: [PATCH 6/9] staging: iio: tsl2583: remove redudant i2c call in taos_als_calibrate()

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> taos_als_calibrate() queries the control register to determine if the
> unit is powered on and has the ADC enabled. It then queries the same
> register a second time to determine if the ADC reading is valid. This
> patch removes the redundant i2c_smbus_read_byte_data() call.
> 
> Verified that the driver still functions correctly using a TSL2581
> hooked up to a Raspberry Pi 2.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index af1cf9b..7eab17f 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -338,18 +338,7 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
>  		dev_err(&chip->client->dev,
>  			"taos_als_calibrate failed: device not powered on with ADC enabled\n");
>  		return -EINVAL;
> -	}
> -
> -	ret = i2c_smbus_read_byte_data(chip->client,
> -				       TSL258X_CMD_REG | TSL258X_CNTRL);
> -	if (ret < 0) {
> -		dev_err(&chip->client->dev,
> -			"%s failed to read from the CNTRL register\n",
> -			__func__);
> -		return ret;
> -	}
> -
> -	if ((ret & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
> +	} else if ((ret & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
>  		dev_err(&chip->client->dev,
>  			"taos_als_calibrate failed: STATUS - ADC not valid.\n");
>  		return -ENODATA;
> 

[PATCH 7/9] staging: iio: tsl2583: fix issue with changes to calibscale and int_time not being set on the chip

[Brian Masney]

When updating the in_illuminance_calibscale and
in_illuminance_integration_time sysfs attributes, these values were not
actually written to the chip. The chip would continue to use the old
parameters. Extracted out tsl2583_set_als_gain() and
tsl2583_set_als_time() functions that are now called when these sysfs
attributes are updated. The chip initialization now calls these these
new functions.

Rename taos_chip_on() to tsl2583_chip_init() since it is now only called
during device probing and when the power management code wakes the
device back up. tsl2583_chip_init() was refactored to use the new
functions mentioned above.

Previously, the current chip state was represented as a tristate
(working, suspended, and unknown). The unknown state was not used. The
chip state is now represented with a single boolean value (suspended).

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/tsl2583.c | 172 ++++++++++++++++--------------------
 1 file changed, 78 insertions(+), 94 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 7eab17f..1ff90b3 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -48,6 +48,8 @@
 #define TSL258X_TMR_LO			0x18
 #define TSL258X_TMR_HI			0x19
 
+#define TSL2583_INTERRUPT_DISABLED	0x00
+
 /* tsl2583 cmd reg masks */
 #define TSL258X_CMD_REG			0x80
 #define TSL258X_CMD_SPL_FN		0x60
@@ -55,6 +57,7 @@
 
 /* tsl2583 cntrl reg masks */
 #define TSL258X_CNTL_ADC_ENBL	0x02
+#define TSL258X_CNTL_PWR_OFF		0x00
 #define TSL258X_CNTL_PWR_ON		0x01
 
 /* tsl2583 status reg masks */
@@ -67,12 +70,6 @@
 #define TSL2583_CHIP_ID			0x90
 #define TSL2583_CHIP_ID_MASK		0xf0
 
-enum {
-	TSL258X_CHIP_UNKNOWN = 0,
-	TSL258X_CHIP_WORKING = 1,
-	TSL258X_CHIP_SUSPENDED = 2
-};
-
 /* Per-device data */
 struct taos_als_info {
 	u16 als_ch0;
@@ -94,19 +91,9 @@ struct tsl2583_chip {
 	struct taos_settings taos_settings;
 	int als_time_scale;
 	int als_saturation;
-	int taos_chip_status;
-	u8 taos_config[8];
+	bool suspended;
 };
 
-/*
- * Initial values for device - this values can/will be changed by driver.
- * and applications as needed.
- * These values are dynamic.
- */
-static const u8 taos_config[8] = {
-		0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00
-}; /*	cntrl atime intC  Athl0 Athl1 Athh0 Athh1 gain */
-
 struct taos_lux {
 	unsigned int ratio;
 	unsigned int ch0;
@@ -188,13 +175,6 @@ static int taos_get_lux(struct iio_dev *indio_dev)
 	u32 ch0lux = 0;
 	u32 ch1lux = 0;
 
-	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
-		/* device is not enabled */
-		dev_err(&chip->client->dev, "taos_get_lux device is not enabled\n");
-		ret = -EBUSY;
-		goto done;
-	}
-
 	ret = i2c_smbus_read_byte_data(chip->client, TSL258X_CMD_REG);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "taos_get_lux failed to read CMD_REG\n");
@@ -362,26 +342,10 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
 	return (int)gain_trim_val;
 }
 
-/*
- * Turn the device on.
- * Configuration must be set before calling this function.
- */
-static int taos_chip_on(struct iio_dev *indio_dev)
+static int tsl2583_set_als_time(struct tsl2583_chip *chip)
 {
-	int i;
-	int ret;
-	u8 *uP;
-	u8 utmp;
-	int als_count;
-	int als_time;
-	struct tsl2583_chip *chip = iio_priv(indio_dev);
-
-	/* and make sure we're not already on */
-	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
-		/* if forcing a register update - turn off, then on */
-		dev_info(&chip->client->dev, "device is already enabled\n");
-		return -EINVAL;
-	}
+	int als_count, als_time, ret;
+	u8 val;
 
 	/* determine als integration register */
 	als_count = (chip->taos_settings.als_time * 100 + 135) / 270;
@@ -390,60 +354,82 @@ static int taos_chip_on(struct iio_dev *indio_dev)
 
 	/* convert back to time (encompasses overrides) */
 	als_time = (als_count * 27 + 5) / 10;
-	chip->taos_config[TSL258X_ALS_TIME] = 256 - als_count;
 
-	/* Set the gain based on taos_settings struct */
-	chip->taos_config[TSL258X_GAIN] = chip->taos_settings.als_gain;
+	val = 256 - als_count;
+	ret = i2c_smbus_write_byte_data(chip->client,
+					TSL258X_CMD_REG | TSL258X_ALS_TIME,
+					val);
+	if (ret < 0) {
+		dev_err(&chip->client->dev,
+			"%s failed to set the als time to %d\n",
+			__func__, val);
+		return ret;
+	}
 
 	/* set chip struct re scaling and saturation */
 	chip->als_saturation = als_count * 922; /* 90% of full scale */
 	chip->als_time_scale = (als_time + 25) / 50;
 
-	/* Power on the device; ADC off. */
-	chip->taos_config[TSL258X_CNTRL] = TSL258X_CNTL_PWR_ON;
+	return ret;
+}
 
-	/*
-	 * Use the following shadow copy for our delay before enabling ADC.
-	 * Write all the registers.
-	 */
-	for (i = 0, uP = chip->taos_config; i < TSL258X_REG_MAX; i++) {
-		ret = i2c_smbus_write_byte_data(chip->client,
-						TSL258X_CMD_REG + i,
-						*uP++);
-		if (ret < 0) {
-			dev_err(&chip->client->dev,
-				"taos_chip_on failed on reg %d.\n", i);
-			return ret;
-		}
-	}
+static int tsl2583_set_als_gain(struct tsl2583_chip *chip)
+{
+	int ret;
 
-	usleep_range(3000, 3500);
-	/*
-	 * NOW enable the ADC
-	 * initialize the desired mode of operation
-	 */
-	utmp = TSL258X_CNTL_PWR_ON | TSL258X_CNTL_ADC_ENBL;
 	ret = i2c_smbus_write_byte_data(chip->client,
-					TSL258X_CMD_REG | TSL258X_CNTRL,
-					utmp);
-	if (ret < 0) {
-		dev_err(&chip->client->dev, "taos_chip_on failed on 2nd CTRL reg.\n");
-		return ret;
-	}
-	chip->taos_chip_status = TSL258X_CHIP_WORKING;
+					TSL258X_CMD_REG | TSL258X_GAIN,
+					chip->taos_settings.als_gain);
+	if (ret < 0)
+		dev_err(&chip->client->dev, "%s failed to set the gain to %d\n",
+			__func__, chip->taos_settings.als_gain);
 
 	return ret;
 }
 
-static int taos_chip_off(struct iio_dev *indio_dev)
+static int tsl2583_set_power_state(struct tsl2583_chip *chip, u8 state)
 {
-	struct tsl2583_chip *chip = iio_priv(indio_dev);
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(chip->client,
+					TSL258X_CMD_REG | TSL258X_CNTRL, state);
+	if (ret < 0)
+		dev_err(&chip->client->dev, "%s failed to set the power state to %d\n",
+			__func__, state);
 
-	/* turn device off */
-	chip->taos_chip_status = TSL258X_CHIP_SUSPENDED;
-	return i2c_smbus_write_byte_data(chip->client,
-					TSL258X_CMD_REG | TSL258X_CNTRL,
-					0x00);
+	return ret;
+}
+
+static int tsl2583_chip_init(struct tsl2583_chip *chip)
+{
+	int ret;
+
+	ret = tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_ON);
+	if (ret < 0)
+		return ret;
+
+	ret = i2c_smbus_write_byte_data(chip->client,
+					TSL258X_CMD_REG | TSL258X_INTERRUPT,
+					TSL2583_INTERRUPT_DISABLED);
+	if (ret < 0) {
+		dev_err(&chip->client->dev, "%s failed to disable interrupts\n",
+			__func__);
+		return ret;
+	}
+
+	ret = tsl2583_set_als_time(chip);
+	if (ret < 0)
+		return ret;
+
+	ret = tsl2583_set_als_gain(chip);
+	if (ret < 0)
+		return ret;
+
+	usleep_range(3000, 3500);
+
+	/* Enable the ADC and initialize the desired mode of operation */
+	return tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_ON |
+					     TSL258X_CNTL_ADC_ENBL);
 }
 
 /* Sysfs Interface Functions */
@@ -616,7 +602,7 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev,
 
 	mutex_lock(&chip->als_mutex);
 
-	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
+	if (chip->suspended) {
 		ret = -EBUSY;
 		goto read_done;
 	}
@@ -693,7 +679,7 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,
 
 	mutex_lock(&chip->als_mutex);
 
-	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
+	if (chip->suspended) {
 		ret = -EBUSY;
 		goto write_done;
 	}
@@ -716,13 +702,14 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,
 					break;
 				}
 			}
+			ret = tsl2583_set_als_gain(chip);
 		}
 		break;
 	case IIO_CHAN_INFO_INT_TIME:
 		if (chan->type == IIO_LIGHT && !val && val2 >= 50 &&
 		    val2 <= 650 && !(val2 % 50)) {
 			chip->taos_settings.als_time = val2;
-			ret = 0;
+			ret = tsl2583_set_als_time(chip);
 		}
 		break;
 	default:
@@ -767,8 +754,7 @@ static int taos_probe(struct i2c_client *clientp,
 	i2c_set_clientdata(clientp, indio_dev);
 
 	mutex_init(&chip->als_mutex);
-	chip->taos_chip_status = TSL258X_CHIP_UNKNOWN;
-	memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
+	chip->suspended = false;
 
 	ret = i2c_smbus_read_byte_data(clientp,
 				       TSL258X_CMD_REG | TSL258X_CHIPID);
@@ -808,7 +794,7 @@ static int taos_probe(struct i2c_client *clientp,
 	taos_defaults(chip);
 
 	/* Make sure the chip is on */
-	ret = taos_chip_on(indio_dev);
+	ret = tsl2583_chip_init(chip);
 	if (ret < 0)
 		return ret;
 
@@ -825,10 +811,8 @@ static int taos_suspend(struct device *dev)
 
 	mutex_lock(&chip->als_mutex);
 
-	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
-		ret = taos_chip_off(indio_dev);
-		chip->taos_chip_status = TSL258X_CHIP_SUSPENDED;
-	}
+	chip->suspended = true;
+	ret = tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_OFF);
 
 	mutex_unlock(&chip->als_mutex);
 	return ret;
@@ -842,8 +826,8 @@ static int taos_resume(struct device *dev)
 
 	mutex_lock(&chip->als_mutex);
 
-	if (chip->taos_chip_status == TSL258X_CHIP_SUSPENDED)
-		ret = taos_chip_on(indio_dev);
+	chip->suspended = false;
+	ret = tsl2583_chip_init(chip);
 
 	mutex_unlock(&chip->als_mutex);
 	return ret;
-- 
2.5.5

Re: [PATCH 7/9] staging: iio: tsl2583: fix issue with changes to calibscale and int_time not being set on the chip

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> When updating the in_illuminance_calibscale and
> in_illuminance_integration_time sysfs attributes, these values were not
> actually written to the chip. The chip would continue to use the old
> parameters. Extracted out tsl2583_set_als_gain() and
> tsl2583_set_als_time() functions that are now called when these sysfs
> attributes are updated. The chip initialization now calls these these
> new functions.
> 
> Rename taos_chip_on() to tsl2583_chip_init() since it is now only called
> during device probing and when the power management code wakes the
> device back up. tsl2583_chip_init() was refactored to use the new
> functions mentioned above.
> 
> Previously, the current chip state was represented as a tristate
> (working, suspended, and unknown). The unknown state was not used. The
> chip state is now represented with a single boolean value (suspended).
Last part should probably have been a separate patch.  Earlier stages could
also have been futher broken up I think to make it easier to review.

The additional init in the resume path should also protect against suspends
which actually cut the power to the chip which is nice.

Just enough bits and pieces inline that I'd like you to do another pass
on this.

Jonathan
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/staging/iio/light/tsl2583.c | 172 ++++++++++++++++--------------------
>  1 file changed, 78 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 7eab17f..1ff90b3 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -48,6 +48,8 @@
>  #define TSL258X_TMR_LO			0x18
>  #define TSL258X_TMR_HI			0x19
>  
> +#define TSL2583_INTERRUPT_DISABLED	0x00
> +
>  /* tsl2583 cmd reg masks */
>  #define TSL258X_CMD_REG			0x80
>  #define TSL258X_CMD_SPL_FN		0x60
> @@ -55,6 +57,7 @@
>  
>  /* tsl2583 cntrl reg masks */
>  #define TSL258X_CNTL_ADC_ENBL	0x02
> +#define TSL258X_CNTL_PWR_OFF		0x00
>  #define TSL258X_CNTL_PWR_ON		0x01
>  
>  /* tsl2583 status reg masks */
> @@ -67,12 +70,6 @@
>  #define TSL2583_CHIP_ID			0x90
>  #define TSL2583_CHIP_ID_MASK		0xf0
>  
> -enum {
> -	TSL258X_CHIP_UNKNOWN = 0,
> -	TSL258X_CHIP_WORKING = 1,
> -	TSL258X_CHIP_SUSPENDED = 2
> -};
> -
>  /* Per-device data */
>  struct taos_als_info {
>  	u16 als_ch0;
> @@ -94,19 +91,9 @@ struct tsl2583_chip {
>  	struct taos_settings taos_settings;
>  	int als_time_scale;
>  	int als_saturation;
> -	int taos_chip_status;
> -	u8 taos_config[8];
> +	bool suspended;
>  };
>  
> -/*
> - * Initial values for device - this values can/will be changed by driver.
> - * and applications as needed.
> - * These values are dynamic.
> - */
> -static const u8 taos_config[8] = {
> -		0x00, 0xee, 0x00, 0x03, 0x00, 0xFF, 0xFF, 0x00
> -}; /*	cntrl atime intC  Athl0 Athl1 Athh0 Athh1 gain */
> -
>  struct taos_lux {
>  	unsigned int ratio;
>  	unsigned int ch0;
> @@ -188,13 +175,6 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  	u32 ch0lux = 0;
>  	u32 ch1lux = 0;
>  
> -	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
> -		/* device is not enabled */
> -		dev_err(&chip->client->dev, "taos_get_lux device is not enabled\n");
> -		ret = -EBUSY;
> -		goto done;
> -	}
> -
This change is because it's already guaranteed to be in the working state?
>  	ret = i2c_smbus_read_byte_data(chip->client, TSL258X_CMD_REG);
>  	if (ret < 0) {
>  		dev_err(&chip->client->dev, "taos_get_lux failed to read CMD_REG\n");
> @@ -362,26 +342,10 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
>  	return (int)gain_trim_val;
>  }
>  
> -/*
> - * Turn the device on.
> - * Configuration must be set before calling this function.
> - */
> -static int taos_chip_on(struct iio_dev *indio_dev)
> +static int tsl2583_set_als_time(struct tsl2583_chip *chip)
>  {
> -	int i;
> -	int ret;
> -	u8 *uP;
> -	u8 utmp;
> -	int als_count;
> -	int als_time;
> -	struct tsl2583_chip *chip = iio_priv(indio_dev);
> -
> -	/* and make sure we're not already on */
> -	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
> -		/* if forcing a register update - turn off, then on */
> -		dev_info(&chip->client->dev, "device is already enabled\n");
> -		return -EINVAL;
> -	}
> +	int als_count, als_time, ret;
> +	u8 val;
>  
>  	/* determine als integration register */
>  	als_count = (chip->taos_settings.als_time * 100 + 135) / 270;
> @@ -390,60 +354,82 @@ static int taos_chip_on(struct iio_dev *indio_dev)
>  
>  	/* convert back to time (encompasses overrides) */
>  	als_time = (als_count * 27 + 5) / 10;
> -	chip->taos_config[TSL258X_ALS_TIME] = 256 - als_count;
>  
> -	/* Set the gain based on taos_settings struct */
> -	chip->taos_config[TSL258X_GAIN] = chip->taos_settings.als_gain;
> +	val = 256 - als_count;
> +	ret = i2c_smbus_write_byte_data(chip->client,
> +					TSL258X_CMD_REG | TSL258X_ALS_TIME,
> +					val);
> +	if (ret < 0) {
> +		dev_err(&chip->client->dev,
> +			"%s failed to set the als time to %d\n",
> +			__func__, val);
> +		return ret;
> +	}
>  
>  	/* set chip struct re scaling and saturation */
>  	chip->als_saturation = als_count * 922; /* 90% of full scale */
>  	chip->als_time_scale = (als_time + 25) / 50;
>  
> -	/* Power on the device; ADC off. */
> -	chip->taos_config[TSL258X_CNTRL] = TSL258X_CNTL_PWR_ON;
> +	return ret;
> +}
>  
> -	/*
> -	 * Use the following shadow copy for our delay before enabling ADC.
> -	 * Write all the registers.
> -	 */
> -	for (i = 0, uP = chip->taos_config; i < TSL258X_REG_MAX; i++) {
> -		ret = i2c_smbus_write_byte_data(chip->client,
> -						TSL258X_CMD_REG + i,
> -						*uP++);
> -		if (ret < 0) {
> -			dev_err(&chip->client->dev,
> -				"taos_chip_on failed on reg %d.\n", i);
> -			return ret;
> -		}
> -	}
> +static int tsl2583_set_als_gain(struct tsl2583_chip *chip)
> +{
> +	int ret;
>  
> -	usleep_range(3000, 3500);
> -	/*
> -	 * NOW enable the ADC
> -	 * initialize the desired mode of operation
> -	 */
> -	utmp = TSL258X_CNTL_PWR_ON | TSL258X_CNTL_ADC_ENBL;
>  	ret = i2c_smbus_write_byte_data(chip->client,
> -					TSL258X_CMD_REG | TSL258X_CNTRL,
> -					utmp);
> -	if (ret < 0) {
> -		dev_err(&chip->client->dev, "taos_chip_on failed on 2nd CTRL reg.\n");
> -		return ret;
> -	}
> -	chip->taos_chip_status = TSL258X_CHIP_WORKING;
> +					TSL258X_CMD_REG | TSL258X_GAIN,
> +					chip->taos_settings.als_gain);
> +	if (ret < 0)
> +		dev_err(&chip->client->dev, "%s failed to set the gain to %d\n",
> +			__func__, chip->taos_settings.als_gain);
>  
>  	return ret;
>  }
>  
> -static int taos_chip_off(struct iio_dev *indio_dev)
> +static int tsl2583_set_power_state(struct tsl2583_chip *chip, u8 state)
>  {
> -	struct tsl2583_chip *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(chip->client,
> +					TSL258X_CMD_REG | TSL258X_CNTRL, state);
> +	if (ret < 0)
> +		dev_err(&chip->client->dev, "%s failed to set the power state to %d\n",
> +			__func__, state);
>  
> -	/* turn device off */
> -	chip->taos_chip_status = TSL258X_CHIP_SUSPENDED;
> -	return i2c_smbus_write_byte_data(chip->client,
> -					TSL258X_CMD_REG | TSL258X_CNTRL,
> -					0x00);
> +	return ret;
> +}
> +
> +static int tsl2583_chip_init(struct tsl2583_chip *chip)
> +{
> +	int ret;
> +
> +	ret = tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_ON);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte_data(chip->client,
> +					TSL258X_CMD_REG | TSL258X_INTERRUPT,
> +					TSL2583_INTERRUPT_DISABLED);
> +	if (ret < 0) {
> +		dev_err(&chip->client->dev, "%s failed to disable interrupts\n",
> +			__func__);
> +		return ret;
> +	}
> +
> +	ret = tsl2583_set_als_time(chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tsl2583_set_als_gain(chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	usleep_range(3000, 3500);
> +
> +	/* Enable the ADC and initialize the desired mode of operation */
> +	return tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_ON |
> +					     TSL258X_CNTL_ADC_ENBL);
>  }
>  
>  /* Sysfs Interface Functions */
> @@ -616,7 +602,7 @@ static int tsl2583_read_raw(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&chip->als_mutex);
>  
> -	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
> +	if (chip->suspended) {
>  		ret = -EBUSY;
>  		goto read_done;
>  	}
> @@ -693,7 +679,7 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,
>  
>  	mutex_lock(&chip->als_mutex);
>  
> -	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
> +	if (chip->suspended) {
>  		ret = -EBUSY;
>  		goto write_done;
>  	}
> @@ -716,13 +702,14 @@ static int tsl2583_write_raw(struct iio_dev *indio_dev,
>  					break;
>  				}
>  			}
> +			ret = tsl2583_set_als_gain(chip);
>  		}
>  		break;
>  	case IIO_CHAN_INFO_INT_TIME:
>  		if (chan->type == IIO_LIGHT && !val && val2 >= 50 &&
>  		    val2 <= 650 && !(val2 % 50)) {
>  			chip->taos_settings.als_time = val2;
> -			ret = 0;
> +			ret = tsl2583_set_als_time(chip);
>  		}
>  		break;
>  	default:
> @@ -767,8 +754,7 @@ static int taos_probe(struct i2c_client *clientp,
>  	i2c_set_clientdata(clientp, indio_dev);
>  
>  	mutex_init(&chip->als_mutex);
> -	chip->taos_chip_status = TSL258X_CHIP_UNKNOWN;
> -	memcpy(chip->taos_config, taos_config, sizeof(chip->taos_config));
> +	chip->suspended = false;
This should really be set after the power up rather than here...
>  
>  	ret = i2c_smbus_read_byte_data(clientp,
>  				       TSL258X_CMD_REG | TSL258X_CHIPID);
> @@ -808,7 +794,7 @@ static int taos_probe(struct i2c_client *clientp,
>  	taos_defaults(chip);
>  
>  	/* Make sure the chip is on */
> -	ret = taos_chip_on(indio_dev);
> +	ret = tsl2583_chip_init(chip);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -825,10 +811,8 @@ static int taos_suspend(struct device *dev)
>  
>  	mutex_lock(&chip->als_mutex);
>  
> -	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
> -		ret = taos_chip_off(indio_dev);
> -		chip->taos_chip_status = TSL258X_CHIP_SUSPENDED;
> -	}
> +	chip->suspended = true;
> +	ret = tsl2583_set_power_state(chip, TSL258X_CNTL_PWR_OFF);
>  
>  	mutex_unlock(&chip->als_mutex);
>  	return ret;
> @@ -842,8 +826,8 @@ static int taos_resume(struct device *dev)
>  
>  	mutex_lock(&chip->als_mutex);
>  
> -	if (chip->taos_chip_status == TSL258X_CHIP_SUSPENDED)
> -		ret = taos_chip_on(indio_dev);
> +	chip->suspended = false;
> +	ret = tsl2583_chip_init(chip);
Would logically expect the two lines above to 'reflect' those in
the suspend so to be in the opposite order.  I'd also be tempted to
name chip_init as something like tsl2583_chip_init_and_power_up
to make it clear that the revers is the power off state above.

>  
>  	mutex_unlock(&chip->als_mutex);
>  	return ret;
> 

Re: [PATCH 7/9] staging: iio: tsl2583: fix issue with changes to calibscale and int_time not being set on the chip

[Brian Masney]

On Sun, Nov 06, 2016 at 12:03:53PM +0000, Jonathan Cameron wrote:
> On 03/11/16 12:56, Brian Masney wrote:
> > When updating the in_illuminance_calibscale and
> > in_illuminance_integration_time sysfs attributes, these values were not
> > actually written to the chip. The chip would continue to use the old
> > parameters. Extracted out tsl2583_set_als_gain() and
> > tsl2583_set_als_time() functions that are now called when these sysfs
> > attributes are updated. The chip initialization now calls these these
> > new functions.
> > 
> > Rename taos_chip_on() to tsl2583_chip_init() since it is now only called
> > during device probing and when the power management code wakes the
> > device back up. tsl2583_chip_init() was refactored to use the new
> > functions mentioned above.
> > 
> > Previously, the current chip state was represented as a tristate
> > (working, suspended, and unknown). The unknown state was not used. The
> > chip state is now represented with a single boolean value (suspended).
> Last part should probably have been a separate patch.  Earlier stages could
> also have been futher broken up I think to make it easier to review.
> 
> The additional init in the resume path should also protect against suspends
> which actually cut the power to the chip which is nice.
> 
> Just enough bits and pieces inline that I'd like you to do another pass
> on this.

No problem, I'll split this one up for you. My next patch series will
also contain a lot of trivial code cleanups, some documentation updates, and
a request to move the driver out of staging.

The device tree documentation
(Documentation/devicetree/bindings/iio/light/tsl2583.txt) has the
interrupt-parent and interrupts properties as optional, however the
driver does not support interrupts. Should I remove these properties from
the device tree documentation? I can add the code to support the
interrupts but I am hesistant to add that new code if no one will use it.

Brian

Re: [PATCH 7/9] staging: iio: tsl2583: fix issue with changes to calibscale and int_time not being set on the chip

[Jonathan Cameron]

On 06/11/16 14:23, Brian Masney wrote:
> On Sun, Nov 06, 2016 at 12:03:53PM +0000, Jonathan Cameron wrote:
>> On 03/11/16 12:56, Brian Masney wrote:
>>> When updating the in_illuminance_calibscale and
>>> in_illuminance_integration_time sysfs attributes, these values were not
>>> actually written to the chip. The chip would continue to use the old
>>> parameters. Extracted out tsl2583_set_als_gain() and
>>> tsl2583_set_als_time() functions that are now called when these sysfs
>>> attributes are updated. The chip initialization now calls these these
>>> new functions.
>>>
>>> Rename taos_chip_on() to tsl2583_chip_init() since it is now only called
>>> during device probing and when the power management code wakes the
>>> device back up. tsl2583_chip_init() was refactored to use the new
>>> functions mentioned above.
>>>
>>> Previously, the current chip state was represented as a tristate
>>> (working, suspended, and unknown). The unknown state was not used. The
>>> chip state is now represented with a single boolean value (suspended).
>> Last part should probably have been a separate patch.  Earlier stages could
>> also have been futher broken up I think to make it easier to review.
>>
>> The additional init in the resume path should also protect against suspends
>> which actually cut the power to the chip which is nice.
>>
>> Just enough bits and pieces inline that I'd like you to do another pass
>> on this.
> 
> No problem, I'll split this one up for you. My next patch series will
> also contain a lot of trivial code cleanups, some documentation updates, and
> a request to move the driver out of staging.
Cool
> 
> The device tree documentation
> (Documentation/devicetree/bindings/iio/light/tsl2583.txt) has the
> interrupt-parent and interrupts properties as optional, however the
> driver does not support interrupts. Should I remove these properties from
> the device tree documentation? I can add the code to support the
> interrupts but I am hesistant to add that new code if no one will use it.
> 
> Brian
> 
Leave them there.  Device tree is about documenting the hardware,
not just those bits we get around to using ;)

Obviously we have to document what we use but no problem documenting
other bits!

Jonathan

[PATCH 8/9] staging: iio: tsl2583: check if chip is suspended in in_illuminance_calibrate_store

[Brian Masney]

in_illuminance_calibrate_store() did not check to see if the chip is
suspended. This patch adds the proper check. The return value from
taos_als_calibrate() was also not checked in this function, so the
proper check was also added while changes are being made here.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/tsl2583.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 1ff90b3..8c8361d 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -473,16 +473,27 @@ static ssize_t in_illuminance_calibrate_store(struct device *dev,
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
-	int value;
+	int value, ret;
 
 	if (kstrtoint(buf, 0, &value) || value != 1)
 		return -EINVAL;
 
 	mutex_lock(&chip->als_mutex);
-	taos_als_calibrate(indio_dev);
+
+	if (chip->suspended) {
+		ret = -EBUSY;
+		goto done;
+	}
+
+	ret = taos_als_calibrate(indio_dev);
+	if (ret < 0)
+		goto done;
+
+	ret = len;
+done:
 	mutex_unlock(&chip->als_mutex);
 
-	return len;
+	return ret;
 }
 
 static ssize_t in_illuminance_lux_table_show(struct device *dev,
-- 
2.5.5

Re: [PATCH 8/9] staging: iio: tsl2583: check if chip is suspended in in_illuminance_calibrate_store

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> in_illuminance_calibrate_store() did not check to see if the chip is
> suspended. This patch adds the proper check. The return value from
> taos_als_calibrate() was also not checked in this function, so the
> proper check was also added while changes are being made here.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
This one is fine, I'll pick up after patch 7 stuff is sorted.

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 1ff90b3..8c8361d 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -473,16 +473,27 @@ static ssize_t in_illuminance_calibrate_store(struct device *dev,
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> -	int value;
> +	int value, ret;
>  
>  	if (kstrtoint(buf, 0, &value) || value != 1)
>  		return -EINVAL;
>  
>  	mutex_lock(&chip->als_mutex);
> -	taos_als_calibrate(indio_dev);
> +
> +	if (chip->suspended) {
> +		ret = -EBUSY;
> +		goto done;
> +	}
> +
> +	ret = taos_als_calibrate(indio_dev);
> +	if (ret < 0)
> +		goto done;
> +
> +	ret = len;
> +done:
>  	mutex_unlock(&chip->als_mutex);
>  
> -	return len;
> +	return ret;
>  }
>  
>  static ssize_t in_illuminance_lux_table_show(struct device *dev,
> 

[PATCH 9/9] staging: iio: tsl2583: remove redundant write to the control register in taos_probe()

[Brian Masney]

taos_probe() calls i2c_smbus_write_byte() to select the control
register, however there is no subsequent calls to i2c_smbus_read_byte().
The write call is unnecessary and is removed by this patch.

Verified that the driver still functions correctly using a TSL2581
hooked up to a Raspberry Pi 2.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
 drivers/staging/iio/light/tsl2583.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 8c8361d..e80c027 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -781,14 +781,6 @@ static int taos_probe(struct i2c_client *clientp,
 		return -EINVAL;
 	}
 
-	ret = i2c_smbus_write_byte(clientp, (TSL258X_CMD_REG | TSL258X_CNTRL));
-	if (ret < 0) {
-		dev_err(&clientp->dev,
-			"i2c_smbus_write_byte() to cmd reg failed in taos_probe(), err = %d\n",
-			ret);
-		return ret;
-	}
-
 	indio_dev->info = &tsl2583_info;
 	indio_dev->channels = tsl2583_channels;
 	indio_dev->num_channels = ARRAY_SIZE(tsl2583_channels);
-- 
2.5.5

Re: [PATCH 9/9] staging: iio: tsl2583: remove redundant write to the control register in taos_probe()

[Jonathan Cameron]

On 03/11/16 12:56, Brian Masney wrote:
> taos_probe() calls i2c_smbus_write_byte() to select the control
> register, however there is no subsequent calls to i2c_smbus_read_byte().
> The write call is unnecessary and is removed by this patch.
> 
> Verified that the driver still functions correctly using a TSL2581
> hooked up to a Raspberry Pi 2.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Again, looks fine - will pickup when precursors are sorted.

Another generally nice series Brian.

Thanks!

Jonathan
> ---
>  drivers/staging/iio/light/tsl2583.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 8c8361d..e80c027 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -781,14 +781,6 @@ static int taos_probe(struct i2c_client *clientp,
>  		return -EINVAL;
>  	}
>  
> -	ret = i2c_smbus_write_byte(clientp, (TSL258X_CMD_REG | TSL258X_CNTRL));
> -	if (ret < 0) {
> -		dev_err(&clientp->dev,
> -			"i2c_smbus_write_byte() to cmd reg failed in taos_probe(), err = %d\n",
> -			ret);
> -		return ret;
> -	}
> -
>  	indio_dev->info = &tsl2583_info;
>  	indio_dev->channels = tsl2583_channels;
>  	indio_dev->num_channels = ARRAY_SIZE(tsl2583_channels);
>