[PATCH 0/5] stk3310 fixes and cleanup

[PATCH 0/5] stk3310 fixes and cleanup

[Hartmut Knaack]

This patch series is based on top of Tiberiu Breanas patch [1]
"[PATCH v2] iio: light: STK3310: un-invert proximity values", although the
first patch fixing REGMAP_I2C dependeny can be applied independently.

[1]https://marc.info/?l=linux-iio&m=143575387110520

Hartmut Knaack (5):
  iio:light:stk3310: Fix REGMAP_I2C dependency
  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/Kconfig   |  1 +
 drivers/iio/light/stk3310.c | 88 ++++++++++++++++++++++++++++++---------------
 2 files changed, 61 insertions(+), 28 deletions(-)

-- 
2.3.6

[PATCH 1/5] iio:light:stk3310: Fix REGMAP_I2C dependency

[Hartmut Knaack]

The stk3310 driver makes use of regmap_i2c, so this dependency needs to be
reflected in Kconfig.

Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
---
 drivers/iio/light/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 730fa80..9c8b423 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -212,6 +212,7 @@ config LTR501
 config STK3310
 	tristate "STK3310 ALS and proximity sensor"
 	depends on I2C
+	select REGMAP_I2C
 	help
 	 Say yes here to get support for the Sensortek STK3310 ambient light
 	 and proximity sensor. The STK3311 model is also supported by this
-- 
2.3.6

[PATCH 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>
---
 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 c1a2182..c52c730 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.3.6

[PATCH 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-cleanup path in _probe to handle failure in
iio_device_register and devm_request_threaded_irq.

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

diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
index c52c730..3650c0c 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);
@@ -611,11 +631,14 @@ static int stk3310_probe(struct i2c_client *client,
 	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;
 	}
 
-	if (client->irq <= 0)
+	if (client->irq <= 0) {
 		client->irq = stk3310_gpio_probe(client);
+		if (client->irq < 0)
+			goto err_unregister;
+	}
 
 	if (client->irq >= 0) {
 		ret = devm_request_threaded_irq(&client->dev, client->irq,
@@ -624,11 +647,20 @@ 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_unregister;
+		}
 	}
 
+	return 0;
+
+err_unregister:
+	iio_device_unregister(indio_dev);
+err_standby:
+	stk3310_set_state(data, STK3310_STATE_STANDBY);
+
 	return ret;
 }
 
-- 
2.3.6

[PATCH 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>
---
 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 3650c0c..be715c7 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);
 
@@ -684,7 +684,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.3.6

[PATCH 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>
---
 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 be715c7..2760f67 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;
 	}
@@ -649,7 +649,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_unregister;
 		}
 	}
-- 
2.3.6

Re: [PATCH 1/5] iio:light:stk3310: Fix REGMAP_I2C dependency

[Jonathan Cameron]

On 04/07/15 16:56, Hartmut Knaack wrote:
> The stk3310 driver makes use of regmap_i2c, so this dependency needs to be
> reflected in Kconfig.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
Applied to the fixes-for-4.2 branch.  I wonder why the autobuilders didn't
pick up on this.

J
> ---
>  drivers/iio/light/Kconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 730fa80..9c8b423 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -212,6 +212,7 @@ config LTR501
>  config STK3310
>  	tristate "STK3310 ALS and proximity sensor"
>  	depends on I2C
> +	select REGMAP_I2C
>  	help
>  	 Say yes here to get support for the Sensortek STK3310 ambient light
>  	 and proximity sensor. The STK3311 model is also supported by this
> 

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

[Jonathan Cameron]

On 04/07/15 16:56, 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-cleanup path in _probe to handle failure in
> iio_device_register and devm_request_threaded_irq.
> 
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
I'm not sure how I missed this before, but the probe order
below is rather unusual.  I'm a little bothered by the theoretical
race condition between the exposure of the device to userspace
(device_register) and the configuration of it's irq later.

It is probably possible (in theory) to enable an event before the irq
is registered and get the event to fire, resulting in an unhandled interrupt.

I'm going to hold back on the rest of this set until Tiberiu has had a chance
to comment on them anyway.

Jonathan
> ---
>  drivers/iio/light/stk3310.c | 60 ++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 46 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> index c52c730..3650c0c 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);
> @@ -611,11 +631,14 @@ static int stk3310_probe(struct i2c_client *client,
>  	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;
>  	}
>  
> -	if (client->irq <= 0)
> +	if (client->irq <= 0) {
>  		client->irq = stk3310_gpio_probe(client);
> +		if (client->irq < 0)
> +			goto err_unregister;
> +	}
>  
>  	if (client->irq >= 0) {
>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
> @@ -624,11 +647,20 @@ 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_unregister;
> +		}
>  	}
>  
> +	return 0;
> +
> +err_unregister:
> +	iio_device_unregister(indio_dev);
This made me nervous, as there is a very good reason the register is almost always
last in the probe function.  It exposes the interfaces to userspace (and in kernel
for that matter).  Everything must be ready before it is called.  Here
we could (though unlikely) get an unhandled interrupt as the handler has not
been registered as yet.
> +err_standby:
> +	stk3310_set_state(data, STK3310_STATE_STANDBY);
> +
>  	return ret;
>  }
>  
> 

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

[Breana, Tiberiu A]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Saturday, July 4, 2015 6:57 PM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 2/5] iio:light:stk3310: make endianness independent of host
> 
> 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>

+1

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
> c1a2182..c52c730 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.3.6

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

[Breana, Tiberiu A]

> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@kernel.org]
> Sent: Sunday, July 5, 2015 2:25 PM
> To: Hartmut Knaack; linux-iio@vger.kernel.org
> Cc: Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
> Subject: Re: [PATCH 3/5] iio:light:stk3310: add more error handling
> 
> On 04/07/15 16:56, 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-cleanup path in _probe to handle failure in
> > iio_device_register and devm_request_threaded_irq.
> >
> > Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
> I'm not sure how I missed this before, but the probe order below is rather
> unusual.  I'm a little bothered by the theoretical race condition between the
> exposure of the device to userspace
> (device_register) and the configuration of it's irq later.
> 
> It is probably possible (in theory) to enable an event before the irq is
> registered and get the event to fire, resulting in an unhandled interrupt.
> 
> I'm going to hold back on the rest of this set until Tiberiu has had a chance to
> comment on them anyway.
> 
> Jonathan

Agreed, the probe order needs to be changed. Some comments below.

Tiberiu

> > ---
> >  drivers/iio/light/stk3310.c | 60
> > ++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 46 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c
> > index c52c730..3650c0c 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); @@
> > -611,11 +631,14 @@ static int stk3310_probe(struct i2c_client *client,
> >  	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;
> >  	}
> >
> > -	if (client->irq <= 0)
> > +	if (client->irq <= 0) {

Sort of unrelated to your patch, but I think this should be (client->irq < 0)
I would appreciate it if you would include it in your next patch set :)

> >  		client->irq = stk3310_gpio_probe(client);
> > +		if (client->irq < 0)
> > +			goto err_unregister;
> > +	}
> >
> >  	if (client->irq >= 0) {
> >  		ret = devm_request_threaded_irq(&client->dev, client->irq,
> @@
> > -624,11 +647,20 @@ 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_unregister;
> > +		}
> >  	}
> >
> > +	return 0;
> > +
> > +err_unregister:
> > +	iio_device_unregister(indio_dev);
> This made me nervous, as there is a very good reason the register is almost
> always last in the probe function.  It exposes the interfaces to userspace (and
> in kernel for that matter).  Everything must be ready before it is called.  Here
> we could (though unlikely) get an unhandled interrupt as the handler has not
> been registered as yet.

The iio_device_register block should indeed be moved to the end of _probe.

> > +err_standby:
> > +	stk3310_set_state(data, STK3310_STATE_STANDBY);
> > +
> >  	return ret;
> >  }
> >
> >

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

[Breana, Tiberiu A]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Saturday, July 4, 2015 6:57 PM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 4/5] iio:light:stk3310: use correct names and type for st=
ate
>=20
> 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.
>=20
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

+1

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

> ---
>  drivers/iio/light/stk3310.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>=20
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c in=
dex
> 3650c0c..be715c7 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
>=20
> -#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
>=20
>  #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 !=3D STK3310_STATE_STANDBY) {
>  		/* Don't reset the 'enabled' flags if we're going in standby */
> -		data->ps_enabled  =3D !!(state & 0x01);
> -		data->als_enabled =3D !!(state & 0x02);
> +		data->ps_enabled  =3D !!(state & STK3310_STATE_EN_PS);
> +		data->als_enabled =3D !!(state & STK3310_STATE_EN_ALS);
>  	}
>  	mutex_unlock(&data->lock);
>=20
> @@ -684,7 +684,7 @@ static int stk3310_suspend(struct device *dev)
>=20
>  static int stk3310_resume(struct device *dev)  {
> -	int state =3D 0;
> +	u8 state =3D 0;
>  	struct stk3310_data *data;
>=20
>  	data =3D iio_priv(i2c_get_clientdata(to_i2c_client(dev)));
> --
> 2.3.6

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

[Breana, Tiberiu A]

> -----Original Message-----
> From: Hartmut Knaack [mailto:knaack.h@gmx.de]
> Sent: Saturday, July 4, 2015 6:57 PM
> To: linux-iio@vger.kernel.org
> Cc: Jonathan Cameron; Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu
> A
> Subject: [PATCH 5/5] iio:light:stk3310: adjust indentation
>=20
> Adjust some indentation issues as spotted by checkpatch.pl --strict
>=20
> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>

+1

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

> ---
>  drivers/iio/light/stk3310.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>=20
> diff --git a/drivers/iio/light/stk3310.c b/drivers/iio/light/stk3310.c in=
dex
> be715c7..2760f67 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 =3D 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;
>=20
> @@ -402,7 +402,7 @@ static int stk3310_write_raw(struct iio_dev
> *indio_dev,
>  			ret =3D 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;
>  	}
> @@ -649,7 +649,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_unregister;
>  		}
>  	}
> --
> 2.3.6

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

[Hartmut Knaack]

Breana, Tiberiu A schrieb am 06.07.2015 um 11:30:
>> -----Original Message-----
>> From: Jonathan Cameron [mailto:jic23@kernel.org]
>> Sent: Sunday, July 5, 2015 2:25 PM
>> To: Hartmut Knaack; linux-iio@vger.kernel.org
>> Cc: Lars-Peter Clausen; Peter Meerwald; Breana, Tiberiu A
>> Subject: Re: [PATCH 3/5] iio:light:stk3310: add more error handling
>>
>> On 04/07/15 16:56, 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-cleanup path in _probe to handle failure in
>>> iio_device_register and devm_request_threaded_irq.
>>>
>>> Signed-off-by: Hartmut Knaack <knaack.h@gmx.de>
>> I'm not sure how I missed this before, but the probe order below is rather
>> unusual.  I'm a little bothered by the theoretical race condition between the
>> exposure of the device to userspace
>> (device_register) and the configuration of it's irq later.
>>
>> It is probably possible (in theory) to enable an event before the irq is
>> registered and get the event to fire, resulting in an unhandled interrupt.
>>
>> I'm going to hold back on the rest of this set until Tiberiu has had a chance to
>> comment on them anyway.
>>
>> Jonathan
> 
> Agreed, the probe order needs to be changed. Some comments below.
> 
> Tiberiu
> 
<...>
>>> -611,11 +631,14 @@ static int stk3310_probe(struct i2c_client *client,
>>>  	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;
>>>  	}
>>>
>>> -	if (client->irq <= 0)
>>> +	if (client->irq <= 0) {
> 
> Sort of unrelated to your patch, but I think this should be (client->irq < 0)
> I would appreciate it if you would include it in your next patch set :)
> 

No problem, this will be included in my new revision.

>>>  		client->irq = stk3310_gpio_probe(client);
>>> +		if (client->irq < 0)
>>> +			goto err_unregister;
>>> +	}
>>>
>>>  	if (client->irq >= 0) {
>>>  		ret = devm_request_threaded_irq(&client->dev, client->irq,
>> @@
>>> -624,11 +647,20 @@ 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_unregister;
>>> +		}
>>>  	}
>>>
>>> +	return 0;
>>> +
>>> +err_unregister:
>>> +	iio_device_unregister(indio_dev);
>> This made me nervous, as there is a very good reason the register is almost
>> always last in the probe function.  It exposes the interfaces to userspace (and
>> in kernel for that matter).  Everything must be ready before it is called.  Here
>> we could (though unlikely) get an unhandled interrupt as the handler has not
>> been registered as yet.
> 
> The iio_device_register block should indeed be moved to the end of _probe.
> 
>>> +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
>