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 >
