[PATCH v2 0/5] stk3310 fixes and cleanup

[PATCH v2 0/5] stk3310 fixes and cleanup

[Hartmut Knaack]

Changes in V2:
  * move interrupt setup before device registration, as pointed out by
    Jonathan and confirmed by Tiberiu Breana
  * incooperate a check for negative-only values of the provided irq
    number, as requested by Tiberiu Breana

Hartmut Knaack (5):
  iio:light:stk3310: move device register to end of probe
  iio:light:stk3310: make endianness independent of host
  iio:light:stk3310: add more error handling
  iio:light:stk3310: use correct names and type for state
  iio:light:stk3310: adjust indentation

 drivers/iio/light/stk3310.c | 97 ++++++++++++++++++++++++++++++---------------
 1 file changed, 64 insertions(+), 33 deletions(-)

-- 
2.4.3

[PATCH v2 1/5] iio:light:stk3310: move device register to end of probe

[Hartmut Knaack]

iio_device_register should be the last operation during probe. Therefor
move up interrupt setup code and while at it, change the check for invalid
values of client->irq to be smaller than zero.
Fixes: 3dd477acbdd1 ("iio: light: Add threshold interrupt support for STK3310")

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/light/stk3310.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index c1a218236be5..e7e6e5a2b1f8 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -608,13 +608,7 @@ static int stk3310_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	ret = iio_device_register(indio_dev);
-	if (ret < 0) {
-		dev_err(&client->dev, "device_register failed\n");
-		stk3310_set_state(data, STK3310_STATE_STANDBY);
-	}
-
-	if (client->irq <= 0)
+	if (client->irq < 0)
 		client->irq = stk3310_gpio_probe(client);
 
 	if (client->irq >= 0) {
@@ -629,6 +623,12 @@ static int stk3310_probe(struct i2c_client *client,
 					client->irq);
 	}
 
+	ret = iio_device_register(indio_dev);
+	if (ret < 0) {
+		dev_err(&client->dev, "device_register failed\n");
+		stk3310_set_state(data, STK3310_STATE_STANDBY);
+	}
+
 	return ret;
 }
 
-- 
2.4.3

[PATCH v2 2/5] iio:light:stk3310: make endianness independent of host

[Hartmut Knaack]

Data is stored in the device in be16 format. Make use of be16_to_cpu and
cpu_to_be16 to have correct endianness on any host architecture.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/light/stk3310.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index e7e6e5a2b1f8..11a027adc204 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -200,7 +200,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
 			      int *val, int *val2)
 {
 	u8 reg;
-	u16 buf;
+	__be16 buf;
 	int ret;
 	struct stk3310_data *data = iio_priv(indio_dev);
 
@@ -222,7 +222,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
 		dev_err(&data->client->dev, "register read failed\n");
 		return ret;
 	}
-	*val = swab16(buf);
+	*val = be16_to_cpu(buf);
 
 	return IIO_VAL_INT;
 }
@@ -235,7 +235,7 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
 			       int val, int val2)
 {
 	u8 reg;
-	u16 buf;
+	__be16 buf;
 	int ret;
 	unsigned int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
@@ -252,7 +252,7 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
 	else
 		return -EINVAL;
 
-	buf = swab16(val);
+	buf = cpu_to_be16(val);
 	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
 	if (ret < 0)
 		dev_err(&client->dev, "failed to set PS threshold!\n");
@@ -301,7 +301,7 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 			    int *val, int *val2, long mask)
 {
 	u8 reg;
-	u16 buf;
+	__be16 buf;
 	int ret;
 	unsigned int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
@@ -322,7 +322,7 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 			mutex_unlock(&data->lock);
 			return ret;
 		}
-		*val = swab16(buf);
+		*val = be16_to_cpu(buf);
 		mutex_unlock(&data->lock);
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_INT_TIME:
-- 
2.4.3

[PATCH v2 3/5] iio:light:stk3310: add more error handling

[Hartmut Knaack]

Check for the following error cases:
  * lower boundary for val in _write_event
  * return value of regmap_(field_)read
  * possible values for chan->type
  * return value of stk3310_gpio_probe

Also add an error path in _probe to put the sensor back into stand-by mode
in case of serious errors.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/light/stk3310.c | 59 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 14 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 11a027adc204..d6df40220007 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -241,8 +241,11 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
 	struct stk3310_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
 
-	regmap_field_read(data->reg_ps_gain, &index);
-	if (val > stk3310_ps_max[index])
+	ret = regmap_field_read(data->reg_ps_gain, &index);
+	if (ret < 0)
+		return ret;
+
+	if (val < 0 || val > stk3310_ps_max[index])
 		return -EINVAL;
 
 	if (dir == IIO_EV_DIR_RISING)
@@ -266,9 +269,12 @@ static int stk3310_read_event_config(struct iio_dev *indio_dev,
 				     enum iio_event_direction dir)
 {
 	unsigned int event_val;
+	int ret;
 	struct stk3310_data *data = iio_priv(indio_dev);
 
-	regmap_field_read(data->reg_int_ps, &event_val);
+	ret = regmap_field_read(data->reg_int_ps, &event_val);
+	if (ret < 0)
+		return ret;
 
 	return event_val;
 }
@@ -307,14 +313,16 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 	struct stk3310_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
 
+	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		if (chan->type == IIO_LIGHT)
 			reg = STK3310_REG_ALS_DATA_MSB;
-		else if (chan->type == IIO_PROXIMITY)
-			reg = STK3310_REG_PS_DATA_MSB;
 		else
-			return -EINVAL;
+			reg = STK3310_REG_PS_DATA_MSB;
+
 		mutex_lock(&data->lock);
 		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
 		if (ret < 0) {
@@ -327,17 +335,23 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_INT_TIME:
 		if (chan->type == IIO_LIGHT)
-			regmap_field_read(data->reg_als_it, &index);
+			ret = regmap_field_read(data->reg_als_it, &index);
 		else
-			regmap_field_read(data->reg_ps_it, &index);
+			ret = regmap_field_read(data->reg_ps_it, &index);
+		if (ret < 0)
+			return ret;
+
 		*val = stk3310_it_table[index][0];
 		*val2 = stk3310_it_table[index][1];
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type == IIO_LIGHT)
-			regmap_field_read(data->reg_als_gain, &index);
+			ret = regmap_field_read(data->reg_als_gain, &index);
 		else
-			regmap_field_read(data->reg_ps_gain, &index);
+			ret = regmap_field_read(data->reg_ps_gain, &index);
+		if (ret < 0)
+			return ret;
+
 		*val = stk3310_scale_table[index][0];
 		*val2 = stk3310_scale_table[index][1];
 		return IIO_VAL_INT_PLUS_MICRO;
@@ -354,6 +368,9 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 	int index;
 	struct stk3310_data *data = iio_priv(indio_dev);
 
+	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
+		return -EINVAL;
+
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
 		index = stk3310_get_index(stk3310_it_table,
@@ -435,7 +452,10 @@ static int stk3310_init(struct iio_dev *indio_dev)
 	struct stk3310_data *data = iio_priv(indio_dev);
 	struct i2c_client *client = data->client;
 
-	regmap_read(data->regmap, STK3310_REG_ID, &chipid);
+	ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
+	if (ret < 0)
+		return ret;
+
 	if (chipid != STK3310_CHIP_ID_VAL &&
 	    chipid != STK3311_CHIP_ID_VAL) {
 		dev_err(&client->dev, "invalid chip id: 0x%x\n", chipid);
@@ -608,8 +628,13 @@ static int stk3310_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	if (client->irq < 0)
+	if (client->irq < 0) {
 		client->irq = stk3310_gpio_probe(client);
+		if (client->irq < 0) {
+			ret = client->irq;
+			goto err_standby;
+		}
+	}
 
 	if (client->irq >= 0) {
 		ret = devm_request_threaded_irq(&client->dev, client->irq,
@@ -618,17 +643,23 @@ static int stk3310_probe(struct i2c_client *client,
 						IRQF_TRIGGER_FALLING |
 						IRQF_ONESHOT,
 						STK3310_EVENT, indio_dev);
-		if (ret < 0)
+		if (ret < 0) {
 			dev_err(&client->dev, "request irq %d failed\n",
 					client->irq);
+			goto err_standby;
+		}
 	}
 
 	ret = iio_device_register(indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev, "device_register failed\n");
-		stk3310_set_state(data, STK3310_STATE_STANDBY);
+		goto err_standby;
 	}
 
+	return 0;
+
+err_standby:
+	stk3310_set_state(data, STK3310_STATE_STANDBY);
 	return ret;
 }
 
-- 
2.4.3

[PATCH v2 4/5] iio:light:stk3310: use correct names and type for state

[Hartmut Knaack]

Indicate the bit number of predefined states, make use of these names and
change the state type in _resume to u8 to avoid type casting.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/light/stk3310.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index d6df40220007..0056f4870357 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -35,8 +35,8 @@
 #define STK3310_REG_ID				0x3E
 #define STK3310_MAX_REG				0x80
 
-#define STK3310_STATE_EN_PS			0x01
-#define STK3310_STATE_EN_ALS			0x02
+#define STK3310_STATE_EN_PS			BIT(0)
+#define STK3310_STATE_EN_ALS			BIT(1)
 #define STK3310_STATE_STANDBY			0x00
 
 #define STK3310_CHIP_ID_VAL			0x13
@@ -436,8 +436,8 @@ static int stk3310_set_state(struct stk3310_data *data, u8 state)
 		dev_err(&client->dev, "failed to change sensor state\n");
 	} else if (state != STK3310_STATE_STANDBY) {
 		/* Don't reset the 'enabled' flags if we're going in standby */
-		data->ps_enabled  = !!(state & 0x01);
-		data->als_enabled = !!(state & 0x02);
+		data->ps_enabled  = !!(state & STK3310_STATE_EN_PS);
+		data->als_enabled = !!(state & STK3310_STATE_EN_ALS);
 	}
 	mutex_unlock(&data->lock);
 
@@ -683,7 +683,7 @@ static int stk3310_suspend(struct device *dev)
 
 static int stk3310_resume(struct device *dev)
 {
-	int state = 0;
+	u8 state = 0;
 	struct stk3310_data *data;
 
 	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
-- 
2.4.3

[PATCH v2 5/5] iio:light:stk3310: adjust indentation

[Hartmut Knaack]

Adjust some indentation issues as spotted by checkpatch.pl --strict

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
---
 drivers/iio/light/stk3310.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index 0056f4870357..e73f6f27cc56 100644
--- a/drivers/iio/light/stk3310.c
+++ b/drivers/iio/light/stk3310.c
@@ -385,7 +385,7 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 			ret = regmap_field_write(data->reg_ps_it, index);
 		if (ret < 0)
 			dev_err(&data->client->dev,
-					"sensor configuration failed\n");
+				"sensor configuration failed\n");
 		mutex_unlock(&data->lock);
 		return ret;
 
@@ -402,7 +402,7 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
 			ret = regmap_field_write(data->reg_ps_gain, index);
 		if (ret < 0)
 			dev_err(&data->client->dev,
-					"sensor configuration failed\n");
+				"sensor configuration failed\n");
 		mutex_unlock(&data->lock);
 		return ret;
 	}
@@ -645,7 +645,7 @@ static int stk3310_probe(struct i2c_client *client,
 						STK3310_EVENT, indio_dev);
 		if (ret < 0) {
 			dev_err(&client->dev, "request irq %d failed\n",
-					client->irq);
+				client->irq);
 			goto err_standby;
 		}
 	}
-- 
2.4.3

RE: [PATCH v2 0/5] stk3310 fixes and cleanup

[Breana, Tiberiu A]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Friday, July 10, 2015 12:51 AM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH v2 0/5] stk3310 fixes and cleanup
>=20
> Changes in V2:
>   * move interrupt setup before device registration, as pointed out by
>     Jonathan and confirmed by Tiberiu Breana
>   * incooperate a check for negative-only values of the provided irq
>     number, as requested by Tiberiu Breana
>=20
> Hartmut Knaack (5):
>   iio:light:stk3310: move device register to end of probe
>   iio:light:stk3310: make endianness independent of host
>   iio:light:stk3310: add more error handling
>   iio:light:stk3310: use correct names and type for state
>   iio:light:stk3310: adjust indentation
>=20
>  drivers/iio/light/stk3310.c | 97 ++++++++++++++++++++++++++++++--------
> -------
>  1 file changed, 64 insertions(+), 33 deletions(-)
>=20
> --
> 2.4.3

+1

Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>

Re: [PATCH v2 1/5] iio:light:stk3310: move device register to end of probe

[Jonathan Cameron]

On 09/07/15 22:51, Hartmut Knaack wrote:
> iio_device_register should be the last operation during probe. Therefor
> move up interrupt setup code and while at it, change the check for invalid
> values of client->irq to be smaller than zero.
> Fixes: 3dd477acbdd1 ("iio: light: Add threshold interrupt support for STK3310")
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Applied to the fixes-togreg branch of iio.git

I added some stuff about why this ordering matters.

Jonathan
> ---
>  drivers/iio/light/stk3310.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index c1a218236be5..e7e6e5a2b1f8 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -608,13 +608,7 @@ static int stk3310_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "device_register failed\n");
> -		stk3310_set_state(data, STK3310_STATE_STANDBY);
> -	}
> -
> -	if (client->irq <= 0)
> +	if (client->irq < 0)
>  		client->irq = stk3310_gpio_probe(client);
>  
>  	if (client->irq >= 0) {
> @@ -629,6 +623,12 @@ static int stk3310_probe(struct i2c_client *client,
>  					client->irq);
>  	}
>  
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "device_register failed\n");
> +		stk3310_set_state(data, STK3310_STATE_STANDBY);
> +	}
> +
>  	return ret;
>  }
>  
> 

Re: [PATCH v2 2/5] iio:light:stk3310: make endianness independent of host

[Jonathan Cameron]

On 09/07/15 22:51, Hartmut Knaack wrote:
> Data is stored in the device in be16 format. Make use of be16_to_cpu and
> cpu_to_be16 to have correct endianness on any host architecture.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Applied to the fixes-togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/iio/light/stk3310.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index e7e6e5a2b1f8..11a027adc204 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -200,7 +200,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
>  			      int *val, int *val2)
>  {
>  	u8 reg;
> -	u16 buf;
> +	__be16 buf;
>  	int ret;
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  
> @@ -222,7 +222,7 @@ static int stk3310_read_event(struct iio_dev *indio_dev,
>  		dev_err(&data->client->dev, "register read failed\n");
>  		return ret;
>  	}
> -	*val = swab16(buf);
> +	*val = be16_to_cpu(buf);
>  
>  	return IIO_VAL_INT;
>  }
> @@ -235,7 +235,7 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
>  			       int val, int val2)
>  {
>  	u8 reg;
> -	u16 buf;
> +	__be16 buf;
>  	int ret;
>  	unsigned int index;
>  	struct stk3310_data *data = iio_priv(indio_dev);
> @@ -252,7 +252,7 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
>  	else
>  		return -EINVAL;
>  
> -	buf = swab16(val);
> +	buf = cpu_to_be16(val);
>  	ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
>  	if (ret < 0)
>  		dev_err(&client->dev, "failed to set PS threshold!\n");
> @@ -301,7 +301,7 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>  			    int *val, int *val2, long mask)
>  {
>  	u8 reg;
> -	u16 buf;
> +	__be16 buf;
>  	int ret;
>  	unsigned int index;
>  	struct stk3310_data *data = iio_priv(indio_dev);
> @@ -322,7 +322,7 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>  			mutex_unlock(&data->lock);
>  			return ret;
>  		}
> -		*val = swab16(buf);
> +		*val = be16_to_cpu(buf);
>  		mutex_unlock(&data->lock);
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_INT_TIME:
> 

Re: [PATCH v2 3/5] iio:light:stk3310: add more error handling

[Jonathan Cameron]

On 09/07/15 22:51, Hartmut Knaack wrote:
> Check for the following error cases:
>   * lower boundary for val in _write_event
>   * return value of regmap_(field_)read
>   * possible values for chan->type
>   * return value of stk3310_gpio_probe
> 
> Also add an error path in _probe to put the sensor back into stand-by mode
> in case of serious errors.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
looks good.  Will have to hold it until the fixes have worked there way
through though.
> ---
>  drivers/iio/light/stk3310.c | 59 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 11a027adc204..d6df40220007 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -241,8 +241,11 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  	struct i2c_client *client = data->client;
>  
> -	regmap_field_read(data->reg_ps_gain, &index);
> -	if (val > stk3310_ps_max[index])
> +	ret = regmap_field_read(data->reg_ps_gain, &index);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (val < 0 || val > stk3310_ps_max[index])
>  		return -EINVAL;
>  
>  	if (dir == IIO_EV_DIR_RISING)
> @@ -266,9 +269,12 @@ static int stk3310_read_event_config(struct iio_dev *indio_dev,
>  				     enum iio_event_direction dir)
>  {
>  	unsigned int event_val;
> +	int ret;
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  
> -	regmap_field_read(data->reg_int_ps, &event_val);
> +	ret = regmap_field_read(data->reg_int_ps, &event_val);
> +	if (ret < 0)
> +		return ret;
>  
>  	return event_val;
>  }
> @@ -307,14 +313,16 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  	struct i2c_client *client = data->client;
>  
> +	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  		if (chan->type == IIO_LIGHT)
>  			reg = STK3310_REG_ALS_DATA_MSB;
> -		else if (chan->type == IIO_PROXIMITY)
> -			reg = STK3310_REG_PS_DATA_MSB;
>  		else
> -			return -EINVAL;
> +			reg = STK3310_REG_PS_DATA_MSB;
> +
>  		mutex_lock(&data->lock);
>  		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
>  		if (ret < 0) {
> @@ -327,17 +335,23 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_INT_TIME:
>  		if (chan->type == IIO_LIGHT)
> -			regmap_field_read(data->reg_als_it, &index);
> +			ret = regmap_field_read(data->reg_als_it, &index);
>  		else
> -			regmap_field_read(data->reg_ps_it, &index);
> +			ret = regmap_field_read(data->reg_ps_it, &index);
> +		if (ret < 0)
> +			return ret;
> +
>  		*val = stk3310_it_table[index][0];
>  		*val2 = stk3310_it_table[index][1];
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_SCALE:
>  		if (chan->type == IIO_LIGHT)
> -			regmap_field_read(data->reg_als_gain, &index);
> +			ret = regmap_field_read(data->reg_als_gain, &index);
>  		else
> -			regmap_field_read(data->reg_ps_gain, &index);
> +			ret = regmap_field_read(data->reg_ps_gain, &index);
> +		if (ret < 0)
> +			return ret;
> +
>  		*val = stk3310_scale_table[index][0];
>  		*val2 = stk3310_scale_table[index][1];
>  		return IIO_VAL_INT_PLUS_MICRO;
> @@ -354,6 +368,9 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
>  	int index;
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  
> +	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
> +		return -EINVAL;
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_INT_TIME:
>  		index = stk3310_get_index(stk3310_it_table,
> @@ -435,7 +452,10 @@ static int stk3310_init(struct iio_dev *indio_dev)
>  	struct stk3310_data *data = iio_priv(indio_dev);
>  	struct i2c_client *client = data->client;
>  
> -	regmap_read(data->regmap, STK3310_REG_ID, &chipid);
> +	ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
> +	if (ret < 0)
> +		return ret;
> +
>  	if (chipid != STK3310_CHIP_ID_VAL &&
>  	    chipid != STK3311_CHIP_ID_VAL) {
>  		dev_err(&client->dev, "invalid chip id: 0x%x\n", chipid);
> @@ -608,8 +628,13 @@ static int stk3310_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (client->irq < 0)
> +	if (client->irq < 0) {
>  		client->irq = stk3310_gpio_probe(client);
> +		if (client->irq < 0) {
> +			ret = client->irq;
> +			goto err_standby;
> +		}
> +	}
>  
>  	if (client->irq >= 0) {
>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
> @@ -618,17 +643,23 @@ static int stk3310_probe(struct i2c_client *client,
>  						IRQF_TRIGGER_FALLING |
>  						IRQF_ONESHOT,
>  						STK3310_EVENT, indio_dev);
> -		if (ret < 0)
> +		if (ret < 0) {
>  			dev_err(&client->dev, "request irq %d failed\n",
>  					client->irq);
> +			goto err_standby;
> +		}
>  	}
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "device_register failed\n");
> -		stk3310_set_state(data, STK3310_STATE_STANDBY);
> +		goto err_standby;
>  	}
>  
> +	return 0;
> +
> +err_standby:
> +	stk3310_set_state(data, STK3310_STATE_STANDBY);
>  	return ret;
>  }
>  
> 

Re: [PATCH v2 4/5] iio:light:stk3310: use correct names and type for state

[Jonathan Cameron]

On 09/07/15 22:51, Hartmut Knaack wrote:
> Indicate the bit number of predefined states, make use of these names and
> change the state type in _resume to u8 to avoid type casting.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Again, needs to wait for the fixes to make their way through the trees.
> ---
>  drivers/iio/light/stk3310.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index d6df40220007..0056f4870357 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -35,8 +35,8 @@
>  #define STK3310_REG_ID				0x3E
>  #define STK3310_MAX_REG				0x80
>  
> -#define STK3310_STATE_EN_PS			0x01
> -#define STK3310_STATE_EN_ALS			0x02
> +#define STK3310_STATE_EN_PS			BIT(0)
> +#define STK3310_STATE_EN_ALS			BIT(1)
>  #define STK3310_STATE_STANDBY			0x00
>  
>  #define STK3310_CHIP_ID_VAL			0x13
> @@ -436,8 +436,8 @@ static int stk3310_set_state(struct stk3310_data *data, u8 state)
>  		dev_err(&client->dev, "failed to change sensor state\n");
>  	} else if (state != STK3310_STATE_STANDBY) {
>  		/* Don't reset the 'enabled' flags if we're going in standby */
> -		data->ps_enabled  = !!(state & 0x01);
> -		data->als_enabled = !!(state & 0x02);
> +		data->ps_enabled  = !!(state & STK3310_STATE_EN_PS);
> +		data->als_enabled = !!(state & STK3310_STATE_EN_ALS);
>  	}
>  	mutex_unlock(&data->lock);
>  
> @@ -683,7 +683,7 @@ static int stk3310_suspend(struct device *dev)
>  
>  static int stk3310_resume(struct device *dev)
>  {
> -	int state = 0;
> +	u8 state = 0;
>  	struct stk3310_data *data;
>  
>  	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> 

Re: [PATCH v2 3/5] iio:light:stk3310: add more error handling

[Jonathan Cameron]

On 19/07/15 14:57, Jonathan Cameron wrote:
> On 09/07/15 22:51, Hartmut Knaack wrote:
>> Check for the following error cases:
>>   * lower boundary for val in _write_event
>>   * return value of regmap_(field_)read
>>   * possible values for chan->type
>>   * return value of stk3310_gpio_probe
>>
>> Also add an error path in _probe to put the sensor back into stand-by mode
>> in case of serious errors.
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> looks good.  Will have to hold it until the fixes have worked there way
> through though.
Applied.
>> ---
>>  drivers/iio/light/stk3310.c | 59 ++++++++++++++++++++++++++++++++++-----------
>>  1 file changed, 45 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
>> index 11a027adc204..d6df40220007 100644
>> --- a/drivers/iio/light/stk3310.c
>> +++ b/drivers/iio/light/stk3310.c
>> @@ -241,8 +241,11 @@ static int stk3310_write_event(struct iio_dev *indio_dev,
>>  	struct stk3310_data *data = iio_priv(indio_dev);
>>  	struct i2c_client *client = data->client;
>>  
>> -	regmap_field_read(data->reg_ps_gain, &index);
>> -	if (val > stk3310_ps_max[index])
>> +	ret = regmap_field_read(data->reg_ps_gain, &index);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (val < 0 || val > stk3310_ps_max[index])
>>  		return -EINVAL;
>>  
>>  	if (dir == IIO_EV_DIR_RISING)
>> @@ -266,9 +269,12 @@ static int stk3310_read_event_config(struct iio_dev *indio_dev,
>>  				     enum iio_event_direction dir)
>>  {
>>  	unsigned int event_val;
>> +	int ret;
>>  	struct stk3310_data *data = iio_priv(indio_dev);
>>  
>> -	regmap_field_read(data->reg_int_ps, &event_val);
>> +	ret = regmap_field_read(data->reg_int_ps, &event_val);
>> +	if (ret < 0)
>> +		return ret;
>>  
>>  	return event_val;
>>  }
>> @@ -307,14 +313,16 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>>  	struct stk3310_data *data = iio_priv(indio_dev);
>>  	struct i2c_client *client = data->client;
>>  
>> +	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
>> +		return -EINVAL;
>> +
>>  	switch (mask) {
>>  	case IIO_CHAN_INFO_RAW:
>>  		if (chan->type == IIO_LIGHT)
>>  			reg = STK3310_REG_ALS_DATA_MSB;
>> -		else if (chan->type == IIO_PROXIMITY)
>> -			reg = STK3310_REG_PS_DATA_MSB;
>>  		else
>> -			return -EINVAL;
>> +			reg = STK3310_REG_PS_DATA_MSB;
>> +
>>  		mutex_lock(&data->lock);
>>  		ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
>>  		if (ret < 0) {
>> @@ -327,17 +335,23 @@ static int stk3310_read_raw(struct iio_dev *indio_dev,
>>  		return IIO_VAL_INT;
>>  	case IIO_CHAN_INFO_INT_TIME:
>>  		if (chan->type == IIO_LIGHT)
>> -			regmap_field_read(data->reg_als_it, &index);
>> +			ret = regmap_field_read(data->reg_als_it, &index);
>>  		else
>> -			regmap_field_read(data->reg_ps_it, &index);
>> +			ret = regmap_field_read(data->reg_ps_it, &index);
>> +		if (ret < 0)
>> +			return ret;
>> +
>>  		*val = stk3310_it_table[index][0];
>>  		*val2 = stk3310_it_table[index][1];
>>  		return IIO_VAL_INT_PLUS_MICRO;
>>  	case IIO_CHAN_INFO_SCALE:
>>  		if (chan->type == IIO_LIGHT)
>> -			regmap_field_read(data->reg_als_gain, &index);
>> +			ret = regmap_field_read(data->reg_als_gain, &index);
>>  		else
>> -			regmap_field_read(data->reg_ps_gain, &index);
>> +			ret = regmap_field_read(data->reg_ps_gain, &index);
>> +		if (ret < 0)
>> +			return ret;
>> +
>>  		*val = stk3310_scale_table[index][0];
>>  		*val2 = stk3310_scale_table[index][1];
>>  		return IIO_VAL_INT_PLUS_MICRO;
>> @@ -354,6 +368,9 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
>>  	int index;
>>  	struct stk3310_data *data = iio_priv(indio_dev);
>>  
>> +	if (chan->type != IIO_LIGHT && chan->type != IIO_PROXIMITY)
>> +		return -EINVAL;
>> +
>>  	switch (mask) {
>>  	case IIO_CHAN_INFO_INT_TIME:
>>  		index = stk3310_get_index(stk3310_it_table,
>> @@ -435,7 +452,10 @@ static int stk3310_init(struct iio_dev *indio_dev)
>>  	struct stk3310_data *data = iio_priv(indio_dev);
>>  	struct i2c_client *client = data->client;
>>  
>> -	regmap_read(data->regmap, STK3310_REG_ID, &chipid);
>> +	ret = regmap_read(data->regmap, STK3310_REG_ID, &chipid);
>> +	if (ret < 0)
>> +		return ret;
>> +
>>  	if (chipid != STK3310_CHIP_ID_VAL &&
>>  	    chipid != STK3311_CHIP_ID_VAL) {
>>  		dev_err(&client->dev, "invalid chip id: 0x%x\n", chipid);
>> @@ -608,8 +628,13 @@ static int stk3310_probe(struct i2c_client *client,
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	if (client->irq < 0)
>> +	if (client->irq < 0) {
>>  		client->irq = stk3310_gpio_probe(client);
>> +		if (client->irq < 0) {
>> +			ret = client->irq;
>> +			goto err_standby;
>> +		}
>> +	}
>>  
>>  	if (client->irq >= 0) {
>>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
>> @@ -618,17 +643,23 @@ static int stk3310_probe(struct i2c_client *client,
>>  						IRQF_TRIGGER_FALLING |
>>  						IRQF_ONESHOT,
>>  						STK3310_EVENT, indio_dev);
>> -		if (ret < 0)
>> +		if (ret < 0) {
>>  			dev_err(&client->dev, "request irq %d failed\n",
>>  					client->irq);
>> +			goto err_standby;
>> +		}
>>  	}
>>  
>>  	ret = iio_device_register(indio_dev);
>>  	if (ret < 0) {
>>  		dev_err(&client->dev, "device_register failed\n");
>> -		stk3310_set_state(data, STK3310_STATE_STANDBY);
>> +		goto err_standby;
>>  	}
>>  
>> +	return 0;
>> +
>> +err_standby:
>> +	stk3310_set_state(data, STK3310_STATE_STANDBY);
>>  	return ret;
>>  }
>>  
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 4/5] iio:light:stk3310: use correct names and type for state

[Jonathan Cameron]

On 19/07/15 14:58, Jonathan Cameron wrote:
> On 09/07/15 22:51, Hartmut Knaack wrote:
>> Indicate the bit number of predefined states, make use of these names and
>> change the state type in _resume to u8 to avoid type casting.
>>
>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
> Again, needs to wait for the fixes to make their way through the trees.
Applied to the togreg branch of iio.git - initially pushed out as
staging for the autobuilders to play with it.

J
>> ---
>>  drivers/iio/light/stk3310.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
>> index d6df40220007..0056f4870357 100644
>> --- a/drivers/iio/light/stk3310.c
>> +++ b/drivers/iio/light/stk3310.c
>> @@ -35,8 +35,8 @@
>>  #define STK3310_REG_ID				0x3E
>>  #define STK3310_MAX_REG				0x80
>>  
>> -#define STK3310_STATE_EN_PS			0x01
>> -#define STK3310_STATE_EN_ALS			0x02
>> +#define STK3310_STATE_EN_PS			BIT(0)
>> +#define STK3310_STATE_EN_ALS			BIT(1)
>>  #define STK3310_STATE_STANDBY			0x00
>>  
>>  #define STK3310_CHIP_ID_VAL			0x13
>> @@ -436,8 +436,8 @@ static int stk3310_set_state(struct stk3310_data *data, u8 state)
>>  		dev_err(&client->dev, "failed to change sensor state\n");
>>  	} else if (state != STK3310_STATE_STANDBY) {
>>  		/* Don't reset the 'enabled' flags if we're going in standby */
>> -		data->ps_enabled  = !!(state & 0x01);
>> -		data->als_enabled = !!(state & 0x02);
>> +		data->ps_enabled  = !!(state & STK3310_STATE_EN_PS);
>> +		data->als_enabled = !!(state & STK3310_STATE_EN_ALS);
>>  	}
>>  	mutex_unlock(&data->lock);
>>  
>> @@ -683,7 +683,7 @@ static int stk3310_suspend(struct device *dev)
>>  
>>  static int stk3310_resume(struct device *dev)
>>  {
>> -	int state = 0;
>> +	u8 state = 0;
>>  	struct stk3310_data *data;
>>  
>>  	data = iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 5/5] iio:light:stk3310: adjust indentation

[Jonathan Cameron]

On 09/07/15 22:51, Hartmut Knaack wrote:
> Adjust some indentation issues as spotted by checkpatch.pl --strict
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> Reviewed-by: Tiberiu Breana <tiberiu.a.breana@intel.com>
Applied.
> ---
>  drivers/iio/light/stk3310.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index 0056f4870357..e73f6f27cc56 100644
> --- a/drivers/iio/light/stk3310.c
> +++ b/drivers/iio/light/stk3310.c
> @@ -385,7 +385,7 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
>  			ret = regmap_field_write(data->reg_ps_it, index);
>  		if (ret < 0)
>  			dev_err(&data->client->dev,
> -					"sensor configuration failed\n");
> +				"sensor configuration failed\n");
>  		mutex_unlock(&data->lock);
>  		return ret;
>  
> @@ -402,7 +402,7 @@ static int stk3310_write_raw(struct iio_dev *indio_dev,
>  			ret = regmap_field_write(data->reg_ps_gain, index);
>  		if (ret < 0)
>  			dev_err(&data->client->dev,
> -					"sensor configuration failed\n");
> +				"sensor configuration failed\n");
>  		mutex_unlock(&data->lock);
>  		return ret;
>  	}
> @@ -645,7 +645,7 @@ static int stk3310_probe(struct i2c_client *client,
>  						STK3310_EVENT, indio_dev);
>  		if (ret < 0) {
>  			dev_err(&client->dev, "request irq %d failed\n",
> -					client->irq);
> +				client->irq);
>  			goto err_standby;
>  		}
>  	}
>