[PATCH 1/7] iio: light: tsl2583: return proper error code

[PATCH 1/7] iio: light: tsl2583: return proper error code

[Brian Masney]

taos_gain_store() and taos_als_calibrate() both have a code path where
-1 was returned. This patch changes the code so that a proper error code
is returned to make the code consistent with the error paths that are
present within those same functions.

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

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 08f1583..1e42a19 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -359,7 +359,7 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
 			!= (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 -1;
+		return -ENODATA;
 	}
 
 	ret = i2c_smbus_write_byte(chip->client,
@@ -569,7 +569,7 @@ static ssize_t taos_gain_store(struct device *dev,
 		break;
 	default:
 		dev_err(dev, "Invalid Gain Index (must be 1,8,16,111)\n");
-		return -1;
+		return -EINVAL;
 	}
 
 	return len;
-- 
2.7.4

[PATCH 2/7] iio: light: tsl2583: change functions to only have a single exit point

[Brian Masney]

Change the following functions to only have a single exit point:
taos_i2c_read(), taos_als_calibrate(), taos_chip_on(),
taos_gain_store(), taos_gain_available_show(), taos_luxtable_store()
and taos_probe().

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

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 1e42a19..8448a87 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -159,7 +159,7 @@ static void taos_defaults(struct tsl2583_chip *chip)
 static int
 taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
 {
-	int i, ret;
+	int i, ret = 0;
 
 	for (i = 0; i < len; i++) {
 		/* select register to write */
@@ -168,14 +168,16 @@ taos_i2c_read(struct i2c_client *client, u8 reg, u8 *val, unsigned int len)
 			dev_err(&client->dev,
 				"taos_i2c_read failed to write register %x\n",
 				reg);
-			return ret;
+			goto read_done;
 		}
 		/* read the data */
 		*val = i2c_smbus_read_byte(client);
 		val++;
 		reg++;
 	}
-	return 0;
+
+read_done:
+	return ret;
 }
 
 /*
@@ -351,7 +353,7 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
 		dev_err(&chip->client->dev,
 			"taos_als_calibrate failed to reach the CNTRL register, ret=%d\n",
 			ret);
-		return ret;
+		goto calibrate_done;
 	}
 
 	reg_val = i2c_smbus_read_byte(chip->client);
@@ -359,7 +361,8 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
 			!= (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 -ENODATA;
+		ret = -ENODATA;
+		goto calibrate_done;
 	}
 
 	ret = i2c_smbus_write_byte(chip->client,
@@ -368,19 +371,21 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
 		dev_err(&chip->client->dev,
 			"taos_als_calibrate failed to reach the STATUS register, ret=%d\n",
 			ret);
-		return ret;
+		goto calibrate_done;
 	}
 	reg_val = i2c_smbus_read_byte(chip->client);
 
 	if ((reg_val & TSL258X_STA_ADC_VALID) != TSL258X_STA_ADC_VALID) {
 		dev_err(&chip->client->dev,
 			"taos_als_calibrate failed: STATUS - ADC not valid.\n");
-		return -ENODATA;
+		ret = -ENODATA;
+		goto calibrate_done;
 	}
 	lux_val = taos_get_lux(indio_dev);
 	if (lux_val < 0) {
 		dev_err(&chip->client->dev, "taos_als_calibrate failed to get lux\n");
-		return lux_val;
+		ret = lux_val;
+		goto calibrate_done;
 	}
 	gain_trim_val = (unsigned int)(((chip->taos_settings.als_cal_target)
 			* chip->taos_settings.als_gain_trim) / lux_val);
@@ -389,11 +394,15 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
 		dev_err(&chip->client->dev,
 			"taos_als_calibrate failed: trim_val of %d is out of range\n",
 			gain_trim_val);
-		return -ENODATA;
+		ret = -ENODATA;
+		goto calibrate_done;
 	}
 	chip->taos_settings.als_gain_trim = (int)gain_trim_val;
+	ret = gain_trim_val;
+
+calibrate_done:
+	return ret;
 
-	return (int)gain_trim_val;
 }
 
 /*
@@ -403,7 +412,7 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
 static int taos_chip_on(struct iio_dev *indio_dev)
 {
 	int i;
-	int ret;
+	int ret = -EINVAL;
 	u8 *uP;
 	u8 utmp;
 	int als_count;
@@ -414,7 +423,7 @@ static int taos_chip_on(struct iio_dev *indio_dev)
 	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;
+		goto chip_on_done;
 	}
 
 	/* determine als integration register */
@@ -442,7 +451,7 @@ static int taos_chip_on(struct iio_dev *indio_dev)
 					TSL258X_CMD_REG | TSL258X_CNTRL, utmp);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "taos_chip_on failed on CNTRL reg.\n");
-		return ret;
+		goto chip_on_done;
 	}
 
 	/*
@@ -456,7 +465,7 @@ static int taos_chip_on(struct iio_dev *indio_dev)
 		if (ret < 0) {
 			dev_err(&chip->client->dev,
 				"taos_chip_on failed on reg %d.\n", i);
-			return ret;
+			goto chip_on_done;
 		}
 	}
 
@@ -471,10 +480,11 @@ static int taos_chip_on(struct iio_dev *indio_dev)
 					utmp);
 	if (ret < 0) {
 		dev_err(&chip->client->dev, "taos_chip_on failed on 2nd CTRL reg.\n");
-		return ret;
+		goto chip_on_done;
 	}
 	chip->taos_chip_status = TSL258X_CHIP_WORKING;
 
+chip_on_done:
 	return ret;
 }
 
@@ -549,10 +559,10 @@ static ssize_t taos_gain_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 = -EINVAL;
 
 	if (kstrtoint(buf, 0, &value))
-		return -EINVAL;
+		goto gain_store_done;
 
 	switch (value) {
 	case 1:
@@ -569,10 +579,12 @@ static ssize_t taos_gain_store(struct device *dev,
 		break;
 	default:
 		dev_err(dev, "Invalid Gain Index (must be 1,8,16,111)\n");
-		return -EINVAL;
+		goto gain_store_done;
 	}
+	ret = len;
 
-	return len;
+gain_store_done:
+	return ret;
 }
 
 static ssize_t taos_gain_available_show(struct device *dev,
@@ -597,20 +609,22 @@ static ssize_t taos_als_time_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 = -EINVAL;
 
 	if (kstrtoint(buf, 0, &value))
-		return -EINVAL;
+		goto als_time_store_done;
 
 	if ((value < 50) || (value > 650))
-		return -EINVAL;
+		goto als_time_store_done;
 
 	if (value % 50)
-		return -EINVAL;
+		goto als_time_store_done;
 
 	chip->taos_settings.als_time = value;
+	ret = len;
 
-	return len;
+als_time_store_done:
+	return ret;
 }
 
 static ssize_t taos_als_time_available_show(struct device *dev,
@@ -734,7 +748,7 @@ static ssize_t taos_luxtable_store(struct device *dev,
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
 	int value[ARRAY_SIZE(taos_device_lux) * 3 + 1];
-	int n;
+	int n, ret = -EINVAL;
 
 	get_options(buf, ARRAY_SIZE(value), value);
 
@@ -746,11 +760,11 @@ static ssize_t taos_luxtable_store(struct device *dev,
 	n = value[0];
 	if ((n % 3) || n < 6 || n > ((ARRAY_SIZE(taos_device_lux) - 1) * 3)) {
 		dev_info(dev, "LUX TABLE INPUT ERROR 1 Value[0]=%d\n", n);
-		return -EINVAL;
+		goto luxable_store_done;
 	}
 	if ((value[(n - 2)] | value[(n - 1)] | value[n]) != 0) {
 		dev_info(dev, "LUX TABLE INPUT ERROR 2 Value[0]=%d\n", n);
-		return -EINVAL;
+		goto luxable_store_done;
 	}
 
 	if (chip->taos_chip_status == TSL258X_CHIP_WORKING)
@@ -761,8 +775,10 @@ static ssize_t taos_luxtable_store(struct device *dev,
 	memcpy(taos_device_lux, &value[1], (value[0] * 4));
 
 	taos_chip_on(indio_dev);
+	ret = len;
 
-	return len;
+luxable_store_done:
+	return ret;
 }
 
 static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
@@ -833,12 +849,15 @@ static int taos_probe(struct i2c_client *clientp,
 	if (!i2c_check_functionality(clientp->adapter,
 				     I2C_FUNC_SMBUS_BYTE_DATA)) {
 		dev_err(&clientp->dev, "taos_probe() - i2c smbus byte data func unsupported\n");
-		return -EOPNOTSUPP;
+		ret = -EOPNOTSUPP;
+		goto probe_done;
 	}
 
 	indio_dev = devm_iio_device_alloc(&clientp->dev, sizeof(*chip));
-	if (!indio_dev)
-		return -ENOMEM;
+	if (!indio_dev) {
+		ret = -ENOMEM;
+		goto probe_done;
+	}
 	chip = iio_priv(indio_dev);
 	chip->client = clientp;
 	i2c_set_clientdata(clientp, indio_dev);
@@ -854,14 +873,14 @@ static int taos_probe(struct i2c_client *clientp,
 			dev_err(&clientp->dev,
 				"i2c_smbus_write_byte to cmd reg failed in taos_probe(), err = %d\n",
 				ret);
-			return ret;
+			goto probe_done;
 		}
 		ret = i2c_smbus_read_byte(clientp);
 		if (ret < 0) {
 			dev_err(&clientp->dev,
 				"i2c_smbus_read_byte from reg failed in taos_probe(), err = %d\n",
 				ret);
-			return ret;
+			goto probe_done;
 		}
 		buf[i] = ret;
 	}
@@ -869,7 +888,8 @@ static int taos_probe(struct i2c_client *clientp,
 	if (!taos_tsl258x_device(buf)) {
 		dev_info(&clientp->dev,
 			 "i2c device found but does not match expected id in taos_probe()\n");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto probe_done;
 	}
 
 	ret = i2c_smbus_write_byte(clientp, (TSL258X_CMD_REG | TSL258X_CNTRL));
@@ -877,7 +897,7 @@ static int taos_probe(struct i2c_client *clientp,
 		dev_err(&clientp->dev,
 			"i2c_smbus_write_byte() to cmd reg failed in taos_probe(), err = %d\n",
 			ret);
-		return ret;
+		goto probe_done;
 	}
 
 	indio_dev->info = &tsl2583_info;
@@ -887,7 +907,7 @@ static int taos_probe(struct i2c_client *clientp,
 	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 	if (ret) {
 		dev_err(&clientp->dev, "iio registration failed\n");
-		return ret;
+		goto probe_done;
 	}
 
 	/* Load up the V2 defaults (these are hard coded defaults for now) */
@@ -897,7 +917,9 @@ static int taos_probe(struct i2c_client *clientp,
 	taos_chip_on(indio_dev);
 
 	dev_info(&clientp->dev, "Light sensor found.\n");
-	return 0;
+
+probe_done:
+	return ret;
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.7.4

Re: [PATCH 2/7] iio: light: tsl2583: change functions to only have a single exit point

[Dan Carpenter]

On Wed, Oct 19, 2016 at 06:32:05AM -0400, Brian Masney wrote:
> Change the following functions to only have a single exit point:
> taos_i2c_read(), taos_als_calibrate(), taos_chip_on(),
> taos_gain_store(), taos_gain_available_show(), taos_luxtable_store()
> and taos_probe().
> 

What's the point of this?  This style of code just makes things more
complicated and leads to "forgot the error code" bugs.  People think
that it future proofs the code in case we add locking but I have looked
into this and it has minimal if any impact at preventing locking bugs.

regards,
dan carpenter

Re: [PATCH 2/7] iio: light: tsl2583: change functions to only have a single exit point

[Brian Masney]

On Wed, Oct 19, 2016 at 02:08:59PM +0300, Dan Carpenter wrote:
> On Wed, Oct 19, 2016 at 06:32:05AM -0400, Brian Masney wrote:
> > Change the following functions to only have a single exit point:
> > taos_i2c_read(), taos_als_calibrate(), taos_chip_on(),
> > taos_gain_store(), taos_gain_available_show(), taos_luxtable_store()
> > and taos_probe().
> > 
> 
> What's the point of this?  This style of code just makes things more
> complicated and leads to "forgot the error code" bugs.  People think
> that it future proofs the code in case we add locking but I have looked
> into this and it has minimal if any impact at preventing locking bugs.

The reason that I did this was due to the locking that I added later in
the patch series. Each function would only have a single call to
mutex_unlock(). I should have mentioned that in my message.

Brian

Re: [PATCH 2/7] iio: light: tsl2583: change functions to only have a single exit point

[Dan Carpenter]

On Wed, Oct 19, 2016 at 08:38:16AM -0400, Brian Masney wrote:
> On Wed, Oct 19, 2016 at 02:08:59PM +0300, Dan Carpenter wrote:
> > On Wed, Oct 19, 2016 at 06:32:05AM -0400, Brian Masney wrote:
> > > Change the following functions to only have a single exit point:
> > > taos_i2c_read(), taos_als_calibrate(), taos_chip_on(),
> > > taos_gain_store(), taos_gain_available_show(), taos_luxtable_store()
> > > and taos_probe().
> > > 
> > 
> > What's the point of this?  This style of code just makes things more
> > complicated and leads to "forgot the error code" bugs.  People think
> > that it future proofs the code in case we add locking but I have looked
> > into this and it has minimal if any impact at preventing locking bugs.
> 
> The reason that I did this was due to the locking that I added later in
> the patch series. Each function would only have a single call to
> mutex_unlock(). I should have mentioned that in my message.

This kind of trick does not work in real life.  I have looked at it.
In theory, it should work but in real life, empirically, it does not
and it introduces additional new bugs.

People are always looking for a magic bullet but there is no such thing.
The best approach is just to write the simplest, clearest code that you
can.

regards,
dan carpenter

[PATCH 3/7] iio: light: tsl2583: use DEVICE_ATTR_{RO, RW, WO} macros

[Brian Masney]

Use the DEVICE_ATTR_RO, IIO_DEVICE_ATTR_RW, and IIO_DEVICE_ATTR_WO
macros to create the device attributes.

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

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 8448a87..bbb8fc3 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -501,8 +501,8 @@ static int taos_chip_off(struct iio_dev *indio_dev)
 
 /* Sysfs Interface Functions */
 
-static ssize_t taos_power_state_show(struct device *dev,
-				     struct device_attribute *attr, char *buf)
+static ssize_t power_state_show(struct device *dev,
+				struct device_attribute *attr, char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -510,9 +510,9 @@ static ssize_t taos_power_state_show(struct device *dev,
 	return sprintf(buf, "%d\n", chip->taos_chip_status);
 }
 
-static ssize_t taos_power_state_store(struct device *dev,
-				      struct device_attribute *attr,
-				      const char *buf, size_t len)
+static ssize_t power_state_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	int value;
@@ -528,8 +528,9 @@ static ssize_t taos_power_state_store(struct device *dev,
 	return len;
 }
 
-static ssize_t taos_gain_show(struct device *dev,
-			      struct device_attribute *attr, char *buf)
+static ssize_t illuminance0_calibscale_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -553,9 +554,9 @@ static ssize_t taos_gain_show(struct device *dev,
 	return sprintf(buf, "%s\n", gain);
 }
 
-static ssize_t taos_gain_store(struct device *dev,
-			       struct device_attribute *attr,
-			       const char *buf, size_t len)
+static ssize_t illuminance0_calibscale_store(struct device *dev,
+					     struct device_attribute *attr,
+					     const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -587,15 +588,16 @@ gain_store_done:
 	return ret;
 }
 
-static ssize_t taos_gain_available_show(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
+static ssize_t illuminance0_calibscale_available_show(struct device *dev,
+						  struct device_attribute *attr,
+						  char *buf)
 {
 	return sprintf(buf, "%s\n", "1 8 16 111");
 }
 
-static ssize_t taos_als_time_show(struct device *dev,
-				  struct device_attribute *attr, char *buf)
+static ssize_t illuminance0_integration_time_show(struct device *dev,
+						  struct device_attribute *attr,
+						  char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -603,9 +605,9 @@ static ssize_t taos_als_time_show(struct device *dev,
 	return sprintf(buf, "%d\n", chip->taos_settings.als_time);
 }
 
-static ssize_t taos_als_time_store(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t len)
+static ssize_t illuminance0_integration_time_store(struct device *dev,
+						  struct device_attribute *attr,
+						  const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -627,16 +629,17 @@ als_time_store_done:
 	return ret;
 }
 
-static ssize_t taos_als_time_available_show(struct device *dev,
-					    struct device_attribute *attr,
-					    char *buf)
+static ssize_t illuminance0_integration_time_available_show(struct device *dev,
+						  struct device_attribute *attr,
+						  char *buf)
 {
 	return sprintf(buf, "%s\n",
 		"50 100 150 200 250 300 350 400 450 500 550 600 650");
 }
 
-static ssize_t taos_als_trim_show(struct device *dev,
-				  struct device_attribute *attr, char *buf)
+static ssize_t illuminance0_calibbias_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -644,9 +647,9 @@ static ssize_t taos_als_trim_show(struct device *dev,
 	return sprintf(buf, "%d\n", chip->taos_settings.als_gain_trim);
 }
 
-static ssize_t taos_als_trim_store(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t len)
+static ssize_t illuminance0_calibbias_store(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -661,9 +664,9 @@ static ssize_t taos_als_trim_store(struct device *dev,
 	return len;
 }
 
-static ssize_t taos_als_cal_target_show(struct device *dev,
-					struct device_attribute *attr,
-					char *buf)
+static ssize_t illuminance0_input_target_show(struct device *dev,
+					      struct device_attribute *attr,
+					      char *buf)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -671,9 +674,9 @@ static ssize_t taos_als_cal_target_show(struct device *dev,
 	return sprintf(buf, "%d\n", chip->taos_settings.als_cal_target);
 }
 
-static ssize_t taos_als_cal_target_store(struct device *dev,
-					 struct device_attribute *attr,
-					 const char *buf, size_t len)
+static ssize_t illuminance0_input_target_store(struct device *dev,
+					       struct device_attribute *attr,
+					       const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -688,8 +691,9 @@ static ssize_t taos_als_cal_target_store(struct device *dev,
 	return len;
 }
 
-static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
-			     char *buf)
+static ssize_t illuminance0_input_show(struct device *dev,
+				       struct device_attribute *attr,
+				       char *buf)
 {
 	int ret;
 
@@ -700,9 +704,9 @@ static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
 	return sprintf(buf, "%d\n", ret);
 }
 
-static ssize_t taos_do_calibrate(struct device *dev,
-				 struct device_attribute *attr,
-				 const char *buf, size_t len)
+static ssize_t illuminance0_calibrate_store(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	int value;
@@ -716,8 +720,9 @@ static ssize_t taos_do_calibrate(struct device *dev,
 	return len;
 }
 
-static ssize_t taos_luxtable_show(struct device *dev,
-				  struct device_attribute *attr, char *buf)
+static ssize_t illuminance0_lux_table_show(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
 {
 	int i;
 	int offset = 0;
@@ -741,9 +746,9 @@ static ssize_t taos_luxtable_show(struct device *dev,
 	return offset;
 }
 
-static ssize_t taos_luxtable_store(struct device *dev,
-				   struct device_attribute *attr,
-				   const char *buf, size_t len)
+static ssize_t illuminance0_lux_table_store(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
 	struct tsl2583_chip *chip = iio_priv(indio_dev);
@@ -781,29 +786,21 @@ luxable_store_done:
 	return ret;
 }
 
-static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
-		taos_power_state_show, taos_power_state_store);
+static DEVICE_ATTR_RW(power_state);
 
-static DEVICE_ATTR(illuminance0_calibscale, S_IRUGO | S_IWUSR,
-		taos_gain_show, taos_gain_store);
-static DEVICE_ATTR(illuminance0_calibscale_available, S_IRUGO,
-		taos_gain_available_show, NULL);
+static DEVICE_ATTR_RW(illuminance0_calibscale);
+static DEVICE_ATTR_RO(illuminance0_calibscale_available);
 
-static DEVICE_ATTR(illuminance0_integration_time, S_IRUGO | S_IWUSR,
-		taos_als_time_show, taos_als_time_store);
-static DEVICE_ATTR(illuminance0_integration_time_available, S_IRUGO,
-		taos_als_time_available_show, NULL);
+static DEVICE_ATTR_RW(illuminance0_integration_time);
+static DEVICE_ATTR_RO(illuminance0_integration_time_available);
 
-static DEVICE_ATTR(illuminance0_calibbias, S_IRUGO | S_IWUSR,
-		taos_als_trim_show, taos_als_trim_store);
+static DEVICE_ATTR_RW(illuminance0_calibbias);
 
-static DEVICE_ATTR(illuminance0_input_target, S_IRUGO | S_IWUSR,
-		taos_als_cal_target_show, taos_als_cal_target_store);
+static DEVICE_ATTR_RW(illuminance0_input_target);
 
-static DEVICE_ATTR(illuminance0_input, S_IRUGO, taos_lux_show, NULL);
-static DEVICE_ATTR(illuminance0_calibrate, S_IWUSR, NULL, taos_do_calibrate);
-static DEVICE_ATTR(illuminance0_lux_table, S_IRUGO | S_IWUSR,
-		taos_luxtable_show, taos_luxtable_store);
+static DEVICE_ATTR_RO(illuminance0_input);
+static DEVICE_ATTR_WO(illuminance0_calibrate);
+static DEVICE_ATTR_RW(illuminance0_lux_table);
 
 static struct attribute *sysfs_attrs_ctrl[] = {
 	&dev_attr_power_state.attr,
-- 
2.7.4

Re: [PATCH 3/7] iio: light: tsl2583: use DEVICE_ATTR_{RO, RW, WO} macros

[Peter Meerwald-Stadler]

> Use the DEVICE_ATTR_RO, IIO_DEVICE_ATTR_RW, and IIO_DEVICE_ATTR_WO
> macros to create the device attributes.

great that you work on cleaning this up!

the patch does a bit more than is claimed, it also renames stuff; 
eventually, a proper prefix ('tsl2583_') should be used...


maybe you can unify the two files in this patch series?
./staging/iio/Documentation/sysfs-bus-iio-light-tsl2583
./staging/iio/Documentation/light/sysfs-bus-iio-light-tsl2583

Suggested-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
:-)


> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/staging/iio/light/tsl2583.c | 117 ++++++++++++++++++------------------
>  1 file changed, 57 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 8448a87..bbb8fc3 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -501,8 +501,8 @@ static int taos_chip_off(struct iio_dev *indio_dev)
>  
>  /* Sysfs Interface Functions */
>  
> -static ssize_t taos_power_state_show(struct device *dev,
> -				     struct device_attribute *attr, char *buf)
> +static ssize_t power_state_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -510,9 +510,9 @@ static ssize_t taos_power_state_show(struct device *dev,
>  	return sprintf(buf, "%d\n", chip->taos_chip_status);
>  }
>  
> -static ssize_t taos_power_state_store(struct device *dev,
> -				      struct device_attribute *attr,
> -				      const char *buf, size_t len)
> +static ssize_t power_state_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	int value;
> @@ -528,8 +528,9 @@ static ssize_t taos_power_state_store(struct device *dev,
>  	return len;
>  }
>  
> -static ssize_t taos_gain_show(struct device *dev,
> -			      struct device_attribute *attr, char *buf)
> +static ssize_t illuminance0_calibscale_show(struct device *dev,
> +					    struct device_attribute *attr,
> +					    char *buf)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -553,9 +554,9 @@ static ssize_t taos_gain_show(struct device *dev,
>  	return sprintf(buf, "%s\n", gain);
>  }
>  
> -static ssize_t taos_gain_store(struct device *dev,
> -			       struct device_attribute *attr,
> -			       const char *buf, size_t len)
> +static ssize_t illuminance0_calibscale_store(struct device *dev,
> +					     struct device_attribute *attr,
> +					     const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -587,15 +588,16 @@ gain_store_done:
>  	return ret;
>  }
>  
> -static ssize_t taos_gain_available_show(struct device *dev,
> -					struct device_attribute *attr,
> -					char *buf)
> +static ssize_t illuminance0_calibscale_available_show(struct device *dev,
> +						  struct device_attribute *attr,
> +						  char *buf)
>  {
>  	return sprintf(buf, "%s\n", "1 8 16 111");
>  }
>  
> -static ssize_t taos_als_time_show(struct device *dev,
> -				  struct device_attribute *attr, char *buf)
> +static ssize_t illuminance0_integration_time_show(struct device *dev,
> +						  struct device_attribute *attr,
> +						  char *buf)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -603,9 +605,9 @@ static ssize_t taos_als_time_show(struct device *dev,
>  	return sprintf(buf, "%d\n", chip->taos_settings.als_time);
>  }
>  
> -static ssize_t taos_als_time_store(struct device *dev,
> -				   struct device_attribute *attr,
> -				   const char *buf, size_t len)
> +static ssize_t illuminance0_integration_time_store(struct device *dev,
> +						  struct device_attribute *attr,
> +						  const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -627,16 +629,17 @@ als_time_store_done:
>  	return ret;
>  }
>  
> -static ssize_t taos_als_time_available_show(struct device *dev,
> -					    struct device_attribute *attr,
> -					    char *buf)
> +static ssize_t illuminance0_integration_time_available_show(struct device *dev,
> +						  struct device_attribute *attr,
> +						  char *buf)
>  {
>  	return sprintf(buf, "%s\n",
>  		"50 100 150 200 250 300 350 400 450 500 550 600 650");
>  }
>  
> -static ssize_t taos_als_trim_show(struct device *dev,
> -				  struct device_attribute *attr, char *buf)
> +static ssize_t illuminance0_calibbias_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -644,9 +647,9 @@ static ssize_t taos_als_trim_show(struct device *dev,
>  	return sprintf(buf, "%d\n", chip->taos_settings.als_gain_trim);
>  }
>  
> -static ssize_t taos_als_trim_store(struct device *dev,
> -				   struct device_attribute *attr,
> -				   const char *buf, size_t len)
> +static ssize_t illuminance0_calibbias_store(struct device *dev,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -661,9 +664,9 @@ static ssize_t taos_als_trim_store(struct device *dev,
>  	return len;
>  }
>  
> -static ssize_t taos_als_cal_target_show(struct device *dev,
> -					struct device_attribute *attr,
> -					char *buf)
> +static ssize_t illuminance0_input_target_show(struct device *dev,
> +					      struct device_attribute *attr,
> +					      char *buf)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -671,9 +674,9 @@ static ssize_t taos_als_cal_target_show(struct device *dev,
>  	return sprintf(buf, "%d\n", chip->taos_settings.als_cal_target);
>  }
>  
> -static ssize_t taos_als_cal_target_store(struct device *dev,
> -					 struct device_attribute *attr,
> -					 const char *buf, size_t len)
> +static ssize_t illuminance0_input_target_store(struct device *dev,
> +					       struct device_attribute *attr,
> +					       const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -688,8 +691,9 @@ static ssize_t taos_als_cal_target_store(struct device *dev,
>  	return len;
>  }
>  
> -static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
> -			     char *buf)
> +static ssize_t illuminance0_input_show(struct device *dev,
> +				       struct device_attribute *attr,
> +				       char *buf)
>  {
>  	int ret;
>  
> @@ -700,9 +704,9 @@ static ssize_t taos_lux_show(struct device *dev, struct device_attribute *attr,
>  	return sprintf(buf, "%d\n", ret);
>  }
>  
> -static ssize_t taos_do_calibrate(struct device *dev,
> -				 struct device_attribute *attr,
> -				 const char *buf, size_t len)
> +static ssize_t illuminance0_calibrate_store(struct device *dev,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	int value;
> @@ -716,8 +720,9 @@ static ssize_t taos_do_calibrate(struct device *dev,
>  	return len;
>  }
>  
> -static ssize_t taos_luxtable_show(struct device *dev,
> -				  struct device_attribute *attr, char *buf)
> +static ssize_t illuminance0_lux_table_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
>  {
>  	int i;
>  	int offset = 0;
> @@ -741,9 +746,9 @@ static ssize_t taos_luxtable_show(struct device *dev,
>  	return offset;
>  }
>  
> -static ssize_t taos_luxtable_store(struct device *dev,
> -				   struct device_attribute *attr,
> -				   const char *buf, size_t len)
> +static ssize_t illuminance0_lux_table_store(struct device *dev,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
>  {
>  	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>  	struct tsl2583_chip *chip = iio_priv(indio_dev);
> @@ -781,29 +786,21 @@ luxable_store_done:
>  	return ret;
>  }
>  
> -static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
> -		taos_power_state_show, taos_power_state_store);
> +static DEVICE_ATTR_RW(power_state);
>  
> -static DEVICE_ATTR(illuminance0_calibscale, S_IRUGO | S_IWUSR,
> -		taos_gain_show, taos_gain_store);
> -static DEVICE_ATTR(illuminance0_calibscale_available, S_IRUGO,
> -		taos_gain_available_show, NULL);
> +static DEVICE_ATTR_RW(illuminance0_calibscale);
> +static DEVICE_ATTR_RO(illuminance0_calibscale_available);
>  
> -static DEVICE_ATTR(illuminance0_integration_time, S_IRUGO | S_IWUSR,
> -		taos_als_time_show, taos_als_time_store);
> -static DEVICE_ATTR(illuminance0_integration_time_available, S_IRUGO,
> -		taos_als_time_available_show, NULL);
> +static DEVICE_ATTR_RW(illuminance0_integration_time);
> +static DEVICE_ATTR_RO(illuminance0_integration_time_available);
>  
> -static DEVICE_ATTR(illuminance0_calibbias, S_IRUGO | S_IWUSR,
> -		taos_als_trim_show, taos_als_trim_store);
> +static DEVICE_ATTR_RW(illuminance0_calibbias);
>  
> -static DEVICE_ATTR(illuminance0_input_target, S_IRUGO | S_IWUSR,
> -		taos_als_cal_target_show, taos_als_cal_target_store);
> +static DEVICE_ATTR_RW(illuminance0_input_target);
>  
> -static DEVICE_ATTR(illuminance0_input, S_IRUGO, taos_lux_show, NULL);
> -static DEVICE_ATTR(illuminance0_calibrate, S_IWUSR, NULL, taos_do_calibrate);
> -static DEVICE_ATTR(illuminance0_lux_table, S_IRUGO | S_IWUSR,
> -		taos_luxtable_show, taos_luxtable_store);
> +static DEVICE_ATTR_RO(illuminance0_input);
> +static DEVICE_ATTR_WO(illuminance0_calibrate);
> +static DEVICE_ATTR_RW(illuminance0_lux_table);
>  
>  static struct attribute *sysfs_attrs_ctrl[] = {
>  	&dev_attr_power_state.attr,
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

Re: [PATCH 3/7] iio: light: tsl2583: use DEVICE_ATTR_{RO, RW, WO} macros

[Brian Masney]

On Wed, Oct 19, 2016 at 12:47:13PM +0200, Peter Meerwald-Stadler wrote:
> 
> > Use the DEVICE_ATTR_RO, IIO_DEVICE_ATTR_RW, and IIO_DEVICE_ATTR_WO
> > macros to create the device attributes.
> 
> great that you work on cleaning this up!
> 
> the patch does a bit more than is claimed, it also renames stuff; 
> eventually, a proper prefix ('tsl2583_') should be used...

We can't have the tsl2583_ prefix on the function with the
DEVICE_ATTR_{RW,RO,WO} macros. drivers/acpi/nfit/core.c is one of many
examples of this.

I will submit a separate patch to fix the prefix on the other functions
when I get to code style cleanups in that driver.

> maybe you can unify the two files in this patch series?
> ./staging/iio/Documentation/sysfs-bus-iio-light-tsl2583
> ./staging/iio/Documentation/light/sysfs-bus-iio-light-tsl2583
> 
> Suggested-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> :-)

I'll add that to my list.

There are plenty of other issues to fix in this driver and I'll have
more cleanup patches. I was at a good stopping point and wanted to send
out what I had so far to get feedback. I should have let the patch series
sit for a day before I sent it out.

Brian

Re: [PATCH 3/7] iio: light: tsl2583: use DEVICE_ATTR_{RO, RW, WO} macros

[Dan Carpenter]

What does illuminance0_ mean?  Can we remove that?

regards,
dan carpenter

Re: [PATCH 3/7] iio: light: tsl2583: use DEVICE_ATTR_{RO, RW, WO} macros

[Brian Masney]

On Wed, Oct 19, 2016 at 02:26:27PM +0300, Dan Carpenter wrote:
> What does illuminance0_ mean?  Can we remove that?

I left the names of the existing sysfs attributes intact to not break
any existing users of the driver. I'm not sure why the original author
named it that way.

Brian

Re: [PATCH 3/7] iio: light: tsl2583: use DEVICE_ATTR_{RO, RW, WO} macros

[Dan Carpenter]

On Wed, Oct 19, 2016 at 09:08:30AM -0400, Brian Masney wrote:
> On Wed, Oct 19, 2016 at 02:26:27PM +0300, Dan Carpenter wrote:
> > What does illuminance0_ mean?  Can we remove that?
> 
> I left the names of the existing sysfs attributes intact to not break
> any existing users of the driver. I'm not sure why the original author
> named it that way.

Ah...  Then I defer to your judgement.

regards,
dan carpenter

Re: [PATCH 3/7] iio: light: tsl2583: use DEVICE_ATTR_{RO, RW, WO} macros

[Jonathan Cameron]

On 19/10/16 14:11, Dan Carpenter wrote:
> On Wed, Oct 19, 2016 at 09:08:30AM -0400, Brian Masney wrote:
>> On Wed, Oct 19, 2016 at 02:26:27PM +0300, Dan Carpenter wrote:
>>> What does illuminance0_ mean?  Can we remove that?
>>
>> I left the names of the existing sysfs attributes intact to not break
>> any existing users of the driver. I'm not sure why the original author
>> named it that way.
> 
> Ah...  Then I defer to your judgement.
Will normally be constructed as this via the info_mask stuff.
Needed to ensure it's obvious which channel we are referring to in
multichannel devices.  Convention usually means we use this when
something is very channel specific even if it's the only channel.
(feels odd otherwise ;)

Looks ugly here because of the rather contrived nature of the
macros.  I'm not keen on that side of them, but then we should
hardly ever need to use them anyway so I'm not that bothered!

Jonathan
> 
> regards,
> dan carpenter
> 
> --
> 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 3/7] iio: light: tsl2583: use DEVICE_ATTR_{RO, RW, WO} macros

[Jonathan Cameron]

On 19 October 2016 11:32:06 BST, Brian Masney <masneyb@onstation.org> wrote:
>Use the DEVICE_ATTR_RO, IIO_DEVICE_ATTR_RW, and IIO_DEVICE_ATTR_WO
>macros to create the device attributes.
>
>Signed-off-by: Brian Masney <masneyb@onstation.org>

Hi Brian, 

One very quick comment.  Driver should be using an iio_chan_spec array and relevant
 info_mask elements plus read_raw etc.

That will get rid of about half of these so I would do that before cleaning up the remainder.

J
>---
>drivers/staging/iio/light/tsl2583.c | 117
>++++++++++++++++++------------------
> 1 file changed, 57 insertions(+), 60 deletions(-)
>
>diff --git a/drivers/staging/iio/light/tsl2583.c
>b/drivers/staging/iio/light/tsl2583.c
>index 8448a87..bbb8fc3 100644
>--- a/drivers/staging/iio/light/tsl2583.c
>+++ b/drivers/staging/iio/light/tsl2583.c
>@@ -501,8 +501,8 @@ static int taos_chip_off(struct iio_dev *indio_dev)
> 
> /* Sysfs Interface Functions */
> 
>-static ssize_t taos_power_state_show(struct device *dev,
>-				     struct device_attribute *attr, char *buf)
>+static ssize_t power_state_show(struct device *dev,
>+				struct device_attribute *attr, char *buf)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -510,9 +510,9 @@ static ssize_t taos_power_state_show(struct device
>*dev,
> 	return sprintf(buf, "%d\n", chip->taos_chip_status);
> }
> 
>-static ssize_t taos_power_state_store(struct device *dev,
>-				      struct device_attribute *attr,
>-				      const char *buf, size_t len)
>+static ssize_t power_state_store(struct device *dev,
>+				 struct device_attribute *attr,
>+				 const char *buf, size_t len)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	int value;
>@@ -528,8 +528,9 @@ static ssize_t taos_power_state_store(struct device
>*dev,
> 	return len;
> }
> 
>-static ssize_t taos_gain_show(struct device *dev,
>-			      struct device_attribute *attr, char *buf)
>+static ssize_t illuminance0_calibscale_show(struct device *dev,
>+					    struct device_attribute *attr,
>+					    char *buf)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -553,9 +554,9 @@ static ssize_t taos_gain_show(struct device *dev,
> 	return sprintf(buf, "%s\n", gain);
> }
> 
>-static ssize_t taos_gain_store(struct device *dev,
>-			       struct device_attribute *attr,
>-			       const char *buf, size_t len)
>+static ssize_t illuminance0_calibscale_store(struct device *dev,
>+					     struct device_attribute *attr,
>+					     const char *buf, size_t len)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -587,15 +588,16 @@ gain_store_done:
> 	return ret;
> }
> 
>-static ssize_t taos_gain_available_show(struct device *dev,
>-					struct device_attribute *attr,
>-					char *buf)
>+static ssize_t illuminance0_calibscale_available_show(struct device
>*dev,
>+						  struct device_attribute *attr,
>+						  char *buf)
> {
> 	return sprintf(buf, "%s\n", "1 8 16 111");
> }
> 
>-static ssize_t taos_als_time_show(struct device *dev,
>-				  struct device_attribute *attr, char *buf)
>+static ssize_t illuminance0_integration_time_show(struct device *dev,
>+						  struct device_attribute *attr,
>+						  char *buf)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -603,9 +605,9 @@ static ssize_t taos_als_time_show(struct device
>*dev,
> 	return sprintf(buf, "%d\n", chip->taos_settings.als_time);
> }
> 
>-static ssize_t taos_als_time_store(struct device *dev,
>-				   struct device_attribute *attr,
>-				   const char *buf, size_t len)
>+static ssize_t illuminance0_integration_time_store(struct device *dev,
>+						  struct device_attribute *attr,
>+						  const char *buf, size_t len)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -627,16 +629,17 @@ als_time_store_done:
> 	return ret;
> }
> 
>-static ssize_t taos_als_time_available_show(struct device *dev,
>-					    struct device_attribute *attr,
>-					    char *buf)
>+static ssize_t illuminance0_integration_time_available_show(struct
>device *dev,
>+						  struct device_attribute *attr,
>+						  char *buf)
> {
> 	return sprintf(buf, "%s\n",
> 		"50 100 150 200 250 300 350 400 450 500 550 600 650");
> }
> 
>-static ssize_t taos_als_trim_show(struct device *dev,
>-				  struct device_attribute *attr, char *buf)
>+static ssize_t illuminance0_calibbias_show(struct device *dev,
>+					   struct device_attribute *attr,
>+					   char *buf)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -644,9 +647,9 @@ static ssize_t taos_als_trim_show(struct device
>*dev,
> 	return sprintf(buf, "%d\n", chip->taos_settings.als_gain_trim);
> }
> 
>-static ssize_t taos_als_trim_store(struct device *dev,
>-				   struct device_attribute *attr,
>-				   const char *buf, size_t len)
>+static ssize_t illuminance0_calibbias_store(struct device *dev,
>+					    struct device_attribute *attr,
>+					    const char *buf, size_t len)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -661,9 +664,9 @@ static ssize_t taos_als_trim_store(struct device
>*dev,
> 	return len;
> }
> 
>-static ssize_t taos_als_cal_target_show(struct device *dev,
>-					struct device_attribute *attr,
>-					char *buf)
>+static ssize_t illuminance0_input_target_show(struct device *dev,
>+					      struct device_attribute *attr,
>+					      char *buf)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -671,9 +674,9 @@ static ssize_t taos_als_cal_target_show(struct
>device *dev,
> 	return sprintf(buf, "%d\n", chip->taos_settings.als_cal_target);
> }
> 
>-static ssize_t taos_als_cal_target_store(struct device *dev,
>-					 struct device_attribute *attr,
>-					 const char *buf, size_t len)
>+static ssize_t illuminance0_input_target_store(struct device *dev,
>+					       struct device_attribute *attr,
>+					       const char *buf, size_t len)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -688,8 +691,9 @@ static ssize_t taos_als_cal_target_store(struct
>device *dev,
> 	return len;
> }
> 
>-static ssize_t taos_lux_show(struct device *dev, struct
>device_attribute *attr,
>-			     char *buf)
>+static ssize_t illuminance0_input_show(struct device *dev,
>+				       struct device_attribute *attr,
>+				       char *buf)
> {
> 	int ret;
> 
>@@ -700,9 +704,9 @@ static ssize_t taos_lux_show(struct device *dev,
>struct device_attribute *attr,
> 	return sprintf(buf, "%d\n", ret);
> }
> 
>-static ssize_t taos_do_calibrate(struct device *dev,
>-				 struct device_attribute *attr,
>-				 const char *buf, size_t len)
>+static ssize_t illuminance0_calibrate_store(struct device *dev,
>+					    struct device_attribute *attr,
>+					    const char *buf, size_t len)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	int value;
>@@ -716,8 +720,9 @@ static ssize_t taos_do_calibrate(struct device
>*dev,
> 	return len;
> }
> 
>-static ssize_t taos_luxtable_show(struct device *dev,
>-				  struct device_attribute *attr, char *buf)
>+static ssize_t illuminance0_lux_table_show(struct device *dev,
>+					   struct device_attribute *attr,
>+					   char *buf)
> {
> 	int i;
> 	int offset = 0;
>@@ -741,9 +746,9 @@ static ssize_t taos_luxtable_show(struct device
>*dev,
> 	return offset;
> }
> 
>-static ssize_t taos_luxtable_store(struct device *dev,
>-				   struct device_attribute *attr,
>-				   const char *buf, size_t len)
>+static ssize_t illuminance0_lux_table_store(struct device *dev,
>+					    struct device_attribute *attr,
>+					    const char *buf, size_t len)
> {
> 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> 	struct tsl2583_chip *chip = iio_priv(indio_dev);
>@@ -781,29 +786,21 @@ luxable_store_done:
> 	return ret;
> }
> 
>-static DEVICE_ATTR(power_state, S_IRUGO | S_IWUSR,
>-		taos_power_state_show, taos_power_state_store);
>+static DEVICE_ATTR_RW(power_state);
> 
>-static DEVICE_ATTR(illuminance0_calibscale, S_IRUGO | S_IWUSR,
>-		taos_gain_show, taos_gain_store);
>-static DEVICE_ATTR(illuminance0_calibscale_available, S_IRUGO,
>-		taos_gain_available_show, NULL);
>+static DEVICE_ATTR_RW(illuminance0_calibscale);
>+static DEVICE_ATTR_RO(illuminance0_calibscale_available);
> 
>-static DEVICE_ATTR(illuminance0_integration_time, S_IRUGO | S_IWUSR,
>-		taos_als_time_show, taos_als_time_store);
>-static DEVICE_ATTR(illuminance0_integration_time_available, S_IRUGO,
>-		taos_als_time_available_show, NULL);
>+static DEVICE_ATTR_RW(illuminance0_integration_time);
>+static DEVICE_ATTR_RO(illuminance0_integration_time_available);
> 
>-static DEVICE_ATTR(illuminance0_calibbias, S_IRUGO | S_IWUSR,
>-		taos_als_trim_show, taos_als_trim_store);
>+static DEVICE_ATTR_RW(illuminance0_calibbias);
> 
>-static DEVICE_ATTR(illuminance0_input_target, S_IRUGO | S_IWUSR,
>-		taos_als_cal_target_show, taos_als_cal_target_store);
>+static DEVICE_ATTR_RW(illuminance0_input_target);
> 
>-static DEVICE_ATTR(illuminance0_input, S_IRUGO, taos_lux_show, NULL);
>-static DEVICE_ATTR(illuminance0_calibrate, S_IWUSR, NULL,
>taos_do_calibrate);
>-static DEVICE_ATTR(illuminance0_lux_table, S_IRUGO | S_IWUSR,
>-		taos_luxtable_show, taos_luxtable_store);
>+static DEVICE_ATTR_RO(illuminance0_input);
>+static DEVICE_ATTR_WO(illuminance0_calibrate);
>+static DEVICE_ATTR_RW(illuminance0_lux_table);
> 
> static struct attribute *sysfs_attrs_ctrl[] = {
> 	&dev_attr_power_state.attr,

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[PATCH 4/7] iio: light: tsl2583: return proper error code in sysfs store functions

[Brian Masney]

illuminance0_calibbias_store(), illuminance0_input_target_store(), and
illuminance0_calibrate_store() did not return an error code when
an invalid value was passed in. The input was checked to see if the
input was valid, however the caller would not be notified that an
invalid value was passed in. This patch changes these three functions to
return -EINVAL when invalid input is passed in.

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

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index bbb8fc3..a60433e 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -657,9 +657,10 @@ static ssize_t illuminance0_calibbias_store(struct device *dev,
 
 	if (kstrtoint(buf, 0, &value))
 		return -EINVAL;
+	else if (!value)
+		return -EINVAL;
 
-	if (value)
-		chip->taos_settings.als_gain_trim = value;
+	chip->taos_settings.als_gain_trim = value;
 
 	return len;
 }
@@ -684,9 +685,10 @@ static ssize_t illuminance0_input_target_store(struct device *dev,
 
 	if (kstrtoint(buf, 0, &value))
 		return -EINVAL;
+	else if (!value)
+		return -EINVAL;
 
-	if (value)
-		chip->taos_settings.als_cal_target = value;
+	chip->taos_settings.als_cal_target = value;
 
 	return len;
 }
@@ -713,9 +715,10 @@ static ssize_t illuminance0_calibrate_store(struct device *dev,
 
 	if (kstrtoint(buf, 0, &value))
 		return -EINVAL;
+	else if (value != 1)
+		return -EINVAL;
 
-	if (value == 1)
-		taos_als_calibrate(indio_dev);
+	taos_als_calibrate(indio_dev);
 
 	return len;
 }
-- 
2.7.4

[PATCH 5/7] iio: light: tsl2583: check return values from taos_chip_{on,off}

[Brian Masney]

The return values from taos_chip_on() and taos_chip_off() was not
checked in several places. This patch adds proper error checking to
these function calls.

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

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index a60433e..60f0ce9 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -515,15 +515,18 @@ static ssize_t power_state_store(struct device *dev,
 				 const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	int value;
+	int value, ret;
 
 	if (kstrtoint(buf, 0, &value))
 		return -EINVAL;
 
 	if (!value)
-		taos_chip_off(indio_dev);
+		ret = taos_chip_off(indio_dev);
 	else
-		taos_chip_on(indio_dev);
+		ret = taos_chip_on(indio_dev);
+
+	if (ret < 0)
+		return ret;
 
 	return len;
 }
@@ -775,14 +778,20 @@ static ssize_t illuminance0_lux_table_store(struct device *dev,
 		goto luxable_store_done;
 	}
 
-	if (chip->taos_chip_status == TSL258X_CHIP_WORKING)
-		taos_chip_off(indio_dev);
+	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
+		ret = taos_chip_off(indio_dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	/* Zero out the table */
 	memset(taos_device_lux, 0, sizeof(taos_device_lux));
 	memcpy(taos_device_lux, &value[1], (value[0] * 4));
 
-	taos_chip_on(indio_dev);
+	ret = taos_chip_on(indio_dev);
+	if (ret < 0)
+		return ret;
+
 	ret = len;
 
 luxable_store_done:
@@ -914,7 +923,9 @@ static int taos_probe(struct i2c_client *clientp,
 	taos_defaults(chip);
 
 	/* Make sure the chip is on */
-	taos_chip_on(indio_dev);
+	ret = taos_chip_on(indio_dev);
+	if (ret < 0)
+		return ret;
 
 	dev_info(&clientp->dev, "Light sensor found.\n");
 
-- 
2.7.4

Re: [PATCH 5/7] iio: light: tsl2583: check return values from taos_chip_{on, off}

[Dan Carpenter]

On Wed, Oct 19, 2016 at 06:32:08AM -0400, Brian Masney wrote:
> @@ -775,14 +778,20 @@ static ssize_t illuminance0_lux_table_store(struct device *dev,
>  		goto luxable_store_done;
>  	}
>  
> -	if (chip->taos_chip_status == TSL258X_CHIP_WORKING)
> -		taos_chip_off(indio_dev);
> +	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
> +		ret = taos_chip_off(indio_dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	/* Zero out the table */
>  	memset(taos_device_lux, 0, sizeof(taos_device_lux));
>  	memcpy(taos_device_lux, &value[1], (value[0] * 4));
>  
> -	taos_chip_on(indio_dev);
> +	ret = taos_chip_on(indio_dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = len;
>  
>  luxable_store_done:


See, here you are adding direct returns in the middle of a single return
function, and you promised yourself that you would never do that and it
would mean that you never ever forgot to add error handling.  But we're
not even outside of the patchset yet and your complicated future
proofing has already failed.

You know you just want direct returns because that is the simplest way
to program.  "goto luxable_store_done;"  What does that even mean?  But
"return -EINVAL;" is simple.  It does one thing and it does it well.

Go with your heart.  My research says that the complicated single return
functions are going to be buggier in the long run anyway so your heart
is leading you on the right path.

regards,
dan carpenter

Re: [PATCH 5/7] iio: light: tsl2583: check return values from taos_chip_{on, off}

[Brian Masney]

On Wed, Oct 19, 2016 at 02:22:49PM +0300, Dan Carpenter wrote:
> On Wed, Oct 19, 2016 at 06:32:08AM -0400, Brian Masney wrote:
> > @@ -775,14 +778,20 @@ static ssize_t illuminance0_lux_table_store(struct device *dev,
> >  		goto luxable_store_done;
> >  	}
> >  
> > -	if (chip->taos_chip_status == TSL258X_CHIP_WORKING)
> > -		taos_chip_off(indio_dev);
> > +	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
> > +		ret = taos_chip_off(indio_dev);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> >  
> >  	/* Zero out the table */
> >  	memset(taos_device_lux, 0, sizeof(taos_device_lux));
> >  	memcpy(taos_device_lux, &value[1], (value[0] * 4));
> >  
> > -	taos_chip_on(indio_dev);
> > +	ret = taos_chip_on(indio_dev);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	ret = len;
> >  
> >  luxable_store_done:
> 
> 
> See, here you are adding direct returns in the middle of a single return
> function, and you promised yourself that you would never do that and it
> would mean that you never ever forgot to add error handling.  But we're
> not even outside of the patchset yet and your complicated future
> proofing has already failed.
> 
> You know you just want direct returns because that is the simplest way
> to program.  "goto luxable_store_done;"  What does that even mean?  But
> "return -EINVAL;" is simple.  It does one thing and it does it well.
> 
> Go with your heart.  My research says that the complicated single return
> functions are going to be buggier in the long run anyway so your heart
> is leading you on the right path.

Ok, I'll rework my patch series to stick with the direct returns. I
personally prefer that approach. I was using the gotos since I thought
that was standard convention in the kernel.

Brian

Re: [PATCH 5/7] iio: light: tsl2583: check return values from taos_chip_{on, off}

[Dan Carpenter]

On Wed, Oct 19, 2016 at 08:48:32AM -0400, Brian Masney wrote:
> Ok, I'll rework my patch series to stick with the direct returns. I
> personally prefer that approach. I was using the gotos since I thought
> that was standard convention in the kernel.
> 

It *should* but "goto unlock;"  When you can tell what the goto does,
that's great.  But these gotos don't do anything so by definition the
names are going to be meaningless.

regards,
dan carpenter

Re: [PATCH 5/7] iio: light: tsl2583: check return values from taos_chip_{on, off}

[Dan Carpenter]

On Wed, Oct 19, 2016 at 03:54:23PM +0300, Dan Carpenter wrote:
> On Wed, Oct 19, 2016 at 08:48:32AM -0400, Brian Masney wrote:
> > Ok, I'll rework my patch series to stick with the direct returns. I
> > personally prefer that approach. I was using the gotos since I thought
> > that was standard convention in the kernel.
> > 
> 
> It *should* but "goto unlock;"

s/but/be/

regards,
dan carpenter

[PATCH 6/7] iio: light: tsl2583: add locking to sysfs *_store() functions

[Brian Masney]

The sysfs attributes modifies variables that are accessed elsewhere when
a mutex is locked. Add locking to the sysfs *_store() functions to avoid
a possible race condition when measurements are taken.

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

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 60f0ce9..47656ae 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -515,16 +515,21 @@ static ssize_t power_state_store(struct device *dev,
 				 const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct tsl2583_chip *chip = iio_priv(indio_dev);
 	int value, ret;
 
 	if (kstrtoint(buf, 0, &value))
 		return -EINVAL;
 
+	mutex_lock(&chip->als_mutex);
+
 	if (!value)
 		ret = taos_chip_off(indio_dev);
 	else
 		ret = taos_chip_on(indio_dev);
 
+	mutex_unlock(&chip->als_mutex);
+
 	if (ret < 0)
 		return ret;
 
@@ -568,6 +573,8 @@ static ssize_t illuminance0_calibscale_store(struct device *dev,
 	if (kstrtoint(buf, 0, &value))
 		goto gain_store_done;
 
+	mutex_lock(&chip->als_mutex);
+
 	switch (value) {
 	case 1:
 		chip->taos_settings.als_gain = 0;
@@ -583,10 +590,12 @@ static ssize_t illuminance0_calibscale_store(struct device *dev,
 		break;
 	default:
 		dev_err(dev, "Invalid Gain Index (must be 1,8,16,111)\n");
-		goto gain_store_done;
+		goto gain_store_unlock;
 	}
 	ret = len;
 
+gain_store_unlock:
+	mutex_unlock(&chip->als_mutex);
 gain_store_done:
 	return ret;
 }
@@ -625,7 +634,9 @@ static ssize_t illuminance0_integration_time_store(struct device *dev,
 	if (value % 50)
 		goto als_time_store_done;
 
+	mutex_lock(&chip->als_mutex);
 	chip->taos_settings.als_time = value;
+	mutex_unlock(&chip->als_mutex);
 	ret = len;
 
 als_time_store_done:
@@ -663,7 +674,9 @@ static ssize_t illuminance0_calibbias_store(struct device *dev,
 	else if (!value)
 		return -EINVAL;
 
+	mutex_lock(&chip->als_mutex);
 	chip->taos_settings.als_gain_trim = value;
+	mutex_unlock(&chip->als_mutex);
 
 	return len;
 }
@@ -691,7 +704,9 @@ static ssize_t illuminance0_input_target_store(struct device *dev,
 	else if (!value)
 		return -EINVAL;
 
+	mutex_lock(&chip->als_mutex);
 	chip->taos_settings.als_cal_target = value;
+	mutex_unlock(&chip->als_mutex);
 
 	return len;
 }
@@ -714,6 +729,7 @@ static ssize_t illuminance0_calibrate_store(struct device *dev,
 					    const char *buf, size_t len)
 {
 	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct tsl2583_chip *chip = iio_priv(indio_dev);
 	int value;
 
 	if (kstrtoint(buf, 0, &value))
@@ -721,7 +737,9 @@ static ssize_t illuminance0_calibrate_store(struct device *dev,
 	else if (value != 1)
 		return -EINVAL;
 
+	mutex_lock(&chip->als_mutex);
 	taos_als_calibrate(indio_dev);
+	mutex_unlock(&chip->als_mutex);
 
 	return len;
 }
@@ -778,6 +796,8 @@ static ssize_t illuminance0_lux_table_store(struct device *dev,
 		goto luxable_store_done;
 	}
 
+	mutex_lock(&chip->als_mutex);
+
 	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
 		ret = taos_chip_off(indio_dev);
 		if (ret < 0)
@@ -794,6 +814,8 @@ static ssize_t illuminance0_lux_table_store(struct device *dev,
 
 	ret = len;
 
+	mutex_unlock(&chip->als_mutex);
+
 luxable_store_done:
 	return ret;
 }
-- 
2.7.4

Re: [PATCH 6/7] iio: light: tsl2583: add locking to sysfs *_store() functions

[Dan Carpenter]

On Wed, Oct 19, 2016 at 06:32:09AM -0400, Brian Masney wrote:
> @@ -778,6 +796,8 @@ static ssize_t illuminance0_lux_table_store(struct device *dev,
>  		goto luxable_store_done;
>  	}
>  
> +	mutex_lock(&chip->als_mutex);
> +
>  	if (chip->taos_chip_status == TSL258X_CHIP_WORKING) {
>  		ret = taos_chip_off(indio_dev);
>  		if (ret < 0)
> @@ -794,6 +814,8 @@ static ssize_t illuminance0_lux_table_store(struct device *dev,
>  
>  	ret = len;
>  
> +	mutex_unlock(&chip->als_mutex);
> +
>  luxable_store_done:
>  	return ret;
>  }


HAHAHHA HAHAHAH HAH AHHA HAHHAH AH HAHAHHAHAH AHHAHHAHAHHA HAHA HAHAHAH.

*wipes tear from eye*.  *continues laughing*.

HAHAHAHAHHAHAHA HAHA HAHAHAHA HAHAHA HAHAHAH AHAHAHAHA AHHAHAH.

This patch introduces a return with lock held bug.  All those do nothing
gotos, and we still missed when we actually had to do a "goto unlock".
If every function is special and complicated that reall means that no
functions are special.

Don't do do-nothing gotos.

regards,
dan carpenter

Re: [PATCH 6/7] iio: light: tsl2583: add locking to sysfs *_store() functions

[Dan Carpenter]

I appologize for laughing, but I am still secretly amused in my heart.

regards,
dan carpenter

Re: [PATCH 6/7] iio: light: tsl2583: add locking to sysfs *_store() functions

[Jonathan Cameron]

On 19/10/16 12:37, Dan Carpenter wrote:
> I appologize for laughing, but I am still secretly amused in my heart.
> 
> regards,
> dan carpenter
> 
Fewer beers or less caffeine for Dan!

Key take away here is keep things simple.  The gotos in my mind
would actually have made sense, but I wouldn't do the refactoring until
you need it. i.e. when you introduce the locking in this patch.
> --
> 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 6/7] iio: light: tsl2583: add locking to sysfs *_store() functions

[Dan Carpenter]

On Sat, Oct 22, 2016 at 06:29:55PM +0100, Jonathan Cameron wrote:
> On 19/10/16 12:37, Dan Carpenter wrote:
> > I appologize for laughing, but I am still secretly amused in my heart.
> > 
> > regards,
> > dan carpenter
> > 
> Fewer beers or less caffeine for Dan!

;)

> 
> Key take away here is keep things simple.  The gotos in my mind
> would actually have made sense, but I wouldn't do the refactoring until
> you need it. i.e. when you introduce the locking in this patch.

Yeah, I totally agree do it when you need it.  I like gotos if they're
needed.

regards,
dan carpenter

[PATCH 7/7] iio: light: tsl2583: fix concurrency issue in taos_get_lux()

[Brian Masney]

taos_get_lux() calls mutex_trylock(). If the lock could not be acquired,
then chip->als_cur_info.lux is returned. The issue is that this value
is updated while the mutex is held and could cause a half written value
to be returned to the caller. This patch changes the call to
mutex_trylock() with mutex_lock().

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
This is the most controversial change in my patch set. There are two
other possible solutions that I could envision to work around this
issue:

1) Return -EBUSY and make the caller responsible for backing off
2) Change this driver to use RCU instead of a mutex

 drivers/staging/iio/light/tsl2583.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
index 47656ae..c4d2e3a 100644
--- a/drivers/staging/iio/light/tsl2583.c
+++ b/drivers/staging/iio/light/tsl2583.c
@@ -206,10 +206,7 @@ static int taos_get_lux(struct iio_dev *indio_dev)
 	u32 ch0lux = 0;
 	u32 ch1lux = 0;
 
-	if (mutex_trylock(&chip->als_mutex) == 0) {
-		dev_info(&chip->client->dev, "taos_get_lux device is busy\n");
-		return chip->als_cur_info.lux; /* busy, so return LAST VALUE */
-	}
+	mutex_lock(&chip->als_mutex);
 
 	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
 		/* device is not enabled */
-- 
2.7.4

Re: [PATCH 7/7] iio: light: tsl2583: fix concurrency issue in taos_get_lux()

[Jonathan Cameron]

On 19/10/16 11:32, Brian Masney wrote:
> taos_get_lux() calls mutex_trylock(). If the lock could not be acquired,
> then chip->als_cur_info.lux is returned. The issue is that this value
> is updated while the mutex is held and could cause a half written value
> to be returned to the caller. This patch changes the call to
> mutex_trylock() with mutex_lock().
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
I'd go with the simple approach you have here.

If someone cares enough they can figure out the complex solution.  Probably
should be pushed off to userspace.  It's not unusual for devices to take
'a little while' to respond to sysfs reads.

Jonathan
> ---
> This is the most controversial change in my patch set. There are two
> other possible solutions that I could envision to work around this
> issue:
> 
> 1) Return -EBUSY and make the caller responsible for backing off
> 2) Change this driver to use RCU instead of a mutex
> 
>  drivers/staging/iio/light/tsl2583.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 47656ae..c4d2e3a 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -206,10 +206,7 @@ static int taos_get_lux(struct iio_dev *indio_dev)
>  	u32 ch0lux = 0;
>  	u32 ch1lux = 0;
>  
> -	if (mutex_trylock(&chip->als_mutex) == 0) {
> -		dev_info(&chip->client->dev, "taos_get_lux device is busy\n");
> -		return chip->als_cur_info.lux; /* busy, so return LAST VALUE */
> -	}
> +	mutex_lock(&chip->als_mutex);
>  
>  	if (chip->taos_chip_status != TSL258X_CHIP_WORKING) {
>  		/* device is not enabled */
> 

Re: [PATCH 1/7] iio: light: tsl2583: return proper error code

[Jonathan Cameron]

On 19/10/16 11:32, Brian Masney wrote:
> taos_gain_store() and taos_als_calibrate() both have a code path where
> -1 was returned. This patch changes the code so that a proper error code
> is returned to make the code consistent with the error paths that are
> present within those same functions.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/staging/iio/light/tsl2583.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/tsl2583.c b/drivers/staging/iio/light/tsl2583.c
> index 08f1583..1e42a19 100644
> --- a/drivers/staging/iio/light/tsl2583.c
> +++ b/drivers/staging/iio/light/tsl2583.c
> @@ -359,7 +359,7 @@ static int taos_als_calibrate(struct iio_dev *indio_dev)
>  			!= (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 -1;
> +		return -ENODATA;
Hmm. Not sure about this as the error value.  Perhaps a simple -EINVAL?

Also, this is still eating the possible error returned directly by
i2c_smbus_read_byte - would you mind fixing that whilst we are here?
>  	}
>  
>  	ret = i2c_smbus_write_byte(chip->client,
> @@ -569,7 +569,7 @@ static ssize_t taos_gain_store(struct device *dev,
>  		break;
>  	default:
>  		dev_err(dev, "Invalid Gain Index (must be 1,8,16,111)\n");
> -		return -1;
> +		return -EINVAL;
>  	}
>  
>  	return len;
>