[PATCH 00/19] staging: iio: isl29028: staging cleanups

[PATCH 00/19] staging: iio: isl29028: staging cleanups

[Brian Masney]

This is my first round of 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 power management. The
rest of the patches are minor improvements, mostly style.

I have one of these devices from
https://www.digikey.com/product-detail/en/intersil/ISL29028AIROZ-T7/ISL29028AIROZ-T7CT-ND/2639604
mounted on a breakout board hooked up to a Raspberry Pi 2. The IR and
light sensor are functioning, however I can't get the proximity sensor
to work. I hooked an oscilloscope up to the IRDR pin and +3V to verify
that the pin state begins pulsing when proximity mode is enabled on the
chip. According to page 9 of the datasheet
http://www.intersil.com/content/dam/Intersil/documents/isl2/isl29028.pdf,
the chip has a noise reduction feature where it will take an IR reading
with the IR LED off to get the baseline, drive the IR LED for 0.1ms, take
another IR reading, and subtract the two readings. My hunch about the
cause of my issue is the 5mm IR LED that I am using is still emitting IR
light during the brief time that the IRDR has no power. The LED is huge
size wise compared to the size of the chip. Replacing the IR LED with a
visible light LED causes the visible light LED to become illuminated
dimly when the IRDR pin begins pulsing.

Any suggestions are welcome. Ideally, I'd like to verify that the
proximity sensing works before submitting a patch moving the driver out
of staging. Looking through the data sheet, and the git commits since
the driver was first introduced into staging, everything in the driver
looks correct so I think this is operator error on my part.

Brian Masney (19):
  staging: iio: isl29028: remove nested if statements
  staging: iio: isl29028: remove enable flag from
    isl29028_enable_proximity()
  staging: iio: isl29028: remove chip test and defaults from
    isl29028_chip_init()
  staging: iio: isl29028: add power management support
  staging: iio: isl29028: made alignment of #defines consistent
  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: made column alignment in isl29028_channels
    consistent
  staging: iio: isl29028: fix comparison between signed and unsigned
    integers
  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 legacy device tree binding

 drivers/staging/iio/light/isl29028.c | 358 +++++++++++++++++++++--------------
 1 file changed, 215 insertions(+), 143 deletions(-)

-- 
2.7.4

[PATCH 01/19] staging: iio: isl29028: remove nested if statements

[Brian Masney]

There are two callers to the function isl29028_set_als_ir_mode() and
both instances use a nested if statement to only change the chip state
if it is not in the proper mode. This patch moves this check into the
isl29028_set_als_ir_mode() function to remove the nested if
statements.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index aa413e5..4e35d00 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -124,6 +124,9 @@ static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
 {
 	int ret = 0;
 
+	if (chip->als_ir_mode == mode)
+		return 0;
+
 	switch (mode) {
 	case ISL29028_MODE_ALS:
 		ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
@@ -160,6 +163,9 @@ static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
 
 	/* Need to wait for conversion time if ALS/IR mode enabled */
 	mdelay(ISL29028_CONV_TIME_MS);
+
+	chip->als_ir_mode = mode;
+
 	return 0;
 }
 
@@ -223,14 +229,10 @@ static int isl29028_als_get(struct isl29028_chip *chip, int *als_data)
 	int ret;
 	int als_ir_data;
 
-	if (chip->als_ir_mode != ISL29028_MODE_ALS) {
-		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);
-			return ret;
-		}
-		chip->als_ir_mode = ISL29028_MODE_ALS;
+	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);
+		return ret;
 	}
 
 	ret = isl29028_read_als_ir(chip, &als_ir_data);
@@ -256,14 +258,10 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
 	struct device *dev = regmap_get_device(chip->regmap);
 	int ret;
 
-	if (chip->als_ir_mode != ISL29028_MODE_IR) {
-		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);
-			return ret;
-		}
-		chip->als_ir_mode = ISL29028_MODE_IR;
+	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);
+		return ret;
 	}
 	return isl29028_read_als_ir(chip, ir_data);
 }
-- 
2.7.4

Re: [PATCH 01/19] staging: iio: isl29028: remove nested if statements

[Jonathan Cameron]

On 04/12/16 02:19, Brian Masney wrote:
> There are two callers to the function isl29028_set_als_ir_mode() and
> both instances use a nested if statement to only change the chip state
> if it is not in the proper mode. This patch moves this check into the
> isl29028_set_als_ir_mode() function to remove the nested if
> statements.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Sensible little clean up.

Applied to the togreg branch of iio.git and pushed out as testing for the
autobuilders to play with it. (If I can get a push out in the last 3 mins
of my free 15 of wifi on this train!)

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/isl29028.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index aa413e5..4e35d00 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -124,6 +124,9 @@ static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
>  {
>  	int ret = 0;
>  
> +	if (chip->als_ir_mode == mode)
> +		return 0;
> +
>  	switch (mode) {
>  	case ISL29028_MODE_ALS:
>  		ret = regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> @@ -160,6 +163,9 @@ static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
>  
>  	/* Need to wait for conversion time if ALS/IR mode enabled */
>  	mdelay(ISL29028_CONV_TIME_MS);
> +
> +	chip->als_ir_mode = mode;
> +
>  	return 0;
>  }
>  
> @@ -223,14 +229,10 @@ static int isl29028_als_get(struct isl29028_chip *chip, int *als_data)
>  	int ret;
>  	int als_ir_data;
>  
> -	if (chip->als_ir_mode != ISL29028_MODE_ALS) {
> -		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);
> -			return ret;
> -		}
> -		chip->als_ir_mode = ISL29028_MODE_ALS;
> +	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);
> +		return ret;
>  	}
>  
>  	ret = isl29028_read_als_ir(chip, &als_ir_data);
> @@ -256,14 +258,10 @@ static int isl29028_ir_get(struct isl29028_chip *chip, int *ir_data)
>  	struct device *dev = regmap_get_device(chip->regmap);
>  	int ret;
>  
> -	if (chip->als_ir_mode != ISL29028_MODE_IR) {
> -		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);
> -			return ret;
> -		}
> -		chip->als_ir_mode = ISL29028_MODE_IR;
> +	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);
> +		return ret;
>  	}
>  	return isl29028_read_als_ir(chip, ir_data);
>  }
> 

[PATCH 02/19] 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>
---
 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 4e35d00..c8e234d 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -93,15 +93,13 @@ static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
 				  sel << ISL29028_CONF_PROX_SLP_SH);
 }
 
-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;
 
@@ -215,7 +213,7 @@ static int isl29028_proxim_get(struct isl29028_chip *chip, int *prox_data)
 	int ret;
 
 	if (!chip->enable_prox) {
-		ret = isl29028_enable_proximity(chip, true);
+		ret = isl29028_enable_proximity(chip);
 		if (ret < 0)
 			return ret;
 		chip->enable_prox = true;
-- 
2.7.4

Re: [PATCH 02/19] staging: iio: isl29028: remove enable flag from isl29028_enable_proximity()

[Jonathan Cameron]

On 04/12/16 02:19, 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>
The first thing that strikes me about this, is why do we have an enable only function?

I think the intention was probably that we also disable the proximity sensing after the
reading was done...  Ideally we'd do this a little more cleverly, perhaps using runtime
pm so that if someone is requesting a stream of proximity measurements, we won't end up
powering up and down each time.

It's a little 'interesting' as we would want to power this element down even if we do
have a continuous stream of reads on the ALS.  As such we may need to roll our own
equivalent of runtime pm.

In the first instance, I'd just put a disable after the reading is taken.  This will
make a bit of a mockery of the faster sampling frequencies but there we are!


Jonathan
> ---
>  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 4e35d00..c8e234d 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -93,15 +93,13 @@ static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
>  				  sel << ISL29028_CONF_PROX_SLP_SH);
>  }
>  
> -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;
>  
> @@ -215,7 +213,7 @@ static int isl29028_proxim_get(struct isl29028_chip *chip, int *prox_data)
>  	int ret;
>  
>  	if (!chip->enable_prox) {
> -		ret = isl29028_enable_proximity(chip, true);
> +		ret = isl29028_enable_proximity(chip);
>  		if (ret < 0)
>  			return ret;
>  		chip->enable_prox = true;
> 

Re: [PATCH 02/19] staging: iio: isl29028: remove enable flag from isl29028_enable_proximity()

[Jonathan Cameron]

On 04/12/16 02:19, 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>
On second thoughts (stupid email is hiding somewhere to be sent when I have wifi
so can't reply to it) perhaps this is a coarse way of only turning proximity on if
the LED is present?  Not sure...
> ---
>  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 4e35d00..c8e234d 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -93,15 +93,13 @@ static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
>  				  sel << ISL29028_CONF_PROX_SLP_SH);
>  }
>  
> -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;
>  
> @@ -215,7 +213,7 @@ static int isl29028_proxim_get(struct isl29028_chip *chip, int *prox_data)
>  	int ret;
>  
>  	if (!chip->enable_prox) {
> -		ret = isl29028_enable_proximity(chip, true);
> +		ret = isl29028_enable_proximity(chip);
>  		if (ret < 0)
>  			return ret;
>  		chip->enable_prox = true;
> 

Re: [PATCH 02/19] staging: iio: isl29028: remove enable flag from isl29028_enable_proximity()

[Brian Masney]

On Sun, Dec 04, 2016 at 11:16:04AM +0000, Jonathan Cameron wrote:
> On 04/12/16 02:19, 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>
>
> The first thing that strikes me about this, is why do we have an enable
> only function?
> 
> I think the intention was probably that we also disable the proximity
> sensing after the
> reading was done...  Ideally we'd do this a little more cleverly,
> perhaps using runtime
> pm so that if someone is requesting a stream of proximity measurements,
> we won't end up
> powering up and down each time.
> 
> It's a little 'interesting' as we would want to power this element down
> even if we do
> have a continuous stream of reads on the ALS.  As such we may need to
> roll our own
> equivalent of runtime pm.
> 
> In the first instance, I'd just put a disable after the reading is
> taken.  This will
> make a bit of a mockery of the faster sampling frequencies but there we
> are!
>
> ---------------------
>
> On second thoughts (stupid email is hiding somewhere to be sent when I
> have wifi so can't reply to it) perhaps this is a coarse way of only
> turning proximity on if the LED is present?  Not sure...

Hi Jonathan,

I chained your two replies together above. I am probably stating the
obvious here, but I've verified with an oscilloscope that the IRDR pin
that drives the external LED is off when the chip is first initialized
and ALS readings are taken. The IRDR pin fluctuates between high and low
every 100us (if memory serves me right) once the first proximity reading
is taken until the chip is suspended.

What do you think about enabling runtime auto suspend after say 2
seconds for the whole device? There is the situation that you describe
where if someone is continuously polling the ALS but asks for a single
proximity reading. The external LED will stay on in that case. Once the
chip is suspended, and later resumes, the IRDR pin that drives the
external LED will be off until the user asks for another proximity
reading. That would allow for the faster sampling frequency.

If you still prefer, I'll go the route of shutting down the IRDR pin
after a proximity reading is taken.

Brian

Re: [PATCH 02/19] staging: iio: isl29028: remove enable flag from isl29028_enable_proximity()

[Jonathan Cameron]

On 14/01/17 20:00, Brian Masney wrote:
> On Sun, Dec 04, 2016 at 11:16:04AM +0000, Jonathan Cameron wrote:
>> On 04/12/16 02:19, 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>
>>
>> The first thing that strikes me about this, is why do we have an enable
>> only function?
>>
>> I think the intention was probably that we also disable the proximity
>> sensing after the
>> reading was done...  Ideally we'd do this a little more cleverly,
>> perhaps using runtime
>> pm so that if someone is requesting a stream of proximity measurements,
>> we won't end up
>> powering up and down each time.
>>
>> It's a little 'interesting' as we would want to power this element down
>> even if we do
>> have a continuous stream of reads on the ALS.  As such we may need to
>> roll our own
>> equivalent of runtime pm.
>>
>> In the first instance, I'd just put a disable after the reading is
>> taken.  This will
>> make a bit of a mockery of the faster sampling frequencies but there we
>> are!
>>
>> ---------------------
>>
>> On second thoughts (stupid email is hiding somewhere to be sent when I
>> have wifi so can't reply to it) perhaps this is a coarse way of only
>> turning proximity on if the LED is present?  Not sure...
> 
> Hi Jonathan,
> 
> I chained your two replies together above. I am probably stating the
> obvious here, but I've verified with an oscilloscope that the IRDR pin
> that drives the external LED is off when the chip is first initialized
> and ALS readings are taken. The IRDR pin fluctuates between high and low
> every 100us (if memory serves me right) once the first proximity reading
> is taken until the chip is suspended.
> 
> What do you think about enabling runtime auto suspend after say 2
> seconds for the whole device? There is the situation that you describe
> where if someone is continuously polling the ALS but asks for a single
> proximity reading. The external LED will stay on in that case. Once the
> chip is suspended, and later resumes, the IRDR pin that drives the
> external LED will be off until the user asks for another proximity
> reading. That would allow for the faster sampling frequency.
> 
> If you still prefer, I'll go the route of shutting down the IRDR pin
> after a proximity reading is taken.
Perhaps runtime auto suspend for the whole thing is the simplest option.
I don't mind that much either way.

Jonathan
> 
> Brian
> 

[PATCH 03/19] staging: iio: isl29028: remove chip test and defaults from isl29028_chip_init()

[Brian Masney]

isl29028_chip_init() contains the device driver defaults and
two I2C calls that detect the presence of the chip. This patch
moves these into isl29028_probe() so that this function can
be used by the power management runtinme in a followup patch. This
patch also renames isl29028_chip_init() to
isl29028_chip_init_and_power_on().

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index c8e234d..4f819a8 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -416,29 +416,11 @@ static const struct iio_info isl29028_info = {
 	.write_raw = isl29028_write_raw,
 };
 
-static int isl29028_chip_init(struct isl29028_chip *chip)
+static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
 {
 	struct device *dev = regmap_get_device(chip->regmap);
 	int ret;
 
-	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) {
-		dev_err(dev, "%s(): write to reg %d failed, err = %d\n",
-			__func__, ISL29028_REG_TEST1_MODE, ret);
-		return ret;
-	}
-	ret = regmap_write(chip->regmap, ISL29028_REG_TEST2_MODE, 0x0);
-	if (ret < 0) {
-		dev_err(dev, "%s(): write to reg %d failed, err = %d\n",
-			__func__, ISL29028_REG_TEST2_MODE, ret);
-		return ret;
-	}
-
 	ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
 	if (ret < 0) {
 		dev_err(dev, "%s(): write to reg %d failed, err = %d\n",
@@ -506,7 +488,27 @@ static int isl29028_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	ret = isl29028_chip_init(chip);
+	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) {
+		dev_err(&client->dev,
+			"%s(): write to reg %d failed, err = %d\n", __func__,
+			ISL29028_REG_TEST1_MODE, 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);
+		return ret;
+	}
+
+	ret = isl29028_chip_init_and_power_on(chip);
 	if (ret < 0) {
 		dev_err(&client->dev, "chip initialization failed: %d\n", ret);
 		return ret;
-- 
2.7.4

Re: [PATCH 03/19] staging: iio: isl29028: remove chip test and defaults from isl29028_chip_init()

[Jonathan Cameron]

On 04/12/16 02:19, Brian Masney wrote:
> isl29028_chip_init() contains the device driver defaults and
> two I2C calls that detect the presence of the chip. This patch
> moves these into isl29028_probe() so that this function can
> be used by the power management runtinme in a followup patch. This
> patch also renames isl29028_chip_init() to
> isl29028_chip_init_and_power_on().
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
I've skipped patch 2 for now but will carry on applying until they stop going in
cleanly.

Applied to the togreg branch of iio.git.

Thanks,

Jonathan
> ---
>  drivers/staging/iio/light/isl29028.c | 42 +++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index c8e234d..4f819a8 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -416,29 +416,11 @@ static const struct iio_info isl29028_info = {
>  	.write_raw = isl29028_write_raw,
>  };
>  
> -static int isl29028_chip_init(struct isl29028_chip *chip)
> +static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
>  {
>  	struct device *dev = regmap_get_device(chip->regmap);
>  	int ret;
>  
> -	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) {
> -		dev_err(dev, "%s(): write to reg %d failed, err = %d\n",
> -			__func__, ISL29028_REG_TEST1_MODE, ret);
> -		return ret;
> -	}
> -	ret = regmap_write(chip->regmap, ISL29028_REG_TEST2_MODE, 0x0);
> -	if (ret < 0) {
> -		dev_err(dev, "%s(): write to reg %d failed, err = %d\n",
> -			__func__, ISL29028_REG_TEST2_MODE, ret);
> -		return ret;
> -	}
> -
>  	ret = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
>  	if (ret < 0) {
>  		dev_err(dev, "%s(): write to reg %d failed, err = %d\n",
> @@ -506,7 +488,27 @@ static int isl29028_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	ret = isl29028_chip_init(chip);
> +	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) {
> +		dev_err(&client->dev,
> +			"%s(): write to reg %d failed, err = %d\n", __func__,
> +			ISL29028_REG_TEST1_MODE, 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);
> +		return ret;
> +	}
> +
> +	ret = isl29028_chip_init_and_power_on(chip);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "chip initialization failed: %d\n", ret);
>  		return ret;
> 

[PATCH 04/19] staging: iio: isl29028: add power management support

[Brian Masney]

This patch adds power management support to the isl29028 driver.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 4f819a8..e96a8cb 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -75,6 +75,7 @@ struct isl29028_chip {
 
 	int			lux_scale;
 	enum isl29028_als_ir_mode	als_ir_mode;
+	bool				suspended;
 };
 
 static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
@@ -274,6 +275,12 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 	int ret = -EINVAL;
 
 	mutex_lock(&chip->lock);
+
+	if (chip->suspended) {
+		ret = -EBUSY;
+		goto write_done;
+	}
+
 	switch (chan->type) {
 	case IIO_PROXIMITY:
 		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
@@ -322,6 +329,8 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 		dev_err(dev, "Unsupported channel type\n");
 		break;
 	}
+
+write_done:
 	mutex_unlock(&chip->lock);
 	return ret;
 }
@@ -335,6 +344,12 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 	int ret = -EINVAL;
 
 	mutex_lock(&chip->lock);
+
+	if (chip->suspended) {
+		ret = -EBUSY;
+		goto read_done;
+	}
+
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 	case IIO_CHAN_INFO_PROCESSED:
@@ -374,6 +389,8 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 		dev_err(dev, "mask value 0x%08lx not supported\n", mask);
 		break;
 	}
+
+read_done:
 	mutex_unlock(&chip->lock);
 	return ret;
 }
@@ -437,6 +454,9 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
 	ret = isl29028_set_als_scale(chip, chip->lux_scale);
 	if (ret < 0)
 		dev_err(dev, "setting als scale failed, err = %d\n", ret);
+
+	chip->suspended = false;
+
 	return ret;
 }
 
@@ -479,6 +499,7 @@ static int isl29028_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, indio_dev);
 	mutex_init(&chip->lock);
+	chip->suspended = true;
 
 	chip->regmap = devm_regmap_init_i2c(client, &isl29028_regmap_config);
 	if (IS_ERR(chip->regmap)) {
@@ -530,6 +551,43 @@ static int isl29028_probe(struct i2c_client *client,
 	return 0;
 }
 
+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 = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
+	if (ret < 0)
+		dev_err(dev, "%s(): Error %d turning off chip\n", __func__,
+			ret);
+
+	chip->suspended = true;
+
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static int __maybe_unused isl29028_resume(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_chip_init_and_power_on(chip);
+
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(isl29028_pm_ops, isl29028_suspend, isl29028_resume);
+
 static const struct i2c_device_id isl29028_id[] = {
 	{"isl29028", 0},
 	{}
@@ -546,6 +604,7 @@ 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,
-- 
2.7.4

Re: [PATCH 04/19] staging: iio: isl29028: add power management support

[Jonathan Cameron]

On 04/12/16 02:19, Brian Masney wrote:
> This patch adds power management support to the isl29028 driver.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Hmm. I'm not sure why we need the suspended boolean.

I 'think' it's impossible to have a sysfs read in progress during suspend or resume.
At least I hope it is as a lot of drivers will be broken otherwise :(

Looks like this particular corner is only really present in light sensor drivers...

Jonathan

> ---
>  drivers/staging/iio/light/isl29028.c | 59 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 4f819a8..e96a8cb 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -75,6 +75,7 @@ struct isl29028_chip {
>  
>  	int			lux_scale;
>  	enum isl29028_als_ir_mode	als_ir_mode;
> +	bool				suspended;
>  };
>  
>  static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
> @@ -274,6 +275,12 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  	int ret = -EINVAL;
>  
>  	mutex_lock(&chip->lock);
> +
> +	if (chip->suspended) {
> +		ret = -EBUSY;
> +		goto write_done;
> +	}
> +
>  	switch (chan->type) {
>  	case IIO_PROXIMITY:
>  		if (mask != IIO_CHAN_INFO_SAMP_FREQ) {
> @@ -322,6 +329,8 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
>  		dev_err(dev, "Unsupported channel type\n");
>  		break;
>  	}
> +
> +write_done:
>  	mutex_unlock(&chip->lock);
>  	return ret;
>  }
> @@ -335,6 +344,12 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  	int ret = -EINVAL;
>  
>  	mutex_lock(&chip->lock);
> +
> +	if (chip->suspended) {
> +		ret = -EBUSY;
> +		goto read_done;
> +	}
> +
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
>  	case IIO_CHAN_INFO_PROCESSED:
> @@ -374,6 +389,8 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
>  		dev_err(dev, "mask value 0x%08lx not supported\n", mask);
>  		break;
>  	}
> +
> +read_done:
>  	mutex_unlock(&chip->lock);
>  	return ret;
>  }
> @@ -437,6 +454,9 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
>  	ret = isl29028_set_als_scale(chip, chip->lux_scale);
>  	if (ret < 0)
>  		dev_err(dev, "setting als scale failed, err = %d\n", ret);
> +
> +	chip->suspended = false;
> +
>  	return ret;
>  }
>  
> @@ -479,6 +499,7 @@ static int isl29028_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, indio_dev);
>  	mutex_init(&chip->lock);
> +	chip->suspended = true;
>  
>  	chip->regmap = devm_regmap_init_i2c(client, &isl29028_regmap_config);
>  	if (IS_ERR(chip->regmap)) {
> @@ -530,6 +551,43 @@ static int isl29028_probe(struct i2c_client *client,
>  	return 0;
>  }
>  
> +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 = regmap_write(chip->regmap, ISL29028_REG_CONFIGURE, 0x0);
> +	if (ret < 0)
> +		dev_err(dev, "%s(): Error %d turning off chip\n", __func__,
> +			ret);
> +
> +	chip->suspended = true;
> +
> +	mutex_unlock(&chip->lock);
> +
> +	return ret;
> +}
> +
> +static int __maybe_unused isl29028_resume(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_chip_init_and_power_on(chip);
> +
> +	mutex_unlock(&chip->lock);
> +
> +	return ret;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(isl29028_pm_ops, isl29028_suspend, isl29028_resume);
> +
>  static const struct i2c_device_id isl29028_id[] = {
>  	{"isl29028", 0},
>  	{}
> @@ -546,6 +604,7 @@ 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,
> 

[PATCH 05/19] staging: iio: isl29028: made alignment of #defines consistent

[Brian Masney]

The alignment of the #defines at the top of the file is not consistent.
This changes all of the defines to use consistent alignment to improve
the code readability.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index e96a8cb..d5b069e 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -27,38 +27,38 @@
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
 
-#define ISL29028_CONV_TIME_MS		100
+#define ISL29028_CONV_TIME_MS			100
 
-#define ISL29028_REG_CONFIGURE		0x01
+#define ISL29028_REG_CONFIGURE			0x01
 
-#define ISL29028_CONF_ALS_IR_MODE_ALS	0
-#define ISL29028_CONF_ALS_IR_MODE_IR	BIT(0)
-#define ISL29028_CONF_ALS_IR_MODE_MASK	BIT(0)
+#define ISL29028_CONF_ALS_IR_MODE_ALS		0
+#define ISL29028_CONF_ALS_IR_MODE_IR		BIT(0)
+#define ISL29028_CONF_ALS_IR_MODE_MASK		BIT(0)
 
-#define ISL29028_CONF_ALS_RANGE_LOW_LUX	0
+#define ISL29028_CONF_ALS_RANGE_LOW_LUX		0
 #define ISL29028_CONF_ALS_RANGE_HIGH_LUX	BIT(1)
-#define ISL29028_CONF_ALS_RANGE_MASK	BIT(1)
+#define ISL29028_CONF_ALS_RANGE_MASK		BIT(1)
 
-#define ISL29028_CONF_ALS_DIS		0
-#define ISL29028_CONF_ALS_EN		BIT(2)
-#define ISL29028_CONF_ALS_EN_MASK	BIT(2)
+#define ISL29028_CONF_ALS_DIS			0
+#define ISL29028_CONF_ALS_EN			BIT(2)
+#define ISL29028_CONF_ALS_EN_MASK		BIT(2)
 
-#define ISL29028_CONF_PROX_SLP_SH	4
-#define ISL29028_CONF_PROX_SLP_MASK	(7 << ISL29028_CONF_PROX_SLP_SH)
+#define ISL29028_CONF_PROX_SLP_SH		4
+#define ISL29028_CONF_PROX_SLP_MASK		(7 << ISL29028_CONF_PROX_SLP_SH)
 
-#define ISL29028_CONF_PROX_EN		BIT(7)
-#define ISL29028_CONF_PROX_EN_MASK	BIT(7)
+#define ISL29028_CONF_PROX_EN			BIT(7)
+#define ISL29028_CONF_PROX_EN_MASK		BIT(7)
 
-#define ISL29028_REG_INTERRUPT		0x02
+#define ISL29028_REG_INTERRUPT			0x02
 
-#define ISL29028_REG_PROX_DATA		0x08
-#define ISL29028_REG_ALSIR_L		0x09
-#define ISL29028_REG_ALSIR_U		0x0A
+#define ISL29028_REG_PROX_DATA			0x08
+#define ISL29028_REG_ALSIR_L			0x09
+#define ISL29028_REG_ALSIR_U			0x0A
 
-#define ISL29028_REG_TEST1_MODE		0x0E
-#define ISL29028_REG_TEST2_MODE		0x0F
+#define ISL29028_REG_TEST1_MODE			0x0E
+#define ISL29028_REG_TEST2_MODE			0x0F
 
-#define ISL29028_NUM_REGS		(ISL29028_REG_TEST2_MODE + 1)
+#define ISL29028_NUM_REGS			(ISL29028_REG_TEST2_MODE + 1)
 
 enum isl29028_als_ir_mode {
 	ISL29028_MODE_NONE = 0,
-- 
2.7.4

Re: [PATCH 05/19] staging: iio: isl29028: made alignment of #defines consistent

[Jonathan Cameron]

On 04/12/16 02:19, Brian Masney wrote:
> The alignment of the #defines at the top of the file is not consistent.
> This changes all of the defines to use consistent alignment to improve
> the code readability.
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
Skipped patch 4 for now, but this one still applies fine so applied to the
togreg branch of iio.git.

Will push out as testing later.
> ---
>  drivers/staging/iio/light/isl29028.c | 42 ++++++++++++++++++------------------
>  1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index e96a8cb..d5b069e 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -27,38 +27,38 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  
> -#define ISL29028_CONV_TIME_MS		100
> +#define ISL29028_CONV_TIME_MS			100
>  
> -#define ISL29028_REG_CONFIGURE		0x01
> +#define ISL29028_REG_CONFIGURE			0x01
>  
> -#define ISL29028_CONF_ALS_IR_MODE_ALS	0
> -#define ISL29028_CONF_ALS_IR_MODE_IR	BIT(0)
> -#define ISL29028_CONF_ALS_IR_MODE_MASK	BIT(0)
> +#define ISL29028_CONF_ALS_IR_MODE_ALS		0
> +#define ISL29028_CONF_ALS_IR_MODE_IR		BIT(0)
> +#define ISL29028_CONF_ALS_IR_MODE_MASK		BIT(0)
>  
> -#define ISL29028_CONF_ALS_RANGE_LOW_LUX	0
> +#define ISL29028_CONF_ALS_RANGE_LOW_LUX		0
>  #define ISL29028_CONF_ALS_RANGE_HIGH_LUX	BIT(1)
> -#define ISL29028_CONF_ALS_RANGE_MASK	BIT(1)
> +#define ISL29028_CONF_ALS_RANGE_MASK		BIT(1)
>  
> -#define ISL29028_CONF_ALS_DIS		0
> -#define ISL29028_CONF_ALS_EN		BIT(2)
> -#define ISL29028_CONF_ALS_EN_MASK	BIT(2)
> +#define ISL29028_CONF_ALS_DIS			0
> +#define ISL29028_CONF_ALS_EN			BIT(2)
> +#define ISL29028_CONF_ALS_EN_MASK		BIT(2)
>  
> -#define ISL29028_CONF_PROX_SLP_SH	4
> -#define ISL29028_CONF_PROX_SLP_MASK	(7 << ISL29028_CONF_PROX_SLP_SH)
> +#define ISL29028_CONF_PROX_SLP_SH		4
> +#define ISL29028_CONF_PROX_SLP_MASK		(7 << ISL29028_CONF_PROX_SLP_SH)
>  
> -#define ISL29028_CONF_PROX_EN		BIT(7)
> -#define ISL29028_CONF_PROX_EN_MASK	BIT(7)
> +#define ISL29028_CONF_PROX_EN			BIT(7)
> +#define ISL29028_CONF_PROX_EN_MASK		BIT(7)
>  
> -#define ISL29028_REG_INTERRUPT		0x02
> +#define ISL29028_REG_INTERRUPT			0x02
>  
> -#define ISL29028_REG_PROX_DATA		0x08
> -#define ISL29028_REG_ALSIR_L		0x09
> -#define ISL29028_REG_ALSIR_U		0x0A
> +#define ISL29028_REG_PROX_DATA			0x08
> +#define ISL29028_REG_ALSIR_L			0x09
> +#define ISL29028_REG_ALSIR_U			0x0A
>  
> -#define ISL29028_REG_TEST1_MODE		0x0E
> -#define ISL29028_REG_TEST2_MODE		0x0F
> +#define ISL29028_REG_TEST1_MODE			0x0E
> +#define ISL29028_REG_TEST2_MODE			0x0F
>  
> -#define ISL29028_NUM_REGS		(ISL29028_REG_TEST2_MODE + 1)
> +#define ISL29028_NUM_REGS			(ISL29028_REG_TEST2_MODE + 1)
>  
>  enum isl29028_als_ir_mode {
>  	ISL29028_MODE_NONE = 0,
> 

[PATCH 06/19] 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 d5b069e..fe8814b 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;
 	bool				suspended;
 };
-- 
2.7.4

[PATCH 07/19] 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 fe8814b..38027ee 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -115,7 +115,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,
@@ -147,7 +147,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.7.4

[PATCH 08/19] 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 38027ee..fa2b283 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -200,6 +200,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);
+		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",
@@ -210,19 +217,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);
-		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);
@@ -362,7 +356,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.7.4

[PATCH 09/19] 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 | 31 ++++++++++++++++++++++---------
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index fa2b283..8f9295f 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;
 	bool				suspended;
@@ -89,6 +87,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);
@@ -106,6 +105,7 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip)
 
 	/* Wait for conversion to be complete for first sample */
 	mdelay(DIV_ROUND_UP(1000, chip->prox_sampling));
+
 	return 0;
 }
 
@@ -138,13 +138,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,
@@ -191,6 +189,7 @@ static int isl29028_read_als_ir(struct isl29028_chip *chip, int *als_ir)
 	}
 
 	*als_ir = ((msb & 0xF) << 8) | (lsb & 0xFF);
+
 	return 0;
 }
 
@@ -204,6 +203,7 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
 		ret = isl29028_enable_proximity(chip);
 		if (ret < 0)
 			return ret;
+
 		chip->enable_prox = true;
 	}
 
@@ -213,7 +213,9 @@ static int isl29028_read_proxim(struct isl29028_chip *chip, int *prox)
 			ISL29028_REG_PROX_DATA, ret);
 		return ret;
 	}
+
 	*prox = data;
+
 	return 0;
 }
 
@@ -244,6 +246,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;
 }
 
@@ -257,6 +260,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);
 }
 
@@ -284,11 +288,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,
@@ -296,9 +302,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,
@@ -306,20 +312,22 @@ 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;
@@ -327,6 +335,7 @@ static int isl29028_write_raw(struct iio_dev *indio_dev,
 
 write_done:
 	mutex_unlock(&chip->lock);
+
 	return ret;
 }
 
@@ -361,25 +370,25 @@ 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;
@@ -387,6 +396,7 @@ static int isl29028_read_raw(struct iio_dev *indio_dev,
 
 read_done:
 	mutex_unlock(&chip->lock);
+
 	return ret;
 }
 
@@ -516,6 +526,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,
@@ -536,6 +547,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,
@@ -543,6 +555,7 @@ static int isl29028_probe(struct i2c_client *client,
 			ret);
 		return ret;
 	}
+
 	return 0;
 }
 
-- 
2.7.4

[PATCH 10/19] 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 8f9295f..0e727ba 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -404,7 +404,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.7.4

[PATCH 11/19] staging: iio: isl29028: made column alignment in isl29028_channels consistent

[Brian Masney]

The three info_mask_separate members OR several BIT(xxx) fields
together. This patch changes the column alignment of these fields to be
aligned at the same column to improve the overall code readability. It
also moves the { for the next channel to the next line to improve code
readability.

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

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 0e727ba..218d165 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -419,14 +419,16 @@ static const struct iio_chan_spec isl29028_channels[] = {
 	{
 		.type = IIO_LIGHT,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
-		BIT(IIO_CHAN_INFO_SCALE),
-	}, {
+				      BIT(IIO_CHAN_INFO_SCALE),
+	},
+	{
 		.type = IIO_INTENSITY,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
-	}, {
+	},
+	{
 		.type = IIO_PROXIMITY,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
-		BIT(IIO_CHAN_INFO_SAMP_FREQ),
+				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	}
 };
 
-- 
2.7.4

Re: [PATCH 11/19] staging: iio: isl29028: made column alignment in isl29028_channels consistent

[Jonathan Cameron]

On 04/12/16 02:19, Brian Masney wrote:
> The three info_mask_separate members OR several BIT(xxx) fields
> together. This patch changes the column alignment of these fields to be
> aligned at the same column to improve the overall code readability. It
> also moves the { for the next channel to the next line to improve code
> readability.
Skip the bracket change - to my mind it doesn't improve readability.
I mostly don't care which form is used on these (others are fussier) but
changing it from the one I slightly prefer to the one I dislike
is a step too far ;)

Jonathan

> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> ---
>  drivers/staging/iio/light/isl29028.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 0e727ba..218d165 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -419,14 +419,16 @@ static const struct iio_chan_spec isl29028_channels[] = {
>  	{
>  		.type = IIO_LIGHT,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
> -		BIT(IIO_CHAN_INFO_SCALE),
> -	}, {
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +	{
>  		.type = IIO_INTENSITY,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> -	}, {
> +	},
> +	{
>  		.type = IIO_PROXIMITY,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> -		BIT(IIO_CHAN_INFO_SAMP_FREQ),
> +				      BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	}
>  };
>  
> 

[PATCH 12/19] staging: iio: isl29028: fix comparison between signed and unsigned integers

[Brian Masney]

Fixed warning found by make W=2 to reduce the amount of build noise:

warning: comparison between signed and unsigned integer expressions
[-Wsign-compare]

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 218d165..23a4db8 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -80,7 +80,7 @@ static int isl29028_set_proxim_sampling(struct isl29028_chip *chip,
 					unsigned int sampling)
 {
 	static unsigned int prox_period[] = {800, 400, 200, 100, 75, 50, 12, 0};
-	int sel;
+	unsigned int sel;
 	unsigned int period = DIV_ROUND_UP(1000, sampling);
 
 	for (sel = 0; sel < ARRAY_SIZE(prox_period); ++sel) {
-- 
2.7.4

Re: [PATCH 12/19] staging: iio: isl29028: fix comparison between signed and unsigned integers

[Dan Carpenter]

On Sat, Dec 03, 2016 at 09:19:36PM -0500, Brian Masney wrote:
> Fixed warning found by make W=2 to reduce the amount of build noise:
> 
> warning: comparison between signed and unsigned integer expressions
> [-Wsign-compare]

Ugh...  Please don't do work arounds for nonsense warnings.  W=2 is so
stupid.  Better to just grep -v this warning instead of trying to please
a broken static analysis.  Warnings like this are why it's disabled by
default.

regards,
dan carpenter

Re: [PATCH 12/19] staging: iio: isl29028: fix comparison between signed and unsigned integers

[Brian Masney]

On Mon, Dec 05, 2016 at 11:53:39PM +0300, Dan Carpenter wrote:
> On Sat, Dec 03, 2016 at 09:19:36PM -0500, Brian Masney wrote:
> > Fixed warning found by make W=2 to reduce the amount of build noise:
> > 
> > warning: comparison between signed and unsigned integer expressions
> > [-Wsign-compare]
> 
> Ugh...  Please don't do work arounds for nonsense warnings.  W=2 is so
> stupid.  Better to just grep -v this warning instead of trying to please
> a broken static analysis.  Warnings like this are why it's disabled by
> default.

Hi Dan,
   I would normally agree, however there could be a case where this
warning flags a legitimate issue. It is obviously not an issue in this
case. Since I'm already working on cleaning up this driver to move it
out of staging, I figured that I would make sure that it builds cleanly
with W=2. This was the only warning found in that driver. The
change is harmless in my opinion and it may eliminate a nonsense warning
for someone else down the road when doing security audits.

   This driver doesn't need much to move it out of staging. Most of the
patches in this series were trivial cleanups and not interesting at all.
Since I already have one of these devices, I figured that I'd do the
grunt work to get it out of staging. My goal with the upcoming final
patch that moves it out of staging is to reduce the amount of code churn
in the driver once it graduates from staging.

Brian

Re: [PATCH 12/19] staging: iio: isl29028: fix comparison between signed and unsigned integers

[Dan Carpenter]

On Mon, Dec 05, 2016 at 07:10:45PM -0500, Brian Masney wrote:
> On Mon, Dec 05, 2016 at 11:53:39PM +0300, Dan Carpenter wrote:
> > On Sat, Dec 03, 2016 at 09:19:36PM -0500, Brian Masney wrote:
> > > Fixed warning found by make W=2 to reduce the amount of build noise:
> > > 
> > > warning: comparison between signed and unsigned integer expressions
> > > [-Wsign-compare]
> > 
> > Ugh...  Please don't do work arounds for nonsense warnings.  W=2 is so
> > stupid.  Better to just grep -v this warning instead of trying to please
> > a broken static analysis.  Warnings like this are why it's disabled by
> > default.
> 
> Hi Dan,
>    I would normally agree, however there could be a case where this
> warning flags a legitimate issue. It is obviously not an issue in this
> case. Since I'm already working on cleaning up this driver to move it
> out of staging, I figured that I would make sure that it builds cleanly
> with W=2. This was the only warning found in that driver. The
> change is harmless in my opinion and it may eliminate a nonsense warning
> for someone else down the road when doing security audits.

Iterators should be int unless there is a specific reason for a fancier
data type.  Using complicated types just makes the code more complicated
and tiring to read.

Smatch or other similar static analysis tools know that "sel" is in the
0-7 range and that ARRAY_SIZE(prox_period) is 8.  GCC almost certainly
knows this as well.  The warning messages is just printed because the
devs are lazy.  It's totally pointless.

Don't work around lazy static analysis.  It sends the wrong message to
do pointless things.

regards,
dan carpenter

[PATCH 13/19] 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 | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 23a4db8..0c8a878 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -79,18 +79,28 @@ 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};
 	unsigned int sel;
 	unsigned int period = DIV_ROUND_UP(1000, sampling);
+	int 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);
+	else
+		chip->prox_sampling = sampling;
+
+	return ret;
 }
 
 static int isl29028_enable_proximity(struct isl29028_chip *chip)
@@ -296,14 +306,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) {
@@ -452,10 +454,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.7.4

Re: [PATCH 13/19] staging: iio: isl29028: move failure logging into isl29028_set_proxim_sampling()

[Jonathan Cameron]

On 04/12/16 02:19, 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>
Minor point inline.
> ---
>  drivers/staging/iio/light/isl29028.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 23a4db8..0c8a878 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -79,18 +79,28 @@ 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};
>  	unsigned int sel;
>  	unsigned int period = DIV_ROUND_UP(1000, sampling);
> +	int 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);
I'd prefer the cleaner style or a return inside this if statement and dropping the
else.
> +	else
> +		chip->prox_sampling = sampling;
> +
> +	return ret;
>  }
>  
>  static int isl29028_enable_proximity(struct isl29028_chip *chip)
> @@ -296,14 +306,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) {
> @@ -452,10 +454,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)
> 

[PATCH 14/19] 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 | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
index 0c8a878..11e1673 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -121,11 +121,21 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip)
 
 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);
+	else
+		chip->lux_scale = lux_scale;
 
-	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
-				  ISL29028_CONF_ALS_RANGE_MASK, val);
+	return ret;
 }
 
 static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
@@ -322,13 +332,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");
@@ -459,7 +462,7 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
 
 	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;
 
 	chip->suspended = false;
 
-- 
2.7.4

Re: [PATCH 14/19] staging: iio: isl29028: move failure logging into isl29028_set_als_scale()

[Jonathan Cameron]

On 04/12/16 02:19, 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>
> ---
>  drivers/staging/iio/light/isl29028.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29028.c b/drivers/staging/iio/light/isl29028.c
> index 0c8a878..11e1673 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -121,11 +121,21 @@ static int isl29028_enable_proximity(struct isl29028_chip *chip)
>  
>  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);
Again, I would prefer a direct return here and drop the else.
Where possible it's always nice to have the 'good' path be the inline one and
indent the error cases.
> +	else
> +		chip->lux_scale = lux_scale;
>  
> -	return regmap_update_bits(chip->regmap, ISL29028_REG_CONFIGURE,
> -				  ISL29028_CONF_ALS_RANGE_MASK, val);
> +	return ret;
>  }
>  
>  static int isl29028_set_als_ir_mode(struct isl29028_chip *chip,
> @@ -322,13 +332,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");
> @@ -459,7 +462,7 @@ static int isl29028_chip_init_and_power_on(struct isl29028_chip *chip)
>  
>  	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;
>  
>  	chip->suspended = false;
>  
> 

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

[Brian Masney]

The wording and style of the different error messages was not
consistent. This patches 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 11e1673..a45d57c 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -197,14 +197,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;
 	}
 
@@ -229,8 +231,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;
 	}
 
@@ -247,7 +249,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;
 	}
 
@@ -277,7 +280,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;
 	}
 
@@ -304,14 +308,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;
 		}
 
@@ -320,21 +325,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;
 	}
 
@@ -395,7 +402,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;
 	}
 
@@ -451,8 +459,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;
 	}
 
@@ -513,8 +521,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;
 	}
 
@@ -526,16 +534,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;
 	}
 
@@ -555,8 +563,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.7.4

[PATCH 16/19] 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 a45d57c..3f01ad1 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -548,10 +548,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.7.4

[PATCH 17/19] 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 3f01ad1..bda5183 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -507,10 +507,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.7.4

[PATCH 18/19] 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 bda5183..ae6896f 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -330,7 +330,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.7.4

[PATCH 19/19] staging: iio: isl29028: remove legacy device tree binding

[Brian Masney]

The isl29028 staging driver contains a legacy binding of "isl,isl29028"
that is marked as legacy and to not use. There are no in tree references
to that binding. This patch removes the legacy binding in preparation
for moving the driver out of staging. Otherwise, we will have to support
this binding indefinitely.

Signed-off-by: Brian Masney <masneyb@onstation.org>
Cc: devicetree@vger.kernel.org
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <Mark.Rutland@arm.com>
---
 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 ae6896f..6be9aa7 100644
--- a/drivers/staging/iio/light/isl29028.c
+++ b/drivers/staging/iio/light/isl29028.c
@@ -611,7 +611,6 @@ static const struct i2c_device_id isl29028_id[] = {
 MODULE_DEVICE_TABLE(i2c, isl29028_id);
 
 static const struct of_device_id isl29028_of_match[] = {
-	{ .compatible = "isl,isl29028", }, /* for backward compat., don't use */
 	{ .compatible = "isil,isl29028", },
 	{ },
 };
-- 
2.7.4

Re: [PATCH 19/19] staging: iio: isl29028: remove legacy device tree binding

[Jonathan Cameron]

On 04/12/16 02:19, Brian Masney wrote:
> The isl29028 staging driver contains a legacy binding of "isl,isl29028"
> that is marked as legacy and to not use. There are no in tree references
> to that binding. This patch removes the legacy binding in preparation
> for moving the driver out of staging. Otherwise, we will have to support
> this binding indefinitely.
Leave this be.  Given it exists there almost certainly device trees out there
using it. Lets not break them just to save on one line of code.

Supporting legacy bindings isn't exactly costly!

I'm happy with all the patches I haven't commented on. Just can't apply
them because they have dependencies on early patches that I have
raised questions on.

Another generally nice bit of cleanup.

Thanks,

Jonathan
> 
> Signed-off-by: Brian Masney <masneyb@onstation.org>
> Cc: devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <Mark.Rutland@arm.com>
> ---
>  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 ae6896f..6be9aa7 100644
> --- a/drivers/staging/iio/light/isl29028.c
> +++ b/drivers/staging/iio/light/isl29028.c
> @@ -611,7 +611,6 @@ static const struct i2c_device_id isl29028_id[] = {
>  MODULE_DEVICE_TABLE(i2c, isl29028_id);
>  
>  static const struct of_device_id isl29028_of_match[] = {
> -	{ .compatible = "isl,isl29028", }, /* for backward compat., don't use */
>  	{ .compatible = "isil,isl29028", },
>  	{ },
>  };
> 

Re: [PATCH 19/19] staging: iio: isl29028: remove legacy device tree binding

[Jonathan Cameron]

Sorry all, appears some replies got stuck in my outbox and have just managed to send for some reason!

Jonathan

On 04/12/16 11:55, Jonathan Cameron wrote:
> On 04/12/16 02:19, Brian Masney wrote:
>> The isl29028 staging driver contains a legacy binding of "isl,isl29028"
>> that is marked as legacy and to not use. There are no in tree references
>> to that binding. This patch removes the legacy binding in preparation
>> for moving the driver out of staging. Otherwise, we will have to support
>> this binding indefinitely.
> Leave this be.  Given it exists there almost certainly device trees out there
> using it. Lets not break them just to save on one line of code.
> 
> Supporting legacy bindings isn't exactly costly!
> 
> I'm happy with all the patches I haven't commented on. Just can't apply
> them because they have dependencies on early patches that I have
> raised questions on.
> 
> Another generally nice bit of cleanup.
> 
> Thanks,
> 
> Jonathan
>>
>> Signed-off-by: Brian Masney <masneyb@onstation.org>
>> Cc: devicetree@vger.kernel.org
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <Mark.Rutland@arm.com>
>> ---
>>  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 ae6896f..6be9aa7 100644
>> --- a/drivers/staging/iio/light/isl29028.c
>> +++ b/drivers/staging/iio/light/isl29028.c
>> @@ -611,7 +611,6 @@ static const struct i2c_device_id isl29028_id[] = {
>>  MODULE_DEVICE_TABLE(i2c, isl29028_id);
>>  
>>  static const struct of_device_id isl29028_of_match[] = {
>> -	{ .compatible = "isl,isl29028", }, /* for backward compat., don't use */
>>  	{ .compatible = "isil,isl29028", },
>>  	{ },
>>  };
>>
> 
> --
> 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
>