Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1553818 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 02/19] staging: iio: isl29028: remove enable flag from  isl29028_enable_proximity() Jonathan Cameron <jic23@kernel.org> - 2017-01-08 10:50 +0100
    Re: [PATCH 02/19] staging: iio: isl29028: remove enable flag from  isl29028_enable_proximity() Brian Masney <masneyb@onstation.org> - 2017-01-14 21:10 +0100
      Re: [PATCH 02/19] staging: iio: isl29028: remove enable flag from  isl29028_enable_proximity() Jonathan Cameron <jic23@kernel.org> - 2017-01-15 15:40 +0100

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

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;
> 

[toc] | [next] | [standalone]

#1559067

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

[toc] | [prev] | [next] | [standalone]

#1559245

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
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web