Re: [PATCH v5 3/3] iio: light: Add support for APDS9306 Light Sensor

[Jonathan Cameron]

On Mon, 22 Jan 2024 21:26:03 +1030
Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx> wrote:

> On 22/1/24 01:53, Jonathan Cameron wrote:
> > On Sun, 21 Jan 2024 15:47:34 +1030
> > Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx> wrote:
> >   
> >> Driver support for Avago (Broadcom) APDS9306 Ambient Light Sensor.
> >> It has two channels - ALS and CLEAR. The ALS (Ambient Light Sensor)
> >> channel approximates the response of the human-eye providing direct
> >> read out where the output count is proportional to ambient light levels.
> >> It is internally temperature compensated and rejects 50Hz and 60Hz flicker
> >> caused by artificial light sources. Hardware interrupt configuration is
> >> optional. It is a low power device with 20 bit resolution and has
> >> configurable adaptive interrupt mode and interrupt persistence mode.
> >> The device also features inbuilt hardware gain, multiple integration time
> >> selection options and sampling frequency selection options.
> >>
> >> This driver also uses the IIO GTS (Gain Time Scale) Helpers Namespace for
> >> Scales, Gains and Integration time implementation.
> >>
> >> Signed-off-by: Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>
> >> ---
> >> v2 -> v5:  
> > 
> > Why did you jump to v5?  Some internal or private reviews perhaps?
> > Better for those tracking on the list if you just used v3.  
> Wish I had someone to review my code before sending it to kernel community!
> I do this in my own time.
> 
> v5 was suggested by you. Now I understand that Suggested-by: tag has to be used :)
> https://lore.kernel.org/all/20231028143631.2545f93e@jic23-huawei/

oops :)  It's fine, I'd just forgotten that discussion entirely!


> >> diff --git a/drivers/iio/light/apds9306.c b/drivers/iio/light/apds9306.c
> >> new file mode 100644
> >> index 000000000000..8ed5899050ed
> >> --- /dev/null
> >> +++ b/drivers/iio/light/apds9306.c
> >> @@ -0,0 +1,1315 @@
> >> +// SPDX-License-Identifier: GPL-2.0-or-later
> >> +/*
> >> + * APDS-9306/APDS-9306-065 Ambient Light Sensor
> >> + * I2C Address: 0x52
> >> + * Datasheet: https://docs.broadcom.com/doc/AV02-4755EN
> >> + *
> >> + * Copyright (C) 2023 Subhajit Ghosh <subhajit.ghosh@xxxxxxxxxxxxxx>  
> > 
> > Given you are still changing it, feel free to include 2024!  
> I sincerely hope that I don't have to update it to 2025!

:)

..

> >   
> >> +	},
> >> +};  
> >   
> >> +  
> >   
> >> +
> >> +static int apds9306_runtime_power_on(struct device *dev)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = pm_runtime_resume_and_get(dev);
> >> +	if (ret < 0)
> >> +		dev_err(dev, "runtime resume failed: %d\n", ret);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >> +static int apds9306_runtime_power_off(struct device *dev)
> >> +{
> >> +	pm_runtime_mark_last_busy(dev);
> >> +	pm_runtime_put_autosuspend(dev);
> >> +
> >> +	return 0;
> >> +}  
> > 
> > I'm not entirely convinced these two wrappers are worthwhile given they
> > aren't that common used and locally obscure what is going on when
> > it could be apparent at the few call sites.  
> The above was suggested by Andy.
> https://lore.kernel.org/linux-devicetree/ZTuuUl0PBklbVjb9@xxxxxxxxxxxxxxxxxx/

Ah. Fair enough.  I'm not that bothered so if Andy prefers this then fine to
keep it. 

> 
> Apologies for my ignorance - "it could be apparent at the few call sites" -
> I did not understand the above statement. Can you please elaborate?

I meant what was going on (the fact it's using autosuspend for example)
could be visible where it is called in a way that is hidden by the
wrappers.  Not a big thing though.





> >> +
> >> +	ret = devm_add_action_or_reset(dev, apds9306_powerdown, data);
> >> +	if (ret)
> >> +		return dev_err_probe(dev, ret, "failed to add action or reset\n");
> >> +
> >> +	ret = devm_iio_device_register(dev, indio_dev);
> >> +	if (ret)
> >> +		return dev_err_probe(dev, ret, "failed iio device registration\n");
> >> +
> >> +	pm_runtime_put_autosuspend(dev);  
> > 
> > Where is the matching get?  I don't recall any of the pm functions
> > leaving us with the reference count raised except for the where it is
> > called out in the function name.
> >   
> I blindly copy pasted your below suggestion.
> https://lore.kernel.org/all/20231028162025.4259f1cc@jic23-huawei/
> 
> "this lot of runtime pm stuff isn't initializing the device, so I don't
> see it as making sense in here. I'd push it out to the caller with
> the power up before init and the autosuspend etc after.
> I'll note that I'd expect to see a a pm_runtime_put_autosuspend()
> at the end of probe to put device to sleep soon after loading."

Ah. Seems I'm being very inconsistent today.  However I would expect
you to need the device to remain powered up until end of probe
(assuming it needs to be powered up to read/write registers?) so indeed
a get to raise the reference count would be the way to ensure that.

> 
> > The runtime pm reference counters are protected against underflowing so this
> > probably just has no impact.  Still good to only have it if necessary and if
> > you do need the power to be on until this point, force it to do so by
> > an appropriate pm_runtime_get().  
> I will use a pm_runtime_get() in the apds9306_pm_init() function above.

Great. That makes sense.

> > 
> >   
> >> +
> >> +	return 0;
> >> +}  
> >   
> Thank you for your review.

Sorry for the inconsistencies!  

Jonathan
> 
> Regards,
> Subhajit Ghosh
>