[PATCH v2 00/15] staging cleanups

[PATCH v2 00/15] staging cleanups

[Brian Masney]

This patch set contains more of my cleanups to the isl29028 light driver
in preparation for moving the driver out of staging. The main feature
introduced by this patch set is support for runtime power management
and autosuspend after two seconds. The rest of the patches are minor
code improvements, mostly style.

The ALS and IR portion of the sensor was tested using an ISL29028 hooked
up to a Raspberry Pi 2. In my v1 submission cover letter, I mention that
I am not able to use the proximity sensing function on my sensor. (This
is before any of my code changes.) I'm going to look into that issue
next. I suspect that it is a hardware issue on my local setup. I verified
with a visible light LED that the IRDR pin continues to be driven as
expected when a proximity reading is requested and is turned off after
two seconds of inactivity. The pin is driven the next time a proximity
reading is requested.

Datasheet:
http://www.intersil.com/content/dam/Intersil/documents/isl2/isl29028.pdf

Changes since version 1:
- Suggestions from Jonathan Cameron
  - In v1 patch #4 (#15 in this series), remove suspended flag. Added
    support for runtime power management support and autosuspend.
  - Dropped v1 patch #19 (remove legacy device tree binding)
  - Dropped v1 patch #11 (made column alignment in isl29028_channels
    consistent)
  - In v1 patches #13 and #14, if there is an error, return inside
    the error check.
  - It was suggested to drop the v1 #2 patch that removes the enable
    flag from isl29028_enable_proximity(). I think that we can apply
    that patch now that the driver has runtime power management support.
- Suggestions from Dan Carpenter
  - Dropped v1 patch #12 (fix comparison between signed and unsigned
    integers)

Brian Masney (15):
  staging: iio: isl29028: made alignment of variables in struct
    isl29028_chip consistent
  staging: iio: isl29028: fix alignment of function arguments
  staging: iio: isl29028: combine isl29028_proxim_get() and
    isl29028_read_proxim()
  staging: iio: isl29028: change newlines to improve readability
  staging: iio: isl29028: remove unused define ISL29028_DEV_ATTR
  staging: iio: isl29028: move failure logging into
    isl29028_set_proxim_sampling()
  staging: iio: isl29028: move failure logging into
    isl29028_set_als_scale()
  staging: iio: isl29028: made error messages consistent
  staging: iio: isl29028: remove unnecessary error logging in
    isl29028_chip_init_and_power_on()
  staging: iio: isl29028: remove out of memory log message
  staging: iio: isl29028: remove unnecessary parenthesis
  staging: iio: isl29028: remove enable flag from
    isl29028_enable_proximity()
  staging: iio: isl29028: only set proximity sampling rate when
    proximity is enabled
  staging: iio: isl29028: only set ALS scale when ALS sensing is enabled
  staging: iio: isl29028: add runtime power management support

 drivers/staging/iio/light/isl29028.c | 323 ++++++++++++++++++++++++-----------
 1 file changed, 221 insertions(+), 102 deletions(-)

-- 
2.9.3

[PATCH v2 01/15] staging: iio: isl29028: made alignment of variables in struct isl29028_chip consistent

[Brian Masney]

The alignment of the variables in the struct isl29028_chip is not
consistent. This changes all of the variables to use consistent
alignment to improve the code readability.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 1de81f5..4cce663 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -67,13 +67,13 @@ enum isl29028_als_ir_mode {
 };
 
 struct isl29028_chip {
-	struct mutex		lock;
-	struct regmap		*regmap;
+	struct mutex			lock;
+	struct regmap			*regmap;
 
-	unsigned int		prox_sampling;
-	bool			enable_prox;
+	unsigned int			prox_sampling;
+	bool				enable_prox;
 
-	int			lux_scale;
+	int				lux_scale;
 	enum isl29028_als_ir_mode	als_ir_mode;
 };
 
-- 
2.9.3

[PATCH v2 02/15] staging: iio: isl29028: fix alignment of function arguments

[Brian Masney]

Two separate calls to regmap_update_bits() in isl29028_set_als_scale()
and isl29028_set_als_ir_mode() did not have their function arguments
on the next line aligned correctly to the open parenthesis. This patch
corrects the alignment.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 4cce663..74eb736 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -116,7 +116,7 @@ static int isl29028_set_als_scale(struct isl29028_chip *chip, int lux_scale)
 					ISL29028_CONF_ALS_RANGE_LOW_LUX;
 
 	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
-		ISL29028_CONF_ALS_RANGE_MASK, val);
+				  ISL29028_CONF_ALS_RANGE_MASK, val);
 }
 
 static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
@@ -148,7 +148,8 @@ static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
 
 	case ISL29028_MODE_NONE:
 		return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
-			ISL29028_CONF_ALS_EN_MASK, ISL29028_CONF_ALS_DIS);
+					  ISL29028_CONF_ALS_EN_MASK,
+					  ISL29028_CONF_ALS_DIS);
 	}
 
 	if (ret < 0)
-- 
2.9.3

[PATCH v2 03/15] staging: iio: isl29028: combine isl29028_proxim_get() and isl29028_read_proxim()

[Brian Masney]

isl29028_proxim_get() checks to see if the promixity needs to be
enabled on the chip and then calls isl29028_read_proxim(). There
are no other callers of isl29028_read_proxim(). The naming between
these two functions can be confusing so this patch combines the
two to avoid the confusion.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 74eb736..a13c8db 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -201,6 +201,13 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
 	unsigned int data;
 	int ret;
 
+	if (!chip->enable_prox) {
+		ret = isl29028_enable_proximity(chip, true);
+		if (ret < 0)
+			return ret;
+		chip->enable_prox = true;
+	}
+
 	ret = regmap_read(chip->regmap, ISL29028_REG_PROX_DATA, &data);
 	if (ret < 0) {
 		dev_err(dev, "Error in reading register %d, error %d\n",
@@ -211,19 +218,6 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
 	return 0;
 }
 
-static int isl29028_proxim_get(struct isl29028_chip *chip, int *prox_data)
-{
-	int ret;
-
-	if (!chip->enable_prox) {
-		ret = isl29028_enable_proximity(chip, true);
-		if (ret < 0)
-			return ret;
-		chip->enable_prox = true;
-	}
-	return isl29028_read_proxim(chip, prox_data);
-}
-
 static int isl29028_als_get(struct isl29028_chip *chip, int *als_data)
 {
 	struct device *dev = regmap_get_device(chip->regmap);
@@ -349,7 +343,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 			ret = isl29028_ir_get(chip, val);
 			break;
 		case IIO_PROXIMITY:
-			ret = isl29028_proxim_get(chip, val);
+			ret = isl29028_read_proxim(chip, val);
 			break;
 		default:
 			break;
-- 
2.9.3

[PATCH v2 04/15] staging: iio: isl29028: change newlines to improve readability

[Brian Masney]

Add and remove newlines to improve code readability in preparation for
moving the driver out of staging.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index a13c8db..d05d761 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -69,10 +69,8 @@ enum isl29028_als_ir_mode {
 struct isl29028_chip {
 	struct mutex			lock;
 	struct regmap			*regmap;
-
 	unsigned int			prox_sampling;
 	bool				enable_prox;
-
 	int				lux_scale;
 	enum isl29028_als_ir_mode	als_ir_mode;
 };
@@ -88,6 +86,7 @@ static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
 		if (period >= prox_period[sel])
 			break;
 	}
+
 	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
 				  ISL29028_CONF_PROX_SLP_MASK,
 				  sel << ISL29028_CONF_PROX_SLP_SH);
@@ -107,6 +106,7 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip, bool enable)
 
 	/* Wait for conversion to be complete for first sample */
 	mdelay(DIV_ROUND_UP(1000, chip->prox_sampling));
+
 	return 0;
 }
 
@@ -139,13 +139,11 @@ static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
 					 ISL29028_CONF_ALS_RANGE_MASK,
 					 ISL29028_CONF_ALS_RANGE_HIGH_LUX);
 		break;
-
 	case ISL29028_MODE_IR:
 		ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
 					 ISL29028_CONF_ALS_IR_MODE_MASK,
 					 ISL29028_CONF_ALS_IR_MODE_IR);
 		break;
-
 	case ISL29028_MODE_NONE:
 		return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
 					  ISL29028_CONF_ALS_EN_MASK,
@@ -192,6 +190,7 @@ static int isl29028_read_als_ir(struct isl29028_chip *chip, int *als_ir)
 	}
 
 	*als_ir = ((msb & 0xF) << 8) | (lsb & 0xFF);
+
 	return 0;
 }
 
@@ -205,6 +204,7 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
 		ret = isl29028_enable_proximity(chip, true);
 		if (ret < 0)
 			return ret;
+
 		chip->enable_prox = true;
 	}
 
@@ -214,7 +214,9 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
 			ISL29028_REG_PROX_DATA, ret);
 		return ret;
 	}
+
 	*prox = data;
+
 	return 0;
 }
 
@@ -245,6 +247,7 @@ static int isl29028_als_get(struct isl29028_chip *chip, int *als_data)
 		als_ir_data = (als_ir_data * 49) / 100;
 
 	*als_data = als_ir_data;
+
 	return 0;
 }
 
@@ -258,6 +261,7 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
 		dev_err(dev, "Error in enabling IR mode err %d\n", ret);
 		return ret;
 	}
+
 	return isl29028_read_als_ir(chip, ir_data);
 }
 
@@ -279,11 +283,13 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 				mask);
 			break;
 		}
+
 		if (val < 1 || val > 100) {
 			dev_err(dev,
 				"Samp_freq %d is not in range[1:100]\n", val);
 			break;
 		}
+
 		ret = isl29028_set_proxim_sampling(chip, val);
 		if (ret < 0) {
 			dev_err(dev,
@@ -291,9 +297,9 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 				ret);
 			break;
 		}
+
 		chip->prox_sampling = val;
 		break;
-
 	case IIO_LIGHT:
 		if (mask != IIO_CHAN_INFO_SCALE) {
 			dev_err(dev,
@@ -301,25 +307,29 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 				mask);
 			break;
 		}
+
 		if ((val != 125) && (val != 2000)) {
 			dev_err(dev,
 				"lux scale %d is invalid [125, 2000]\n", val);
 			break;
 		}
+
 		ret = isl29028_set_als_scale(chip, val);
 		if (ret < 0) {
 			dev_err(dev,
 				"Setting lux scale fail with error %d\n", ret);
 			break;
 		}
+
 		chip->lux_scale = val;
 		break;
-
 	default:
 		dev_err(dev, "Unsupported channel type\n");
 		break;
 	}
+
 	mutex_unlock(&chip->lock);
+
 	return ret;
 }
 
@@ -348,30 +358,32 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 		default:
 			break;
 		}
+
 		if (ret < 0)
 			break;
+
 		ret = IIO_VAL_INT;
 		break;
-
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		if (chan->type != IIO_PROXIMITY)
 			break;
+
 		*val = chip->prox_sampling;
 		ret = IIO_VAL_INT;
 		break;
-
 	case IIO_CHAN_INFO_SCALE:
 		if (chan->type != IIO_LIGHT)
 			break;
 		*val = chip->lux_scale;
 		ret = IIO_VAL_INT;
 		break;
-
 	default:
 		dev_err(dev, "mask value 0x%08lx not supported\n", mask);
 		break;
 	}
+
 	mutex_unlock(&chip->lock);
+
 	return ret;
 }
 
@@ -497,6 +509,7 @@ static int isl29028_probe(struct i2c_client *client,
 			ISL29028_REG_TEST1_MODE, ret);
 		return ret;
 	}
+
 	ret = regmap_write(chip->regmap, ISL29028_REG_TEST2_MODE, 0x0);
 	if (ret < 0) {
 		dev_err(&client->dev,
@@ -517,6 +530,7 @@ static int isl29028_probe(struct i2c_client *client,
 	indio_dev->name = id->name;
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->modes = INDIO_DIRECT_MODE;
+
 	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev,
@@ -524,6 +538,7 @@ static int isl29028_probe(struct i2c_client *client,
 			ret);
 		return ret;
 	}
+
 	return 0;
 }
 
-- 
2.9.3

[PATCH v2 05/15] staging: iio: isl29028: remove unused define ISL29028_DEV_ATTR

[Brian Masney]

The #define ISL29028_DEV_ATTR was not used so this patch removes the
unnecessary code.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index d05d761..7074e62 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -391,7 +391,6 @@ static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
 				"1 3 5 10 13 20 83 100");
 static IIO_CONST_ATTR(in_illuminance_scale_available, "125 2000");
 
-#define ISL29028_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
 #define ISL29028_CONST_ATTR(name) (&iio_const_attr_##name.dev_attr.attr)
 static struct attribute *isl29028_attributes[] = {
 	ISL29028_CONST_ATTR(in_proximity_sampling_frequency_available),
-- 
2.9.3

[PATCH v2 06/15] staging: iio: isl29028: move failure logging into isl29028_set_proxim_sampling()

[Brian Masney]

When isl29028_set_proxim_sampling() fails, it was up to both callers to
log the failure message. This patch moves the logging into
isl29028_set_proxim_sampling() to reduce the overall amount of code in
the driver.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 7074e62..bd85ccb 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -78,18 +78,29 @@ struct isl29028_chip {
 static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
 					unsigned int sampling)
 {
+	struct device *dev = regmap_get_device(chip->regmap);
 	static unsigned int prox_period[] = {800, 400, 200, 100, 75, 50, 12, 0};
-	int sel;
 	unsigned int period = DIV_ROUND_UP(1000, sampling);
+	int sel, ret;
 
 	for (sel = 0; sel < ARRAY_SIZE(prox_period); ++sel) {
 		if (period >= prox_period[sel])
 			break;
 	}
 
-	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
-				  ISL29028_CONF_PROX_SLP_MASK,
-				  sel << ISL29028_CONF_PROX_SLP_SH);
+	ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
+				 ISL29028_CONF_PROX_SLP_MASK,
+				 sel << ISL29028_CONF_PROX_SLP_SH);
+
+	if (ret < 0) {
+		dev_err(dev, "%s(): Error %d setting the proximity sampling\n",
+			__func__, ret);
+		return ret;
+	}
+
+	chip->prox_sampling = sampling;
+
+	return ret;
 }
 
 static int isl29028_enable_proximity(struct isl29028_chip *chip, bool enable)
@@ -291,14 +302,6 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 		}
 
 		ret = isl29028_set_proxim_sampling(chip, val);
-		if (ret < 0) {
-			dev_err(dev,
-				"Setting proximity samp_freq fail, err %d\n",
-				ret);
-			break;
-		}
-
-		chip->prox_sampling = val;
 		break;
 	case IIO_LIGHT:
 		if (mask != IIO_CHAN_INFO_SCALE) {
@@ -437,10 +440,8 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
 	}
 
 	ret = isl29028_set_proxim_sampling(chip, chip->prox_sampling);
-	if (ret < 0) {
-		dev_err(dev, "setting the proximity, err = %d\n", ret);
+	if (ret < 0)
 		return ret;
-	}
 
 	ret = isl29028_set_als_scale(chip, chip->lux_scale);
 	if (ret < 0)
-- 
2.9.3

[PATCH v2 07/15] staging: iio: isl29028: move failure logging into isl29028_set_als_scale()

[Brian Masney]

When isl29028_set_als_scale() fails, it was up to both callers to log
the failure message. This patch moves the logging into
isl29028_set_als_scale() to reduce the overall amount of code in the
driver.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index bd85ccb..8e7b3db 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -123,11 +123,22 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip, bool enable)
 
 static int isl29028_set_als_scale(struct isl29028_chip *chip, int lux_scale)
 {
+	struct device *dev = regmap_get_device(chip->regmap);
 	int val = (lux_scale == 2000) ? ISL29028_CONF_ALS_RANGE_HIGH_LUX :
 					ISL29028_CONF_ALS_RANGE_LOW_LUX;
+	int ret;
+
+	ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
+				 ISL29028_CONF_ALS_RANGE_MASK, val);
+	if (ret < 0) {
+		dev_err(dev, "%s(): Error %d setting the ALS scale\n", __func__,
+			ret);
+		return ret;
+	}
 
-	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
-				  ISL29028_CONF_ALS_RANGE_MASK, val);
+	chip->lux_scale = lux_scale;
+
+	return ret;
 }
 
 static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
@@ -318,13 +329,6 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 		}
 
 		ret = isl29028_set_als_scale(chip, val);
-		if (ret < 0) {
-			dev_err(dev,
-				"Setting lux scale fail with error %d\n", ret);
-			break;
-		}
-
-		chip->lux_scale = val;
 		break;
 	default:
 		dev_err(dev, "Unsupported channel type\n");
@@ -443,10 +447,7 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
 	if (ret < 0)
 		return ret;
 
-	ret = isl29028_set_als_scale(chip, chip->lux_scale);
-	if (ret < 0)
-		dev_err(dev, "setting als scale failed, err = %d\n", ret);
-	return ret;
+	return isl29028_set_als_scale(chip, chip->lux_scale);
 }
 
 static bool isl29028_is_volatile_reg(struct device *dev, unsigned int reg)
-- 
2.9.3

[PATCH v2 08/15] staging: iio: isl29028: made error messages consistent

[Brian Masney]

The wording and style of the different error messages was not
consistent. This patch makes the wording and style consistent
throughout the driver.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 8e7b3db..cffecf9 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -200,14 +200,16 @@ static int isl29028_read_als_ir(struct isl29028_chip *chip, int *als_ir)
 	ret = regmap_read(chip->regmap, ISL29028_REG_ALSIR_L, &lsb);
 	if (ret < 0) {
 		dev_err(dev,
-			"Error in reading register ALSIR_L err %d\n", ret);
+			"%s(): Error %d reading register ALSIR_L\n",
+			__func__, ret);
 		return ret;
 	}
 
 	ret = regmap_read(chip->regmap, ISL29028_REG_ALSIR_U, &msb);
 	if (ret < 0) {
 		dev_err(dev,
-			"Error in reading register ALSIR_U err %d\n", ret);
+			"%s(): Error %d reading register ALSIR_U\n",
+			__func__, ret);
 		return ret;
 	}
 
@@ -232,8 +234,8 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
 
 	ret = regmap_read(chip->regmap, ISL29028_REG_PROX_DATA, &data);
 	if (ret < 0) {
-		dev_err(dev, "Error in reading register %d, error %d\n",
-			ISL29028_REG_PROX_DATA, ret);
+		dev_err(dev, "%s(): Error %d reading register PROX_DATA\n",
+			__func__, ret);
 		return ret;
 	}
 
@@ -250,7 +252,8 @@ static int isl29028_als_get(struct isl29028_chip *chip, int *als_data)
 
 	ret = isl29028_set_als_ir_mode(chip, ISL29028_MODE_ALS);
 	if (ret < 0) {
-		dev_err(dev, "Error in enabling ALS mode err %d\n", ret);
+		dev_err(dev, "%s(): Error %d enabling ALS mode\n", __func__,
+			ret);
 		return ret;
 	}
 
@@ -280,7 +283,8 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
 
 	ret = isl29028_set_als_ir_mode(chip, ISL29028_MODE_IR);
 	if (ret < 0) {
-		dev_err(dev, "Error in enabling IR mode err %d\n", ret);
+		dev_err(dev, "%s(): Error %d enabling IR mode\n", __func__,
+			ret);
 		return ret;
 	}
 
@@ -301,14 +305,15 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 	case IIO_PROXIMITY:
 		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
 			dev_err(dev,
-				"proximity: mask value 0x%08lx not supported\n",
-				mask);
+				"%s(): proximity: Mask value 0x%08lx is not supported\n",
+				__func__, mask);
 			break;
 		}
 
 		if (val < 1 || val > 100) {
 			dev_err(dev,
-				"Samp_freq %d is not in range[1:100]\n", val);
+				"%s(): proximity: Sampling frequency %d is not in the range [1:100]\n",
+				__func__, val);
 			break;
 		}
 
@@ -317,21 +322,23 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 	case IIO_LIGHT:
 		if (mask != IIO_CHAN_INFO_SCALE) {
 			dev_err(dev,
-				"light: mask value 0x%08lx not supported\n",
-				mask);
+				"%s(): light: Mask value 0x%08lx is not supported\n",
+				__func__, mask);
 			break;
 		}
 
 		if ((val != 125) && (val != 2000)) {
 			dev_err(dev,
-				"lux scale %d is invalid [125, 2000]\n", val);
+				"%s(): light: Lux scale %d is not in the set {125, 2000}\n",
+				__func__, val);
 			break;
 		}
 
 		ret = isl29028_set_als_scale(chip, val);
 		break;
 	default:
-		dev_err(dev, "Unsupported channel type\n");
+		dev_err(dev, "%s(): Unsupported channel type %x\n",
+			__func__, chan->type);
 		break;
 	}
 
@@ -385,7 +392,8 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 		ret = IIO_VAL_INT;
 		break;
 	default:
-		dev_err(dev, "mask value 0x%08lx not supported\n", mask);
+		dev_err(dev, "%s(): mask value 0x%08lx is not supported\n",
+			__func__, mask);
 		break;
 	}
 
@@ -438,8 +446,8 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
 
 	ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
 	if (ret < 0) {
-		dev_err(dev, "%s(): write to reg %d failed, err = %d\n",
-			__func__, ISL29028_REG_CONFIGURE, ret);
+		dev_err(dev, "%s(): Error %d clearing the CONFIGURE register\n",
+			__func__, ret);
 		return ret;
 	}
 
@@ -493,8 +501,8 @@ static int isl29028_probe(struct i2c_client *client,
 	chip->regmap = devm_regmap_init_i2c(client, &isl29028_regmap_config);
 	if (IS_ERR(chip->regmap)) {
 		ret = PTR_ERR(chip->regmap);
-		dev_err(&client->dev, "regmap initialization failed: %d\n",
-			ret);
+		dev_err(&client->dev, "%s: Error %d initializing regmap\n",
+			__func__, ret);
 		return ret;
 	}
 
@@ -506,16 +514,16 @@ static int isl29028_probe(struct i2c_client *client,
 	ret = regmap_write(chip->regmap, ISL29028_REG_TEST1_MODE, 0x0);
 	if (ret < 0) {
 		dev_err(&client->dev,
-			"%s(): write to reg %d failed, err = %d\n", __func__,
-			ISL29028_REG_TEST1_MODE, ret);
+			"%s(): Error %d writing to TEST1_MODE register\n",
+			__func__, ret);
 		return ret;
 	}
 
 	ret = regmap_write(chip->regmap, ISL29028_REG_TEST2_MODE, 0x0);
 	if (ret < 0) {
 		dev_err(&client->dev,
-			"%s(): write to reg %d failed, err = %d\n", __func__,
-			ISL29028_REG_TEST2_MODE, ret);
+			"%s(): Error %d writing to TEST2_MODE register\n",
+			__func__, ret);
 		return ret;
 	}
 
@@ -535,8 +543,8 @@ static int isl29028_probe(struct i2c_client *client,
 	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev,
-			"iio registration fails with error %d\n",
-			ret);
+			"%s(): iio registration failed with error %d\n",
+			__func__, ret);
 		return ret;
 	}
 
-- 
2.9.3

[PATCH v2 09/15] staging: iio: isl29028: remove unnecessary error logging in isl29028_chip_init_and_power_on()

[Brian Masney]

If the call to isl29028_chip_init_and_power_on() in isl29028_probe()
fails, then isl29028_probe() will log an error message. All of the
error paths in that call path already have error logging in place. This
patch removes the unnecessary logging.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index cffecf9..be1fc4a 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -528,10 +528,8 @@ static int isl29028_probe(struct i2c_client *client,
 	}
 
 	ret = isl29028_chip_init_and_power_on(chip);
-	if (ret < 0) {
-		dev_err(&client->dev, "chip initialization failed: %d\n", ret);
+	if (ret < 0)
 		return ret;
-	}
 
 	indio_dev->info = &isl29028_info;
 	indio_dev->channels = isl29028_channels;
-- 
2.9.3

[PATCH v2 10/15] staging: iio: isl29028: remove out of memory log message

[Brian Masney]

If the call to devm_iio_device_alloc() fails, then isl29028_probe()
logs a message saying that memory cannot be allocated. The user's system
most likely has larger issues at this point. This patch removes that
error message since the error code is passed on and the message is not
necessary.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index be1fc4a..e93077b 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -488,10 +488,8 @@ static int isl29028_probe(struct i2c_client *client,
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
-	if (!indio_dev) {
-		dev_err(&client->dev, "iio allocation fails\n");
+	if (!indio_dev)
 		return -ENOMEM;
-	}
 
 	chip = iio_priv(indio_dev);
 
-- 
2.9.3

[PATCH v2 11/15] staging: iio: isl29028: remove unnecessary parenthesis

[Brian Masney]

isl29028_write_raw() contains unnecessary parenthesis when checking to
see if the passed in lux scale is valid. This patch removes the
unnecessary parenthesis.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index e93077b..bc9c01d 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -327,7 +327,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 			break;
 		}
 
-		if ((val != 125) && (val != 2000)) {
+		if (val != 125 && val != 2000) {
 			dev_err(dev,
 				"%s(): light: Lux scale %d is not in the set {125, 2000}\n",
 				__func__, val);
-- 
2.9.3

[PATCH v2 12/15] staging: iio: isl29028: remove enable flag from isl29028_enable_proximity()

[Brian Masney]

isl29028_enable_proximity() has a boolean argument named enable. This
function is only called once and the enable flag is set to true in that
call. This patch removes the enable parameter from that function.

Signed-off-by: Brian Masney <masneyb@onstation.org>
---
The device gets runtime power management support in the next patch in
this set and autosuspends after two seconds of inactivity. Once the
device suspends, the pin that drives the external LED for proximity
sensing will only go high the next time that the user asks for a
reading from the proximity sensor.

This patch also sets the stage for additional cleanups prior to the
introduction of runtime power management support.

 drivers/staging/iio/light/isl29028.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index bc9c01d..f1b3651 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -103,15 +103,13 @@ static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
 	return ret;
 }
 
-static int isl29028_enable_proximity(struct isl29028_chip *chip, bool enable)
+static int isl29028_enable_proximity(struct isl29028_chip *chip)
 {
 	int ret;
-	int val = 0;
 
-	if (enable)
-		val = ISL29028_CONF_PROX_EN;
 	ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
-				 ISL29028_CONF_PROX_EN_MASK, val);
+				 ISL29028_CONF_PROX_EN_MASK,
+				 ISL29028_CONF_PROX_EN);
 	if (ret < 0)
 		return ret;
 
@@ -225,7 +223,7 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
 	int ret;
 
 	if (!chip->enable_prox) {
-		ret = isl29028_enable_proximity(chip, true);
+		ret = isl29028_enable_proximity(chip);
 		if (ret < 0)
 			return ret;
 
-- 
2.9.3

[PATCH v2 13/15] staging: iio: isl29028: only set proximity sampling rate when proximity is enabled

[Brian Masney]

isl29028_chip_init_and_power_on() calls isl29028_set_proxim_sampling()
and this is not needed until the user actually needs to take a proximity
reading. This patch moves the isl29028_set_proxim_sampling() call from
isl29028_chip_init_and_power_on() to isl29028_enable_proximity().
This sets the stage for faster resume times from the runtime power
management if the user is only querying the ALS/IR sensor.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index f1b3651..fa58d08 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -107,6 +107,10 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip)
 {
 	int ret;
 
+	ret = isl29028_set_proxim_sampling(chip, chip->prox_sampling);
+	if (ret < 0)
+		return ret;
+
 	ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
 				 ISL29028_CONF_PROX_EN_MASK,
 				 ISL29028_CONF_PROX_EN);
@@ -449,10 +453,6 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
 		return ret;
 	}
 
-	ret = isl29028_set_proxim_sampling(chip, chip->prox_sampling);
-	if (ret < 0)
-		return ret;
-
 	return isl29028_set_als_scale(chip, chip->lux_scale);
 }
 
-- 
2.9.3

[PATCH v2 14/15] staging: iio: isl29028: only set ALS scale when ALS/IR sensing is enabled

[Brian Masney]

isl29028_chip_init_and_power_on() calls isl29028_set_als_scale() and
this is not needed until the user actually needs to take a reading from
the ALS/IR sensor. This patch moves the isl29028_set_als_scale() call
from isl29028_chip_init_and_power_on() to isl29028_set_als_ir_mode().
This sets the stage for faster resume times from runtime power
management if the user is only querying the proximity sensor.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index fa58d08..598a5a5 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -146,11 +146,15 @@ static int isl29028_set_als_scale(struct isl29028_chip *chip, int lux_scale)
 static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
 				    enum isl29028_als_ir_mode mode)
 {
-	int ret = 0;
+	int ret;
 
 	if (chip->als_ir_mode == mode)
 		return 0;
 
+	ret = isl29028_set_als_scale(chip, chip->lux_scale);
+	if (ret < 0)
+		return ret;
+
 	switch (mode) {
 	case ISL29028_MODE_ALS:
 		ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
@@ -453,7 +457,7 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
 		return ret;
 	}
 
-	return isl29028_set_als_scale(chip, chip->lux_scale);
+	return ret;
 }
 
 static bool isl29028_is_volatile_reg(struct device *dev, unsigned int reg)
-- 
2.9.3

[PATCH v2 15/15] staging: iio: isl29028: add runtime power management support

[Brian Masney]

This patch adds runtime power management support to the isl29028 driver.
It defaults to powering off the device after two seconds of inactivity.

isl29028_chip_init_and_power_on() currently only zeros the CONFIGURE
register on the chip, which will cause the chip to turn off. This patch
also renames that function to isl29028_clear_configure_reg() since it is
now used in several places.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 598a5a5..a3264f7 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -26,6 +26,7 @@
 #include <linux/regmap.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/pm_runtime.h>
 
 #define ISL29028_CONV_TIME_MS			100
 
@@ -60,6 +61,8 @@
 
 #define ISL29028_NUM_REGS			(ISL29028_REG_TEST2_MODE + 1)
 
+#define ISL29028_POWER_OFF_DELAY_MS		2000
+
 enum isl29028_als_ir_mode {
 	ISL29028_MODE_NONE = 0,
 	ISL29028_MODE_ALS,
@@ -297,6 +300,23 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
 	return isl29028_read_als_ir(chip, ir_data);
 }
 
+static int isl29028_set_pm_runtime_busy(struct isl29028_chip *chip, bool on)
+{
+	struct device *dev = regmap_get_device(chip->regmap);
+	int ret;
+
+	if (on) {
+		ret = pm_runtime_get_sync(dev);
+		if (ret < 0)
+			pm_runtime_put_noidle(dev);
+	} else {
+		pm_runtime_mark_last_busy(dev);
+		ret = pm_runtime_put_autosuspend(dev);
+	}
+
+	return ret;
+}
+
 /* Channel IO */
 static int isl29028_write_raw(struct iio_dev *indio_dev,
 			      struct iio_chan_spec const *chan,
@@ -304,9 +324,15 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 {
 	struct isl29028_chip *chip = iio_priv(indio_dev);
 	struct device *dev = regmap_get_device(chip->regmap);
-	int ret = -EINVAL;
+	int ret;
+
+	ret = isl29028_set_pm_runtime_busy(chip, true);
+	if (ret < 0)
+		return ret;
 
 	mutex_lock(&chip->lock);
+
+	ret = -EINVAL;
 	switch (chan->type) {
 	case IIO_PROXIMITY:
 		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
@@ -350,6 +376,13 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 
 	mutex_unlock(&chip->lock);
 
+	if (ret < 0)
+		return ret;
+
+	ret = isl29028_set_pm_runtime_busy(chip, false);
+	if (ret < 0)
+		return ret;
+
 	return ret;
 }
 
@@ -359,9 +392,15 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 {
 	struct isl29028_chip *chip = iio_priv(indio_dev);
 	struct device *dev = regmap_get_device(chip->regmap);
-	int ret = -EINVAL;
+	int ret, pm_ret;
+
+	ret = isl29028_set_pm_runtime_busy(chip, true);
+	if (ret < 0)
+		return ret;
 
 	mutex_lock(&chip->lock);
+
+	ret = -EINVAL;
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 	case IIO_CHAN_INFO_PROCESSED:
@@ -405,6 +444,18 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 
 	mutex_unlock(&chip->lock);
 
+	if (ret < 0)
+		return ret;
+
+	/**
+	 * Preserve the ret variable if the call to
+	 * isl29028_set_pm_runtime_busy() is successful so the reading
+	 * (if applicable) is returned to user space.
+	 */
+	pm_ret = isl29028_set_pm_runtime_busy(chip, false);
+	if (pm_ret < 0)
+		return pm_ret;
+
 	return ret;
 }
 
@@ -445,17 +496,18 @@ static const struct iio_info isl29028_info = {
 	.write_raw = isl29028_write_raw,
 };
 
-static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
+static int isl29028_clear_configure_reg(struct isl29028_chip *chip)
 {
 	struct device *dev = regmap_get_device(chip->regmap);
 	int ret;
 
 	ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
-	if (ret < 0) {
+	if (ret < 0)
 		dev_err(dev, "%s(): Error %d clearing the CONFIGURE register\n",
 			__func__, ret);
-		return ret;
-	}
+
+	chip->als_ir_mode = ISL29028_MODE_NONE;
+	chip->enable_prox = false;
 
 	return ret;
 }
@@ -509,7 +561,6 @@ static int isl29028_probe(struct i2c_client *client,
 	chip->enable_prox  = false;
 	chip->prox_sampling = 20;
 	chip->lux_scale = 2000;
-	chip->als_ir_mode = ISL29028_MODE_NONE;
 
 	ret = regmap_write(chip->regmap, ISL29028_REG_TEST1_MODE, 0x0);
 	if (ret < 0) {
@@ -527,7 +578,7 @@ static int isl29028_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = isl29028_chip_init_and_power_on(chip);
+	ret = isl29028_clear_configure_reg(chip);
 	if (ret < 0)
 		return ret;
 
@@ -538,6 +589,11 @@ static int isl29028_probe(struct i2c_client *client,
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
+	pm_runtime_enable(&client->dev);
+	pm_runtime_set_autosuspend_delay(&client->dev,
+					 ISL29028_POWER_OFF_DELAY_MS);
+	pm_runtime_use_autosuspend(&client->dev);
+
 	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
 	if (ret < 0) {
 		dev_err(&client->dev,
@@ -549,6 +605,50 @@ static int isl29028_probe(struct i2c_client *client,
 	return 0;
 }
 
+static int isl29028_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct isl29028_chip *chip = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	pm_runtime_disable(&client->dev);
+	pm_runtime_set_suspended(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+
+	return isl29028_clear_configure_reg(chip);
+}
+
+static int __maybe_unused isl29028_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct isl29028_chip *chip = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&chip->lock);
+
+	ret = isl29028_clear_configure_reg(chip);
+
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static int __maybe_unused isl29028_resume(struct device *dev)
+{
+	/**
+	 * The specific component (ALS/IR or proximity) will enable itself as
+	 * needed the next time that the user requests a reading. This is done
+	 * above in isl29028_set_als_ir_mode() and isl29028_enable_proximity().
+	 */
+	return 0;
+}
+
+static const struct dev_pm_ops isl29028_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(isl29028_suspend, isl29028_resume)
+	SET_RUNTIME_PM_OPS(isl29028_suspend, isl29028_resume, NULL)
+};
+
 static const struct i2c_device_id isl29028_id[] = {
 	{"isl29028", 0},
 	{}
@@ -565,9 +665,11 @@ MODULE_DEVICE_TABLE(of, isl29028_of_match);
 static struct i2c_driver isl29028_driver = {
 	.driver  = {
 		.name = "isl29028",
+		.pm = &isl29028_pm_ops,
 		.of_match_table = isl29028_of_match,
 	},
 	.probe	 = isl29028_probe,
+	.remove  = isl29028_remove,
 	.id_table = isl29028_id,
 };
 
-- 
2.9.3

Re: [PATCH v2 01/15] staging: iio: isl29028: made alignment of variables in struct isl29028_chip consistent

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> The alignment of the variables in the struct isl29028_chip is not
> consistent. This changes all of the variables to use consistent
> alignment to improve the code readability.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it. 

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/isl29028.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 1de81f5..4cce663 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -67,13 +67,13 @@ enum isl29028_als_ir_mode {
>  };
>  
>  struct isl29028_chip {
> -	struct mutex		lock;
> -	struct regmap		*regmap;
> +	struct mutex			lock;
> +	struct regmap			*regmap;
>  
> -	unsigned int		prox_sampling;
> -	bool			enable_prox;
> +	unsigned int			prox_sampling;
> +	bool				enable_prox;
>  
> -	int			lux_scale;
> +	int				lux_scale;
>  	enum isl29028_als_ir_mode	als_ir_mode;
>  };
>  
> 

Re: [PATCH v2 02/15] staging: iio: isl29028: fix alignment of function arguments

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> Two separate calls to regmap_update_bits() in isl29028_set_als_scale()
> and isl29028_set_als_ir_mode() did not have their function arguments
> on the next line aligned correctly to the open parenthesis. This patch
> corrects the alignment.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.
> ---
>  drivers/staging/iio/light/isl29028.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 4cce663..74eb736 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -116,7 +116,7 @@ static int isl29028_set_als_scale(struct isl29028_chip *chip, int lux_scale)
>  					ISL29028_CONF_ALS_RANGE_LOW_LUX;
>  
>  	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> -		ISL29028_CONF_ALS_RANGE_MASK, val);
> +				  ISL29028_CONF_ALS_RANGE_MASK, val);
>  }
>  
>  static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
> @@ -148,7 +148,8 @@ static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
>  
>  	case ISL29028_MODE_NONE:
>  		return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> -			ISL29028_CONF_ALS_EN_MASK, ISL29028_CONF_ALS_DIS);
> +					  ISL29028_CONF_ALS_EN_MASK,
> +					  ISL29028_CONF_ALS_DIS);
>  	}
>  
>  	if (ret < 0)
> 

Re: [PATCH v2 06/15] staging: iio: isl29028: move failure logging into isl29028_set_proxim_sampling()

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> When isl29028_set_proxim_sampling() fails, it was up to both callers to
> log the failure message. This patch moves the logging into
> isl29028_set_proxim_sampling() to reduce the overall amount of code in
> the driver.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
I'm not 100% convinced of the logic of this as a stand alone patch, but
it does no harm and is possibly slightly nicer!

Hence applied.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/isl29028.c | 31 ++++++++++++++++---------------
>  1 file changed, 16 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 7074e62..bd85ccb 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -78,18 +78,29 @@ struct isl29028_chip {
>  static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
>  					unsigned int sampling)
>  {
> +	struct device *dev = regmap_get_device(chip->regmap);
>  	static unsigned int prox_period[] = {800, 400, 200, 100, 75, 50, 12, 0};
> -	int sel;
>  	unsigned int period = DIV_ROUND_UP(1000, sampling);
> +	int sel, ret;
>  
>  	for (sel = 0; sel < ARRAY_SIZE(prox_period); ++sel) {
>  		if (period >= prox_period[sel])
>  			break;
>  	}
>  
> -	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> -				  ISL29028_CONF_PROX_SLP_MASK,
> -				  sel << ISL29028_CONF_PROX_SLP_SH);
> +	ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> +				 ISL29028_CONF_PROX_SLP_MASK,
> +				 sel << ISL29028_CONF_PROX_SLP_SH);
> +
> +	if (ret < 0) {
> +		dev_err(dev, "%s(): Error %d setting the proximity sampling\n",
> +			__func__, ret);
> +		return ret;
> +	}
> +
> +	chip->prox_sampling = sampling;
> +
> +	return ret;
>  }
>  
>  static int isl29028_enable_proximity(struct isl29028_chip *chip, bool enable)
> @@ -291,14 +302,6 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  		}
>  
>  		ret = isl29028_set_proxim_sampling(chip, val);
> -		if (ret < 0) {
> -			dev_err(dev,
> -				"Setting proximity samp_freq fail, err %d\n",
> -				ret);
> -			break;
> -		}
> -
> -		chip->prox_sampling = val;
>  		break;
>  	case IIO_LIGHT:
>  		if (mask != IIO_CHAN_INFO_SCALE) {
> @@ -437,10 +440,8 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
>  	}
>  
>  	ret = isl29028_set_proxim_sampling(chip, chip->prox_sampling);
> -	if (ret < 0) {
> -		dev_err(dev, "setting the proximity, err = %d\n", ret);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	ret = isl29028_set_als_scale(chip, chip->lux_scale);
>  	if (ret < 0)
> 

Re: [PATCH v2 03/15] staging: iio: isl29028: combine isl29028_proxim_get() and isl29028_read_proxim()

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> isl29028_proxim_get() checks to see if the promixity needs to be
> enabled on the chip and then calls isl29028_read_proxim(). There
> are no other callers of isl29028_read_proxim(). The naming between
> these two functions can be confusing so this patch combines the
> two to avoid the confusion.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.
> ---
>  drivers/staging/iio/light/isl29028.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 74eb736..a13c8db 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -201,6 +201,13 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
>  	unsigned int data;
>  	int ret;
>  
> +	if (!chip->enable_prox) {
> +		ret = isl29028_enable_proximity(chip, true);
> +		if (ret < 0)
> +			return ret;
> +		chip->enable_prox = true;
> +	}
> +
>  	ret = regmap_read(chip->regmap, ISL29028_REG_PROX_DATA, &data);
>  	if (ret < 0) {
>  		dev_err(dev, "Error in reading register %d, error %d\n",
> @@ -211,19 +218,6 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
>  	return 0;
>  }
>  
> -static int isl29028_proxim_get(struct isl29028_chip *chip, int *prox_data)
> -{
> -	int ret;
> -
> -	if (!chip->enable_prox) {
> -		ret = isl29028_enable_proximity(chip, true);
> -		if (ret < 0)
> -			return ret;
> -		chip->enable_prox = true;
> -	}
> -	return isl29028_read_proxim(chip, prox_data);
> -}
> -
>  static int isl29028_als_get(struct isl29028_chip *chip, int *als_data)
>  {
>  	struct device *dev = regmap_get_device(chip->regmap);
> @@ -349,7 +343,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  			ret = isl29028_ir_get(chip, val);
>  			break;
>  		case IIO_PROXIMITY:
> -			ret = isl29028_proxim_get(chip, val);
> +			ret = isl29028_read_proxim(chip, val);
>  			break;
>  		default:
>  			break;
> 

Re: [PATCH v2 04/15] staging: iio: isl29028: change newlines to improve readability

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> Add and remove newlines to improve code readability in preparation for
> moving the driver out of staging.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.
> ---
>  drivers/staging/iio/light/isl29028.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index a13c8db..d05d761 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -69,10 +69,8 @@ enum isl29028_als_ir_mode {
>  struct isl29028_chip {
>  	struct mutex			lock;
>  	struct regmap			*regmap;
> -
>  	unsigned int			prox_sampling;
>  	bool				enable_prox;
> -
>  	int				lux_scale;
>  	enum isl29028_als_ir_mode	als_ir_mode;
>  };
> @@ -88,6 +86,7 @@ static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
>  		if (period >= prox_period[sel])
>  			break;
>  	}
> +
>  	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
>  				  ISL29028_CONF_PROX_SLP_MASK,
>  				  sel << ISL29028_CONF_PROX_SLP_SH);
> @@ -107,6 +106,7 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip, bool enable)
>  
>  	/* Wait for conversion to be complete for first sample */
>  	mdelay(DIV_ROUND_UP(1000, chip->prox_sampling));
> +
>  	return 0;
>  }
>  
> @@ -139,13 +139,11 @@ static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
>  					 ISL29028_CONF_ALS_RANGE_MASK,
>  					 ISL29028_CONF_ALS_RANGE_HIGH_LUX);
>  		break;
> -
>  	case ISL29028_MODE_IR:
>  		ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
>  					 ISL29028_CONF_ALS_IR_MODE_MASK,
>  					 ISL29028_CONF_ALS_IR_MODE_IR);
>  		break;
> -
>  	case ISL29028_MODE_NONE:
>  		return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
>  					  ISL29028_CONF_ALS_EN_MASK,
> @@ -192,6 +190,7 @@ static int isl29028_read_als_ir(struct isl29028_chip *chip, int *als_ir)
>  	}
>  
>  	*als_ir = ((msb & 0xF) << 8) | (lsb & 0xFF);
> +
>  	return 0;
>  }
>  
> @@ -205,6 +204,7 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
>  		ret = isl29028_enable_proximity(chip, true);
>  		if (ret < 0)
>  			return ret;
> +
>  		chip->enable_prox = true;
>  	}
>  
> @@ -214,7 +214,9 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
>  			ISL29028_REG_PROX_DATA, ret);
>  		return ret;
>  	}
> +
>  	*prox = data;
> +
>  	return 0;
>  }
>  
> @@ -245,6 +247,7 @@ static int isl29028_als_get(struct isl29028_chip *chip, int *als_data)
>  		als_ir_data = (als_ir_data * 49) / 100;
>  
>  	*als_data = als_ir_data;
> +
>  	return 0;
>  }
>  
> @@ -258,6 +261,7 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
>  		dev_err(dev, "Error in enabling IR mode err %d\n", ret);
>  		return ret;
>  	}
> +
>  	return isl29028_read_als_ir(chip, ir_data);
>  }
>  
> @@ -279,11 +283,13 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  				mask);
>  			break;
>  		}
> +
>  		if (val < 1 || val > 100) {
>  			dev_err(dev,
>  				"Samp_freq %d is not in range[1:100]\n", val);
>  			break;
>  		}
> +
>  		ret = isl29028_set_proxim_sampling(chip, val);
>  		if (ret < 0) {
>  			dev_err(dev,
> @@ -291,9 +297,9 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  				ret);
>  			break;
>  		}
> +
>  		chip->prox_sampling = val;
>  		break;
> -
>  	case IIO_LIGHT:
>  		if (mask != IIO_CHAN_INFO_SCALE) {
>  			dev_err(dev,
> @@ -301,25 +307,29 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  				mask);
>  			break;
>  		}
> +
>  		if ((val != 125) && (val != 2000)) {
>  			dev_err(dev,
>  				"lux scale %d is invalid [125, 2000]\n", val);
>  			break;
>  		}
> +
>  		ret = isl29028_set_als_scale(chip, val);
>  		if (ret < 0) {
>  			dev_err(dev,
>  				"Setting lux scale fail with error %d\n", ret);
>  			break;
>  		}
> +
>  		chip->lux_scale = val;
>  		break;
> -
>  	default:
>  		dev_err(dev, "Unsupported channel type\n");
>  		break;
>  	}
> +
>  	mutex_unlock(&chip->lock);
> +
>  	return ret;
>  }
>  
> @@ -348,30 +358,32 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  		default:
>  			break;
>  		}
> +
>  		if (ret < 0)
>  			break;
> +
>  		ret = IIO_VAL_INT;
>  		break;
> -
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		if (chan->type != IIO_PROXIMITY)
>  			break;
> +
>  		*val = chip->prox_sampling;
>  		ret = IIO_VAL_INT;
>  		break;
> -
>  	case IIO_CHAN_INFO_SCALE:
>  		if (chan->type != IIO_LIGHT)
>  			break;
>  		*val = chip->lux_scale;
>  		ret = IIO_VAL_INT;
>  		break;
> -
>  	default:
>  		dev_err(dev, "mask value 0x%08lx not supported\n", mask);
>  		break;
>  	}
> +
>  	mutex_unlock(&chip->lock);
> +
>  	return ret;
>  }
>  
> @@ -497,6 +509,7 @@ static int isl29028_probe(struct i2c_client *client,
>  			ISL29028_REG_TEST1_MODE, ret);
>  		return ret;
>  	}
> +
>  	ret = regmap_write(chip->regmap, ISL29028_REG_TEST2_MODE, 0x0);
>  	if (ret < 0) {
>  		dev_err(&client->dev,
> @@ -517,6 +530,7 @@ static int isl29028_probe(struct i2c_client *client,
>  	indio_dev->name = id->name;
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> +
>  	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev,
> @@ -524,6 +538,7 @@ static int isl29028_probe(struct i2c_client *client,
>  			ret);
>  		return ret;
>  	}
> +
>  	return 0;
>  }
>  
> 

Re: [PATCH v2 05/15] staging: iio: isl29028: remove unused define ISL29028_DEV_ATTR

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> The #define ISL29028_DEV_ATTR was not used so this patch removes the
> unnecessary code.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.
> ---
>  drivers/staging/iio/light/isl29028.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index d05d761..7074e62 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -391,7 +391,6 @@ static IIO_CONST_ATTR(in_proximity_sampling_frequency_available,
>  				"1 3 5 10 13 20 83 100");
>  static IIO_CONST_ATTR(in_illuminance_scale_available, "125 2000");
>  
> -#define ISL29028_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
>  #define ISL29028_CONST_ATTR(name) (&iio_const_attr_##name.dev_attr.attr)
>  static struct attribute *isl29028_attributes[] = {
>  	ISL29028_CONST_ATTR(in_proximity_sampling_frequency_available),
> 

Re: [PATCH v2 07/15] staging: iio: isl29028: move failure logging into isl29028_set_als_scale()

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> When isl29028_set_als_scale() fails, it was up to both callers to log
> the failure message. This patch moves the logging into
> isl29028_set_als_scale() to reduce the overall amount of code in the
> driver.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied
> ---
>  drivers/staging/iio/light/isl29028.c | 27 ++++++++++++++-------------
>  1 file changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index bd85ccb..8e7b3db 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -123,11 +123,22 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip, bool enable)
>  
>  static int isl29028_set_als_scale(struct isl29028_chip *chip, int lux_scale)
>  {
> +	struct device *dev = regmap_get_device(chip->regmap);
>  	int val = (lux_scale == 2000) ? ISL29028_CONF_ALS_RANGE_HIGH_LUX :
>  					ISL29028_CONF_ALS_RANGE_LOW_LUX;
> +	int ret;
> +
> +	ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> +				 ISL29028_CONF_ALS_RANGE_MASK, val);
> +	if (ret < 0) {
> +		dev_err(dev, "%s(): Error %d setting the ALS scale\n", __func__,
> +			ret);
> +		return ret;
> +	}
>  
> -	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> -				  ISL29028_CONF_ALS_RANGE_MASK, val);
> +	chip->lux_scale = lux_scale;
> +
> +	return ret;
>  }
>  
>  static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
> @@ -318,13 +329,6 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  		}
>  
>  		ret = isl29028_set_als_scale(chip, val);
> -		if (ret < 0) {
> -			dev_err(dev,
> -				"Setting lux scale fail with error %d\n", ret);
> -			break;
> -		}
> -
> -		chip->lux_scale = val;
>  		break;
>  	default:
>  		dev_err(dev, "Unsupported channel type\n");
> @@ -443,10 +447,7 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
>  	if (ret < 0)
>  		return ret;
>  
> -	ret = isl29028_set_als_scale(chip, chip->lux_scale);
> -	if (ret < 0)
> -		dev_err(dev, "setting als scale failed, err = %d\n", ret);
> -	return ret;
> +	return isl29028_set_als_scale(chip, chip->lux_scale);
>  }
>  
>  static bool isl29028_is_volatile_reg(struct device *dev, unsigned int reg)
> 

Re: [PATCH v2 08/15] staging: iio: isl29028: made error messages consistent

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> The wording and style of the different error messages was not
> consistent. This patch makes the wording and style consistent
> throughout the driver.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.
> ---
>  drivers/staging/iio/light/isl29028.c | 56 ++++++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 8e7b3db..cffecf9 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -200,14 +200,16 @@ static int isl29028_read_als_ir(struct isl29028_chip *chip, int *als_ir)
>  	ret = regmap_read(chip->regmap, ISL29028_REG_ALSIR_L, &lsb);
>  	if (ret < 0) {
>  		dev_err(dev,
> -			"Error in reading register ALSIR_L err %d\n", ret);
> +			"%s(): Error %d reading register ALSIR_L\n",
> +			__func__, ret);
>  		return ret;
>  	}
>  
>  	ret = regmap_read(chip->regmap, ISL29028_REG_ALSIR_U, &msb);
>  	if (ret < 0) {
>  		dev_err(dev,
> -			"Error in reading register ALSIR_U err %d\n", ret);
> +			"%s(): Error %d reading register ALSIR_U\n",
> +			__func__, ret);
>  		return ret;
>  	}
>  
> @@ -232,8 +234,8 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
>  
>  	ret = regmap_read(chip->regmap, ISL29028_REG_PROX_DATA, &data);
>  	if (ret < 0) {
> -		dev_err(dev, "Error in reading register %d, error %d\n",
> -			ISL29028_REG_PROX_DATA, ret);
> +		dev_err(dev, "%s(): Error %d reading register PROX_DATA\n",
> +			__func__, ret);
>  		return ret;
>  	}
>  
> @@ -250,7 +252,8 @@ static int isl29028_als_get(struct isl29028_chip *chip, int *als_data)
>  
>  	ret = isl29028_set_als_ir_mode(chip, ISL29028_MODE_ALS);
>  	if (ret < 0) {
> -		dev_err(dev, "Error in enabling ALS mode err %d\n", ret);
> +		dev_err(dev, "%s(): Error %d enabling ALS mode\n", __func__,
> +			ret);
>  		return ret;
>  	}
>  
> @@ -280,7 +283,8 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
>  
>  	ret = isl29028_set_als_ir_mode(chip, ISL29028_MODE_IR);
>  	if (ret < 0) {
> -		dev_err(dev, "Error in enabling IR mode err %d\n", ret);
> +		dev_err(dev, "%s(): Error %d enabling IR mode\n", __func__,
> +			ret);
>  		return ret;
>  	}
>  
> @@ -301,14 +305,15 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  	case IIO_PROXIMITY:
>  		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
>  			dev_err(dev,
> -				"proximity: mask value 0x%08lx not supported\n",
> -				mask);
> +				"%s(): proximity: Mask value 0x%08lx is not supported\n",
> +				__func__, mask);
>  			break;
>  		}
>  
>  		if (val < 1 || val > 100) {
>  			dev_err(dev,
> -				"Samp_freq %d is not in range[1:100]\n", val);
> +				"%s(): proximity: Sampling frequency %d is not in the range [1:100]\n",
> +				__func__, val);
>  			break;
>  		}
>  
> @@ -317,21 +322,23 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  	case IIO_LIGHT:
>  		if (mask != IIO_CHAN_INFO_SCALE) {
>  			dev_err(dev,
> -				"light: mask value 0x%08lx not supported\n",
> -				mask);
> +				"%s(): light: Mask value 0x%08lx is not supported\n",
> +				__func__, mask);
>  			break;
>  		}
>  
>  		if ((val != 125) && (val != 2000)) {
>  			dev_err(dev,
> -				"lux scale %d is invalid [125, 2000]\n", val);
> +				"%s(): light: Lux scale %d is not in the set {125, 2000}\n",
> +				__func__, val);
>  			break;
>  		}
>  
>  		ret = isl29028_set_als_scale(chip, val);
>  		break;
>  	default:
> -		dev_err(dev, "Unsupported channel type\n");
> +		dev_err(dev, "%s(): Unsupported channel type %x\n",
> +			__func__, chan->type);
>  		break;
>  	}
>  
> @@ -385,7 +392,8 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  		ret = IIO_VAL_INT;
>  		break;
>  	default:
> -		dev_err(dev, "mask value 0x%08lx not supported\n", mask);
> +		dev_err(dev, "%s(): mask value 0x%08lx is not supported\n",
> +			__func__, mask);
>  		break;
>  	}
>  
> @@ -438,8 +446,8 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
>  
>  	ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
>  	if (ret < 0) {
> -		dev_err(dev, "%s(): write to reg %d failed, err = %d\n",
> -			__func__, ISL29028_REG_CONFIGURE, ret);
> +		dev_err(dev, "%s(): Error %d clearing the CONFIGURE register\n",
> +			__func__, ret);
>  		return ret;
>  	}
>  
> @@ -493,8 +501,8 @@ static int isl29028_probe(struct i2c_client *client,
>  	chip->regmap = devm_regmap_init_i2c(client, &isl29028_regmap_config);
>  	if (IS_ERR(chip->regmap)) {
>  		ret = PTR_ERR(chip->regmap);
> -		dev_err(&client->dev, "regmap initialization failed: %d\n",
> -			ret);
> +		dev_err(&client->dev, "%s: Error %d initializing regmap\n",
> +			__func__, ret);
>  		return ret;
>  	}
>  
> @@ -506,16 +514,16 @@ static int isl29028_probe(struct i2c_client *client,
>  	ret = regmap_write(chip->regmap, ISL29028_REG_TEST1_MODE, 0x0);
>  	if (ret < 0) {
>  		dev_err(&client->dev,
> -			"%s(): write to reg %d failed, err = %d\n", __func__,
> -			ISL29028_REG_TEST1_MODE, ret);
> +			"%s(): Error %d writing to TEST1_MODE register\n",
> +			__func__, ret);
>  		return ret;
>  	}
>  
>  	ret = regmap_write(chip->regmap, ISL29028_REG_TEST2_MODE, 0x0);
>  	if (ret < 0) {
>  		dev_err(&client->dev,
> -			"%s(): write to reg %d failed, err = %d\n", __func__,
> -			ISL29028_REG_TEST2_MODE, ret);
> +			"%s(): Error %d writing to TEST2_MODE register\n",
> +			__func__, ret);
>  		return ret;
>  	}
>  
> @@ -535,8 +543,8 @@ static int isl29028_probe(struct i2c_client *client,
>  	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev,
> -			"iio registration fails with error %d\n",
> -			ret);
> +			"%s(): iio registration failed with error %d\n",
> +			__func__, ret);
>  		return ret;
>  	}
>  
> 

Re: [PATCH v2 09/15] staging: iio: isl29028: remove unnecessary error logging in isl29028_chip_init_and_power_on()

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> If the call to isl29028_chip_init_and_power_on() in isl29028_probe()
> fails, then isl29028_probe() will log an error message. All of the
> error paths in that call path already have error logging in place. This
> patch removes the unnecessary logging.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
applied.
> ---
>  drivers/staging/iio/light/isl29028.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index cffecf9..be1fc4a 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -528,10 +528,8 @@ static int isl29028_probe(struct i2c_client *client,
>  	}
>  
>  	ret = isl29028_chip_init_and_power_on(chip);
> -	if (ret < 0) {
> -		dev_err(&client->dev, "chip initialization failed: %d\n", ret);
> +	if (ret < 0)
>  		return ret;
> -	}
>  
>  	indio_dev->info = &isl29028_info;
>  	indio_dev->channels = isl29028_channels;
> 

Re: [PATCH v2 10/15] staging: iio: isl29028: remove out of memory log message

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> If the call to devm_iio_device_alloc() fails, then isl29028_probe()
> logs a message saying that memory cannot be allocated. The user's system
> most likely has larger issues at this point. This patch removes that
> error message since the error code is passed on and the message is not
> necessary.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.
> ---
>  drivers/staging/iio/light/isl29028.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index be1fc4a..e93077b 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -488,10 +488,8 @@ static int isl29028_probe(struct i2c_client *client,
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip));
> -	if (!indio_dev) {
> -		dev_err(&client->dev, "iio allocation fails\n");
> +	if (!indio_dev)
>  		return -ENOMEM;
> -	}
>  
>  	chip = iio_priv(indio_dev);
>  
> 

Re: [PATCH v2 11/15] staging: iio: isl29028: remove unnecessary parenthesis

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> isl29028_write_raw() contains unnecessary parenthesis when checking to
> see if the passed in lux scale is valid. This patch removes the
> unnecessary parenthesis.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
hmm. Not sure I really care about this either way.

Ah well, applied.

Jonathan
> ---
>  drivers/staging/iio/light/isl29028.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index e93077b..bc9c01d 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -327,7 +327,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  			break;
>  		}
>  
> -		if ((val != 125) && (val != 2000)) {
> +		if (val != 125 && val != 2000) {
>  			dev_err(dev,
>  				"%s(): light: Lux scale %d is not in the set {125, 2000}\n",
>  				__func__, val);
> 

Re: [PATCH v2 12/15] staging: iio: isl29028: remove enable flag from isl29028_enable_proximity()

[Jonathan Cameron]

On 17/01/17 09:24, Brian Masney wrote:
> isl29028_enable_proximity() has a boolean argument named enable. This
> function is only called once and the enable flag is set to true in that
> call. This patch removes the enable parameter from that function.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Applied.
> ---
> The device gets runtime power management support in the next patch in
> this set and autosuspends after two seconds of inactivity. Once the
> device suspends, the pin that drives the external LED for proximity
> sensing will only go high the next time that the user asks for a
> reading from the proximity sensor.
> 
> This patch also sets the stage for additional cleanups prior to the
> introduction of runtime power management support.
> 
>  drivers/staging/iio/light/isl29028.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index bc9c01d..f1b3651 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -103,15 +103,13 @@ static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
>  	return ret;
>  }
>  
> -static int isl29028_enable_proximity(struct isl29028_chip *chip, bool enable)
> +static int isl29028_enable_proximity(struct isl29028_chip *chip)
>  {
>  	int ret;
> -	int val = 0;
>  
> -	if (enable)
> -		val = ISL29028_CONF_PROX_EN;
>  	ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> -				 ISL29028_CONF_PROX_EN_MASK, val);
> +				 ISL29028_CONF_PROX_EN_MASK,
> +				 ISL29028_CONF_PROX_EN);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -225,7 +223,7 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
>  	int ret;
>  
>  	if (!chip->enable_prox) {
> -		ret = isl29028_enable_proximity(chip, true);
> +		ret = isl29028_enable_proximity(chip);
>  		if (ret < 0)
>  			return ret;
>  
> 

Re: [PATCH v2 13/15] staging: iio: isl29028: only set proximity sampling rate when proximity is enabled

[Jonathan Cameron]

On 17/01/17 09:25, Brian Masney wrote:
> isl29028_chip_init_and_power_on() calls isl29028_set_proxim_sampling()
> and this is not needed until the user actually needs to take a proximity
> reading. This patch moves the isl29028_set_proxim_sampling() call from
> isl29028_chip_init_and_power_on() to isl29028_enable_proximity().
> This sets the stage for faster resume times from the runtime power
> management if the user is only querying the ALS/IR sensor.
> 
Logical.  Applied.
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/staging/iio/light/isl29028.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index f1b3651..fa58d08 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -107,6 +107,10 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip)
>  {
>  	int ret;
>  
> +	ret = isl29028_set_proxim_sampling(chip, chip->prox_sampling);
> +	if (ret < 0)
> +		return ret;
> +
>  	ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
>  				 ISL29028_CONF_PROX_EN_MASK,
>  				 ISL29028_CONF_PROX_EN);
> @@ -449,10 +453,6 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
>  		return ret;
>  	}
>  
> -	ret = isl29028_set_proxim_sampling(chip, chip->prox_sampling);
> -	if (ret < 0)
> -		return ret;
> -
>  	return isl29028_set_als_scale(chip, chip->lux_scale);
>  }
>  
> 

Re: [PATCH v2 14/15] staging: iio: isl29028: only set ALS scale when ALS/IR sensing is enabled

[Jonathan Cameron]

On 17/01/17 09:25, Brian Masney wrote:
> isl29028_chip_init_and_power_on() calls isl29028_set_als_scale() and
> this is not needed until the user actually needs to take a reading from
> the ALS/IR sensor. This patch moves the isl29028_set_als_scale() call
> from isl29028_chip_init_and_power_on() to isl29028_set_als_ir_mode().
> This sets the stage for faster resume times from runtime power
> management if the user is only querying the proximity sensor.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Makes sense.
> ---
>  drivers/staging/iio/light/isl29028.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index fa58d08..598a5a5 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -146,11 +146,15 @@ static int isl29028_set_als_scale(struct isl29028_chip *chip, int lux_scale)
>  static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
>  				    enum isl29028_als_ir_mode mode)
>  {
> -	int ret = 0;
> +	int ret;
>  
>  	if (chip->als_ir_mode == mode)
>  		return 0;
>  
> +	ret = isl29028_set_als_scale(chip, chip->lux_scale);
> +	if (ret < 0)
> +		return ret;
> +
>  	switch (mode) {
>  	case ISL29028_MODE_ALS:
>  		ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> @@ -453,7 +457,7 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
>  		return ret;
>  	}
>  
> -	return isl29028_set_als_scale(chip, chip->lux_scale);
> +	return ret;
>  }
>  
>  static bool isl29028_is_volatile_reg(struct device *dev, unsigned int reg)
> 

Re: [PATCH v2 15/15] staging: iio: isl29028: add runtime power management support

[Jonathan Cameron]

On 17/01/17 09:25, Brian Masney wrote:
> This patch adds runtime power management support to the isl29028 driver.
> It defaults to powering off the device after two seconds of inactivity.
> 
> isl29028_chip_init_and_power_on() currently only zeros the CONFIGURE
> register on the chip, which will cause the chip to turn off. This patch
> also renames that function to isl29028_clear_configure_reg() since it is
> now used in several places.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Whilst I'm not against it by an means, runtime PM is hardly a requirement
for moving out of staging! Good stuff though so I'm not going to turn it
down ;)

I'm not 100% sure about the one comment I made at the end.
It's a very unlikely to occur condition anyway, but if you want to follow up
with a patch on that then feel free.

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

thanks,

Jonathan
> ---
>  drivers/staging/iio/light/isl29028.c | 118 ++++++++++++++++++++++++++++++++---
>  1 file changed, 110 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 598a5a5..a3264f7 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -26,6 +26,7 @@
>  #include <linux/regmap.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/pm_runtime.h>
>  
>  #define ISL29028_CONV_TIME_MS			100
>  
> @@ -60,6 +61,8 @@
>  
>  #define ISL29028_NUM_REGS			(ISL29028_REG_TEST2_MODE + 1)
>  
> +#define ISL29028_POWER_OFF_DELAY_MS		2000
> +
>  enum isl29028_als_ir_mode {
>  	ISL29028_MODE_NONE = 0,
>  	ISL29028_MODE_ALS,
> @@ -297,6 +300,23 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
>  	return isl29028_read_als_ir(chip, ir_data);
>  }
>  
> +static int isl29028_set_pm_runtime_busy(struct isl29028_chip *chip, bool on)
> +{
> +	struct device *dev = regmap_get_device(chip->regmap);
> +	int ret;
> +
> +	if (on) {
> +		ret = pm_runtime_get_sync(dev);
> +		if (ret < 0)
> +			pm_runtime_put_noidle(dev);
> +	} else {
> +		pm_runtime_mark_last_busy(dev);
> +		ret = pm_runtime_put_autosuspend(dev);
> +	}
> +
> +	return ret;
> +}
> +
>  /* Channel IO */
>  static int isl29028_write_raw(struct iio_dev *indio_dev,
>  			      struct iio_chan_spec const *chan,
> @@ -304,9 +324,15 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  {
>  	struct isl29028_chip *chip = iio_priv(indio_dev);
>  	struct device *dev = regmap_get_device(chip->regmap);
> -	int ret = -EINVAL;
> +	int ret;
> +
> +	ret = isl29028_set_pm_runtime_busy(chip, true);
> +	if (ret < 0)
> +		return ret;
>  
>  	mutex_lock(&chip->lock);
> +
> +	ret = -EINVAL;
>  	switch (chan->type) {
>  	case IIO_PROXIMITY:
>  		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
> @@ -350,6 +376,13 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  
>  	mutex_unlock(&chip->lock);
>  
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = isl29028_set_pm_runtime_busy(chip, false);
> +	if (ret < 0)
> +		return ret;
> +
>  	return ret;
>  }
>  
> @@ -359,9 +392,15 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  {
>  	struct isl29028_chip *chip = iio_priv(indio_dev);
>  	struct device *dev = regmap_get_device(chip->regmap);
> -	int ret = -EINVAL;
> +	int ret, pm_ret;
> +
> +	ret = isl29028_set_pm_runtime_busy(chip, true);
> +	if (ret < 0)
> +		return ret;
>  
>  	mutex_lock(&chip->lock);
> +
> +	ret = -EINVAL;
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  	case IIO_CHAN_INFO_PROCESSED:
> @@ -405,6 +444,18 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  
>  	mutex_unlock(&chip->lock);
>  
> +	if (ret < 0)
> +		return ret;
> +
> +	/**
> +	 * Preserve the ret variable if the call to
> +	 * isl29028_set_pm_runtime_busy() is successful so the reading
> +	 * (if applicable) is returned to user space.
> +	 */
> +	pm_ret = isl29028_set_pm_runtime_busy(chip, false);
> +	if (pm_ret < 0)
> +		return pm_ret;
> +
>  	return ret;
>  }
>  
> @@ -445,17 +496,18 @@ static const struct iio_info isl29028_info = {
>  	.write_raw = isl29028_write_raw,
>  };
>  
> -static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
> +static int isl29028_clear_configure_reg(struct isl29028_chip *chip)
>  {
>  	struct device *dev = regmap_get_device(chip->regmap);
>  	int ret;
>  
>  	ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
> -	if (ret < 0) {
> +	if (ret < 0)
>  		dev_err(dev, "%s(): Error %d clearing the CONFIGURE register\n",
>  			__func__, ret);
> -		return ret;
> -	}
> +
> +	chip->als_ir_mode = ISL29028_MODE_NONE;
> +	chip->enable_prox = false;
>  
>  	return ret;
>  }
> @@ -509,7 +561,6 @@ static int isl29028_probe(struct i2c_client *client,
>  	chip->enable_prox  = false;
>  	chip->prox_sampling = 20;
>  	chip->lux_scale = 2000;
> -	chip->als_ir_mode = ISL29028_MODE_NONE;
>  
>  	ret = regmap_write(chip->regmap, ISL29028_REG_TEST1_MODE, 0x0);
>  	if (ret < 0) {
> @@ -527,7 +578,7 @@ static int isl29028_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ret = isl29028_chip_init_and_power_on(chip);
> +	ret = isl29028_clear_configure_reg(chip);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -538,6 +589,11 @@ static int isl29028_probe(struct i2c_client *client,
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> +	pm_runtime_enable(&client->dev);
> +	pm_runtime_set_autosuspend_delay(&client->dev,
> +					 ISL29028_POWER_OFF_DELAY_MS);
> +	pm_runtime_use_autosuspend(&client->dev);
> +
>  	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>  	if (ret < 0) {
>  		dev_err(&client->dev,
> @@ -549,6 +605,50 @@ static int isl29028_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +static int isl29028_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct isl29028_chip *chip = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	pm_runtime_disable(&client->dev);
> +	pm_runtime_set_suspended(&client->dev);
> +	pm_runtime_put_noidle(&client->dev);
> +
> +	return isl29028_clear_configure_reg(chip);
> +}
> +
> +static int __maybe_unused isl29028_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct isl29028_chip *chip = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&chip->lock);
> +
> +	ret = isl29028_clear_configure_reg(chip);
I'm stretching a bit, but there might be a race here.
If we get a suspend and resume in under the time it takes for
the auto suspend to conclude it is good to suspend, it might think
the device is still ready to go when it isn't.
> +
> +	mutex_unlock(&chip->lock);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused isl29028_resume(struct device *dev)
> +{
> +	/**
> +	 * The specific component (ALS/IR or proximity) will enable itself as
> +	 * needed the next time that the user requests a reading. This is done
> +	 * above in isl29028_set_als_ir_mode() and isl29028_enable_proximity().
> +	 */
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops isl29028_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(isl29028_suspend, isl29028_resume)
> +	SET_RUNTIME_PM_OPS(isl29028_suspend, isl29028_resume, NULL)
> +};
> +
>  static const struct i2c_device_id isl29028_id[] = {
>  	{"isl29028", 0},
>  	{}
> @@ -565,9 +665,11 @@ MODULE_DEVICE_TABLE(of, isl29028_of_match);
>  static struct i2c_driver isl29028_driver = {
>  	.driver  = {
>  		.name = "isl29028",
> +		.pm = &isl29028_pm_ops,
>  		.of_match_table = isl29028_of_match,
>  	},
>  	.probe	 = isl29028_probe,
> +	.remove  = isl29028_remove,
>  	.id_table = isl29028_id,
>  };
>  
> 

Re: [PATCH v2 15/15] staging: iio: isl29028: add runtime power management support

[Brian Masney]

On Sat, Jan 21, 2017 at 02:58:12PM +0000, Jonathan Cameron wrote:
> On 17/01/17 09:25, Brian Masney wrote:
> > This patch adds runtime power management support to the isl29028 driver.
> > It defaults to powering off the device after two seconds of inactivity.
> > 
> > isl29028_chip_init_and_power_on() currently only zeros the CONFIGURE
> > register on the chip, which will cause the chip to turn off. This patch
> > also renames that function to isl29028_clear_configure_reg() since it is
> > now used in several places.
> > 
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> Whilst I'm not against it by an means, runtime PM is hardly a requirement
> for moving out of staging! Good stuff though so I'm not going to turn it
> down ;)

Thanks. I know it wasn't a requirement for a staging graduation but I
did it just for the fun of it to learn more about power management in
the kernel.

> I'm not 100% sure about the one comment I made at the end.
> It's a very unlikely to occur condition anyway, but if you want to follow up
> with a patch on that then feel free.

See below.

> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to play with it.
> 
> thanks,
> 
> Jonathan
> > ---
> >  drivers/staging/iio/light/isl29028.c | 118 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 110 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> > index 598a5a5..a3264f7 100644
> > --- a/drivers/staging/iio/light/isl29028.c
> > +++ b/drivers/staging/iio/light/isl29028.c
> > @@ -26,6 +26,7 @@
> >  #include <linux/regmap.h>
> >  #include <linux/iio/iio.h>
> >  #include <linux/iio/sysfs.h>
> > +#include <linux/pm_runtime.h>
> >  
> >  #define ISL29028_CONV_TIME_MS			100
> >  
> > @@ -60,6 +61,8 @@
> >  
> >  #define ISL29028_NUM_REGS			(ISL29028_REG_TEST2_MODE + 1)
> >  
> > +#define ISL29028_POWER_OFF_DELAY_MS		2000
> > +
> >  enum isl29028_als_ir_mode {
> >  	ISL29028_MODE_NONE = 0,
> >  	ISL29028_MODE_ALS,
> > @@ -297,6 +300,23 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
> >  	return isl29028_read_als_ir(chip, ir_data);
> >  }
> >  
> > +static int isl29028_set_pm_runtime_busy(struct isl29028_chip *chip, bool on)
> > +{
> > +	struct device *dev = regmap_get_device(chip->regmap);
> > +	int ret;
> > +
> > +	if (on) {
> > +		ret = pm_runtime_get_sync(dev);
> > +		if (ret < 0)
> > +			pm_runtime_put_noidle(dev);
> > +	} else {
> > +		pm_runtime_mark_last_busy(dev);
> > +		ret = pm_runtime_put_autosuspend(dev);
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  /* Channel IO */
> >  static int isl29028_write_raw(struct iio_dev *indio_dev,
> >  			      struct iio_chan_spec const *chan,
> > @@ -304,9 +324,15 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
> >  {
> >  	struct isl29028_chip *chip = iio_priv(indio_dev);
> >  	struct device *dev = regmap_get_device(chip->regmap);
> > -	int ret = -EINVAL;
> > +	int ret;
> > +
> > +	ret = isl29028_set_pm_runtime_busy(chip, true);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	mutex_lock(&chip->lock);
> > +
> > +	ret = -EINVAL;
> >  	switch (chan->type) {
> >  	case IIO_PROXIMITY:
> >  		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
> > @@ -350,6 +376,13 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
> >  
> >  	mutex_unlock(&chip->lock);
> >  
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = isl29028_set_pm_runtime_busy(chip, false);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	return ret;
> >  }
> >  
> > @@ -359,9 +392,15 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
> >  {
> >  	struct isl29028_chip *chip = iio_priv(indio_dev);
> >  	struct device *dev = regmap_get_device(chip->regmap);
> > -	int ret = -EINVAL;
> > +	int ret, pm_ret;
> > +
> > +	ret = isl29028_set_pm_runtime_busy(chip, true);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	mutex_lock(&chip->lock);
> > +
> > +	ret = -EINVAL;
> >  	switch (mask) {
> >  	case IIO_CHAN_INFO_RAW:
> >  	case IIO_CHAN_INFO_PROCESSED:
> > @@ -405,6 +444,18 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
> >  
> >  	mutex_unlock(&chip->lock);
> >  
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/**
> > +	 * Preserve the ret variable if the call to
> > +	 * isl29028_set_pm_runtime_busy() is successful so the reading
> > +	 * (if applicable) is returned to user space.
> > +	 */
> > +	pm_ret = isl29028_set_pm_runtime_busy(chip, false);
> > +	if (pm_ret < 0)
> > +		return pm_ret;
> > +
> >  	return ret;
> >  }
> >  
> > @@ -445,17 +496,18 @@ static const struct iio_info isl29028_info = {
> >  	.write_raw = isl29028_write_raw,
> >  };
> >  
> > -static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
> > +static int isl29028_clear_configure_reg(struct isl29028_chip *chip)
> >  {
> >  	struct device *dev = regmap_get_device(chip->regmap);
> >  	int ret;
> >  
> >  	ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
> > -	if (ret < 0) {
> > +	if (ret < 0)
> >  		dev_err(dev, "%s(): Error %d clearing the CONFIGURE register\n",
> >  			__func__, ret);
> > -		return ret;
> > -	}
> > +
> > +	chip->als_ir_mode = ISL29028_MODE_NONE;
> > +	chip->enable_prox = false;
> >  
> >  	return ret;
> >  }
> > @@ -509,7 +561,6 @@ static int isl29028_probe(struct i2c_client *client,
> >  	chip->enable_prox  = false;
> >  	chip->prox_sampling = 20;
> >  	chip->lux_scale = 2000;
> > -	chip->als_ir_mode = ISL29028_MODE_NONE;
> >  
> >  	ret = regmap_write(chip->regmap, ISL29028_REG_TEST1_MODE, 0x0);
> >  	if (ret < 0) {
> > @@ -527,7 +578,7 @@ static int isl29028_probe(struct i2c_client *client,
> >  		return ret;
> >  	}
> >  
> > -	ret = isl29028_chip_init_and_power_on(chip);
> > +	ret = isl29028_clear_configure_reg(chip);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > @@ -538,6 +589,11 @@ static int isl29028_probe(struct i2c_client *client,
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->modes = INDIO_DIRECT_MODE;
> >  
> > +	pm_runtime_enable(&client->dev);
> > +	pm_runtime_set_autosuspend_delay(&client->dev,
> > +					 ISL29028_POWER_OFF_DELAY_MS);
> > +	pm_runtime_use_autosuspend(&client->dev);
> > +
> >  	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> >  	if (ret < 0) {
> >  		dev_err(&client->dev,
> > @@ -549,6 +605,50 @@ static int isl29028_probe(struct i2c_client *client,
> >  	return 0;
> >  }
> >  
> > +static int isl29028_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +	struct isl29028_chip *chip = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +
> > +	pm_runtime_disable(&client->dev);
> > +	pm_runtime_set_suspended(&client->dev);
> > +	pm_runtime_put_noidle(&client->dev);
> > +
> > +	return isl29028_clear_configure_reg(chip);
> > +}
> > +
> > +static int __maybe_unused isl29028_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct isl29028_chip *chip = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&chip->lock);
> > +
> > +	ret = isl29028_clear_configure_reg(chip);
> I'm stretching a bit, but there might be a race here.
> If we get a suspend and resume in under the time it takes for
> the auto suspend to conclude it is good to suspend, it might think
> the device is still ready to go when it isn't.

I don't believe that there is a race condition in that situation since
all of the i2c calls outside of the probe and remove functions come from
either isl29028_write_raw(), isl29028_read_raw(), or isl29028_suspend().
All of three of those functions lock a common mutex.

I just noticed that isl29028_remove() doesn't lock that shared mutex
when the device is removed and the chip is powered off. This is
unlikely to be an issue but I'll send a follow up patch in my next
series fixing that just to be sure.

Brian

Re: [PATCH v2 15/15] staging: iio: isl29028: add runtime power management support

[Jonathan Cameron]

On 21 January 2017 16:49:09 GMT+00:00, Brian Masney <masneyb@onstation.org> wrote:
>On Sat, Jan 21, 2017 at 02:58:12PM +0000, Jonathan Cameron wrote:
>> On 17/01/17 09:25, Brian Masney wrote:
>> > This patch adds runtime power management support to the isl29028
>driver.
>> > It defaults to powering off the device after two seconds of
>inactivity.
>> > 
>> > isl29028_chip_init_and_power_on() currently only zeros the
>CONFIGURE
>> > register on the chip, which will cause the chip to turn off. This
>patch
>> > also renames that function to isl29028_clear_configure_reg() since
>it is
>> > now used in several places.
>> > 
>> > Signed-off-by: Brian Masney <masneyb@onstation.org>
>> Whilst I'm not against it by an means, runtime PM is hardly a
>requirement
>> for moving out of staging! Good stuff though so I'm not going to turn
>it
>> down ;)
>
>Thanks. I know it wasn't a requirement for a staging graduation but I
>did it just for the fun of it to learn more about power management in
>the kernel.
>
>> I'm not 100% sure about the one comment I made at the end.
>> It's a very unlikely to occur condition anyway, but if you want to
>follow up
>> with a patch on that then feel free.
>
>See below.
>
>> Applied to the togreg branch of iio.git and pushed out as testing for
>> the autobuilders to play with it.
>> 
>> thanks,
>> 
>> Jonathan
>> > ---
>> >  drivers/staging/iio/light/isl29028.c | 118
>++++++++++++++++++++++++++++++++---
>> >  1 file changed, 110 insertions(+), 8 deletions(-)
>> > 
>> > diff --git a/drivers/staging/iio/light/isl29028.c
>b/drivers/staging/iio/light/isl29028.c
>> > index 598a5a5..a3264f7 100644
>> > --- a/drivers/staging/iio/light/isl29028.c
>> > +++ b/drivers/staging/iio/light/isl29028.c
>> > @@ -26,6 +26,7 @@
>> >  #include <linux/regmap.h>
>> >  #include <linux/iio/iio.h>
>> >  #include <linux/iio/sysfs.h>
>> > +#include <linux/pm_runtime.h>
>> >  
>> >  #define ISL29028_CONV_TIME_MS			100
>> >  
>> > @@ -60,6 +61,8 @@
>> >  
>> >  #define ISL29028_NUM_REGS			(ISL29028_REG_TEST2_MODE + 1)
>> >  
>> > +#define ISL29028_POWER_OFF_DELAY_MS		2000
>> > +
>> >  enum isl29028_als_ir_mode {
>> >  	ISL29028_MODE_NONE = 0,
>> >  	ISL29028_MODE_ALS,
>> > @@ -297,6 +300,23 @@ static int isl29028_ir_get(struct
>isl29028_chip *chip, int *ir_data)
>> >  	return isl29028_read_als_ir(chip, ir_data);
>> >  }
>> >  
>> > +static int isl29028_set_pm_runtime_busy(struct isl29028_chip
>*chip, bool on)
>> > +{
>> > +	struct device *dev = regmap_get_device(chip->regmap);
>> > +	int ret;
>> > +
>> > +	if (on) {
>> > +		ret = pm_runtime_get_sync(dev);
>> > +		if (ret < 0)
>> > +			pm_runtime_put_noidle(dev);
>> > +	} else {
>> > +		pm_runtime_mark_last_busy(dev);
>> > +		ret = pm_runtime_put_autosuspend(dev);
>> > +	}
>> > +
>> > +	return ret;
>> > +}
>> > +
>> >  /* Channel IO */
>> >  static int isl29028_write_raw(struct iio_dev *indio_dev,
>> >  			      struct iio_chan_spec const *chan,
>> > @@ -304,9 +324,15 @@ static int isl29028_write_raw(struct iio_dev
>*indio_dev,
>> >  {
>> >  	struct isl29028_chip *chip = iio_priv(indio_dev);
>> >  	struct device *dev = regmap_get_device(chip->regmap);
>> > -	int ret = -EINVAL;
>> > +	int ret;
>> > +
>> > +	ret = isl29028_set_pm_runtime_busy(chip, true);
>> > +	if (ret < 0)
>> > +		return ret;
>> >  
>> >  	mutex_lock(&chip->lock);
>> > +
>> > +	ret = -EINVAL;
>> >  	switch (chan->type) {
>> >  	case IIO_PROXIMITY:
>> >  		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
>> > @@ -350,6 +376,13 @@ static int isl29028_write_raw(struct iio_dev
>*indio_dev,
>> >  
>> >  	mutex_unlock(&chip->lock);
>> >  
>> > +	if (ret < 0)
>> > +		return ret;
>> > +
>> > +	ret = isl29028_set_pm_runtime_busy(chip, false);
>> > +	if (ret < 0)
>> > +		return ret;
>> > +
>> >  	return ret;
>> >  }
>> >  
>> > @@ -359,9 +392,15 @@ static int isl29028_read_raw(struct iio_dev
>*indio_dev,
>> >  {
>> >  	struct isl29028_chip *chip = iio_priv(indio_dev);
>> >  	struct device *dev = regmap_get_device(chip->regmap);
>> > -	int ret = -EINVAL;
>> > +	int ret, pm_ret;
>> > +
>> > +	ret = isl29028_set_pm_runtime_busy(chip, true);
>> > +	if (ret < 0)
>> > +		return ret;
>> >  
>> >  	mutex_lock(&chip->lock);
>> > +
>> > +	ret = -EINVAL;
>> >  	switch (mask) {
>> >  	case IIO_CHAN_INFO_RAW:
>> >  	case IIO_CHAN_INFO_PROCESSED:
>> > @@ -405,6 +444,18 @@ static int isl29028_read_raw(struct iio_dev
>*indio_dev,
>> >  
>> >  	mutex_unlock(&chip->lock);
>> >  
>> > +	if (ret < 0)
>> > +		return ret;
>> > +
>> > +	/**
>> > +	 * Preserve the ret variable if the call to
>> > +	 * isl29028_set_pm_runtime_busy() is successful so the reading
>> > +	 * (if applicable) is returned to user space.
>> > +	 */
>> > +	pm_ret = isl29028_set_pm_runtime_busy(chip, false);
>> > +	if (pm_ret < 0)
>> > +		return pm_ret;
>> > +
>> >  	return ret;
>> >  }
>> >  
>> > @@ -445,17 +496,18 @@ static const struct iio_info isl29028_info =
>{
>> >  	.write_raw = isl29028_write_raw,
>> >  };
>> >  
>> > -static int isl29028_chip_init_and_power_on(struct isl29028_chip
>*chip)
>> > +static int isl29028_clear_configure_reg(struct isl29028_chip
>*chip)
>> >  {
>> >  	struct device *dev = regmap_get_device(chip->regmap);
>> >  	int ret;
>> >  
>> >  	ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
>> > -	if (ret < 0) {
>> > +	if (ret < 0)
>> >  		dev_err(dev, "%s(): Error %d clearing the CONFIGURE register\n",
>> >  			__func__, ret);
>> > -		return ret;
>> > -	}
>> > +
>> > +	chip->als_ir_mode = ISL29028_MODE_NONE;
>> > +	chip->enable_prox = false;
>> >  
>> >  	return ret;
>> >  }
>> > @@ -509,7 +561,6 @@ static int isl29028_probe(struct i2c_client
>*client,
>> >  	chip->enable_prox  = false;
>> >  	chip->prox_sampling = 20;
>> >  	chip->lux_scale = 2000;
>> > -	chip->als_ir_mode = ISL29028_MODE_NONE;
>> >  
>> >  	ret = regmap_write(chip->regmap, ISL29028_REG_TEST1_MODE, 0x0);
>> >  	if (ret < 0) {
>> > @@ -527,7 +578,7 @@ static int isl29028_probe(struct i2c_client
>*client,
>> >  		return ret;
>> >  	}
>> >  
>> > -	ret = isl29028_chip_init_and_power_on(chip);
>> > +	ret = isl29028_clear_configure_reg(chip);
>> >  	if (ret < 0)
>> >  		return ret;
>> >  
>> > @@ -538,6 +589,11 @@ static int isl29028_probe(struct i2c_client
>*client,
>> >  	indio_dev->dev.parent = &client->dev;
>> >  	indio_dev->modes = INDIO_DIRECT_MODE;
>> >  
>> > +	pm_runtime_enable(&client->dev);
>> > +	pm_runtime_set_autosuspend_delay(&client->dev,
>> > +					 ISL29028_POWER_OFF_DELAY_MS);
>> > +	pm_runtime_use_autosuspend(&client->dev);
>> > +
>> >  	ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
>> >  	if (ret < 0) {
>> >  		dev_err(&client->dev,
>> > @@ -549,6 +605,50 @@ static int isl29028_probe(struct i2c_client
>*client,
>> >  	return 0;
>> >  }
>> >  
>> > +static int isl29028_remove(struct i2c_client *client)
>> > +{
>> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> > +	struct isl29028_chip *chip = iio_priv(indio_dev);
>> > +
>> > +	iio_device_unregister(indio_dev);
>> > +
>> > +	pm_runtime_disable(&client->dev);
>> > +	pm_runtime_set_suspended(&client->dev);
>> > +	pm_runtime_put_noidle(&client->dev);
>> > +
>> > +	return isl29028_clear_configure_reg(chip);
>> > +}
>> > +
>> > +static int __maybe_unused isl29028_suspend(struct device *dev)
>> > +{
>> > +	struct iio_dev *indio_dev =
>i2c_get_clientdata(to_i2c_client(dev));
>> > +	struct isl29028_chip *chip = iio_priv(indio_dev);
>> > +	int ret;
>> > +
>> > +	mutex_lock(&chip->lock);
>> > +
>> > +	ret = isl29028_clear_configure_reg(chip);
>> I'm stretching a bit, but there might be a race here.
>> If we get a suspend and resume in under the time it takes for
>> the auto suspend to conclude it is good to suspend, it might think
>> the device is still ready to go when it isn't.
>
>I don't believe that there is a race condition in that situation since
>all of the i2c calls outside of the probe and remove functions come
>from
>either isl29028_write_raw(), isl29028_read_raw(), or
>isl29028_suspend().
>All of three of those functions lock a common mutex.
Was more thinking that you are disabling the device without letting runtime pm know.
So it might not bring it up again....
Not chased the paths in enough depth to be sure.
>
>I just noticed that isl29028_remove() doesn't lock that shared mutex
>when the device is removed and the chip is powered off. This is
>unlikely to be an issue but I'll send a follow up patch in my next
>series fixing that just to be sure.
>
>Brian
>
>--
>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

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