Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop

From: Michael Nazzareno Trimarchi
Date: Thu Jun 02 2022 - 01:57:23 EST

Next message: Palmer Dabbelt: "Re: [PATCH v3] RISC-V: Mark IORESOURCE_EXCLUSIVE for reserved mem instead of IORESOURCE_BUSY"
Previous message: Dongliang Mu: "[PATCH] fs: nfsd: make destory function more elegant"
In reply to: Michael Nazzareno Trimarchi: "Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Mark

Add Dongchun Zhu, for the patch of regulator changes and mark brown to
clarify the API for bulk regulator.

The commit f1a64f56663e9d03e509439016dcbddd0166b2da states that the
regulator bulk api does not guarantee the order.
Can you help me with this?

On Wed, Jun 1, 2022 at 10:39 AM Michael Nazzareno Trimarchi
<michael@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> Hi
>
> On Wed, Jun 1, 2022 at 10:11 AM Jacopo Mondi <jacopo@xxxxxxxxxx> wrote:
> >
> > Hi Micheal,
> >
> > On Tue, May 31, 2022 at 05:50:51PM +0200, Michael Nazzareno Trimarchi wrote:
> > > Hi
> > >
> > > On Tue, May 31, 2022 at 5:40 PM Tommaso Merciai
> > > <tommaso.merciai@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > Hi Jacopo,
> > > > On Tue, May 31, 2022 at 03:14:09PM +0200, Jacopo Mondi wrote:
> > > > > Hi Tommaso,
> > > > >
> > > > > On Thu, May 19, 2022 at 09:51:14AM +0200, Tommaso Merciai wrote:
> > > > > > Enable regulator using regulator_bulk_enable/regulatore_bulk_disable
> > > > > > function in __ov5695_power_on/__ov5695_power_off function instead of for loop.
> > > > > > This reduce code size and make things more clear
> > > > > >
> > > > > > Signed-off-by: Tommaso Merciai <tommaso.merciai@xxxxxxxxxxxxxxxxxxxx>
> > > > > > Co-Developed-by: Michael Trimarchi <michael@xxxxxxxxxxxxxxxxxxxx>
> > > > > > ---
> > > > > > drivers/media/i2c/ov5695.c | 25 +++++++------------------
> > > > > > 1 file changed, 7 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/media/i2c/ov5695.c b/drivers/media/i2c/ov5695.c
> > > > > > index 439385938a51..880b586e55fe 100644
> > > > > > --- a/drivers/media/i2c/ov5695.c
> > > > > > +++ b/drivers/media/i2c/ov5695.c
> > > > > > @@ -972,7 +972,7 @@ static int ov5695_s_stream(struct v4l2_subdev *sd, int on)
> > > > > >
> > > > > > static int __ov5695_power_on(struct ov5695 *ov5695)
> > > > > > {
> > > > > > - int i, ret;
> > > > > > + int ret;
> > > > > > struct device *dev = &ov5695->client->dev;
> > > > > >
> > > > > > ret = clk_prepare_enable(ov5695->xvclk);
> > > > > > @@ -987,13 +987,10 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > > > > > * The hardware requires the regulators to be powered on in order,
> > > > > > * so enable them one by one.
> > > > > > */
> > > > >
> > > > > The comment says that the hardware requires regulators to be enabled
> > > > > in precise order
> > > > >
> > >
> > > They are enabled on the array order.
> > >
> > > > > > - for (i = 0; i < OV5695_NUM_SUPPLIES; i++) {
> > > > > > - ret = regulator_enable(ov5695->supplies[i].consumer);
> > > > > > - if (ret) {
> > > > > > - dev_err(dev, "Failed to enable %s: %d\n",
> > > > > > - ov5695->supplies[i].supply, ret);
> > > > > > - goto disable_reg_clk;
> > > > > > - }
> > > > > > + ret = regulator_bulk_enable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
> > > > >
> > > > > bulk_enable() uses the async API (async_schedule_domain() in
> > > > > particular) which by the name makes me think such ordering guarantee
> > > > > cannot be respected.
> > >
> > > I don't think so. Will make no sense because if it fails, revert them.
> > > Even the bulk disable disable them
> > > in reverse order
> > >
> >
> > I understand your points, but even the commit message in the patch
> > linked by Tommaso [1] (which I see in mainline as
> > f1a64f56663e ("media: i2c: ov5695: Fix power on and off sequences"))
> > reports:
> >
> > "Given the bulk API does not give any guarantee about the order of
> > regulators, change the driver to use regulator_disable() instead."
> >
> > However I would have expected the core regulator API to clearly document
> > this behaviour.
> >
>
> Yes, I agree. I see two points:
> - patch f1a64f56663e is not fully consistent
> - a patch is needed to the regulator api documentation
>
> I think that we need better documentation of the api but:
> Work-queues are SMP-safe and guarantee serialization of actual work performed.
>
> Michael
>
>
>
> >
> > > > >
> > > > > However most sensors require some kind of ordering when enabling
> > > > > regulators, and most of the use the bulk API anyhow. The fact this
> > > > > driver uses the bulk API to get an release the regulators but not for
> > > > > enabling them and the above comment, makes me think it has been done
> > > > > on purpose ? Could you check with the driver author maybe ?
> > > >
> > > > Thanks for suggestion, good question.
> > > > I see also ov5693 driver use bulk_enable/bulk_disable
> > > > on ov5693_sensor_powerdown and ov5693_sensor_powerup functions, I take
> > > > this as reference (and I'm wrong)
> > > >
> > > > In a functional test on PX30_Mini_evb_v11_20190507, after this series
> > > > I'm able to see the correct chip id during probe and do some capture.
> > > >
> > > > I think you are right Jacopo, we can drop off this [PATCH 1/4]
> > > > On the following link I found the issue that you describe: [1]
> > > >
> > >
> > > WHy drop?
> >
> > As this is a partial revert of [1].
> >
> > I think in practice this won't make any actual difference, but if not
> > 100% sure, better leave it the way it is as the authors of [1] might
> > have actually been experiencing issues. Even more as this patch is
> > not a bugfix but a nice-to-have. Up to you :)
> >
> >
> > >
> > > Michael
> > >
> > > > >
> > > > > > + if (ret) {
> > > > > > + dev_err(dev, "Failed to enable regulators %d\n", ret);
> > > > > > + goto disable_reg_clk;
> > > > > > }
> > > > > >
> > > > > > gpiod_set_value_cansleep(ov5695->reset_gpio, 0);
> > > > > > @@ -1003,8 +1000,7 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > > > > > return 0;
> > > > > >
> > > > > > disable_reg_clk:
> > > > > > - for (--i; i >= 0; i--)
> > > > > > - regulator_disable(ov5695->supplies[i].consumer);
> > > > > > + regulator_bulk_disable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
> > > > >
> > > > > FYI the bulk API does this for you if enabling any of the regulators fails.
> > > > > Hence this should not be necessary.
> > > >
> > > > Thanks for sharing! This is new to me.
> > > > I'll update the series on v2 removing this patch.
> > > >
> > > > Regards,
> > > > Tommaso
> > > >
> > > > [1]: https://mailweb.openeuler.org/hyperkitty/list/kernel@xxxxxxxxxxxxx/message/4X54QYJDRRE4K5BW4FTDZUGRAL4GRQWY/
> > > >
> > > > > Thanks
> > > > > j
> > > > >
> > > > > > clk_disable_unprepare(ov5695->xvclk);
> > > > > >
> > > > > > return ret;
> > > > > > @@ -1012,8 +1008,6 @@ static int __ov5695_power_on(struct ov5695 *ov5695)
> > > > > >
> > > > > > static void __ov5695_power_off(struct ov5695 *ov5695)
> > > > > > {
> > > > > > - struct device *dev = &ov5695->client->dev;
> > > > > > - int i, ret;
> > > > > >
> > > > > > clk_disable_unprepare(ov5695->xvclk);
> > > > > > gpiod_set_value_cansleep(ov5695->reset_gpio, 1);
> > > > > > @@ -1022,12 +1016,7 @@ static void __ov5695_power_off(struct ov5695 *ov5695)
> > > > > > * The hardware requires the regulators to be powered off in order,
> > > > > > * so disable them one by one.
> > > > > > */
> > > > > > - for (i = OV5695_NUM_SUPPLIES - 1; i >= 0; i--) {
> > > > > > - ret = regulator_disable(ov5695->supplies[i].consumer);
> > > > > > - if (ret)
> > > > > > - dev_err(dev, "Failed to disable %s: %d\n",
> > > > > > - ov5695->supplies[i].supply, ret);
> > > > > > - }
> > > > > > + regulator_bulk_disable(ARRAY_SIZE(ov5695->supplies), ov5695->supplies);
> > > > > > }
> > > > > >
> > > > > > static int __maybe_unused ov5695_runtime_resume(struct device *dev)
> > > > > > --
> > > > > > 2.25.1
> > > > > >
> > > >
> > > > --
> > > > Tommaso Merciai
> > > > Embedded Linux Engineer
> > > > tommaso.merciai@xxxxxxxxxxxxxxxxxxxx
> > > > __________________________________
> > > >
> > > > Amarula Solutions SRL
> > > > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > > > T. +39 042 243 5310
> > > > info@xxxxxxxxxxxxxxxxxxxx
> > > > www.amarulasolutions.com
> > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > michael@xxxxxxxxxxxxxxxxxxxx
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > info@xxxxxxxxxxxxxxxxxxxx
> > > www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@xxxxxxxxxxxxxxxxxxxx
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@xxxxxxxxxxxxxxxxxxxx
> www.amarulasolutions.com

--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
michael@xxxxxxxxxxxxxxxxxxxx
__________________________________

Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
info@xxxxxxxxxxxxxxxxxxxx
www.amarulasolutions.com

Next message: Palmer Dabbelt: "Re: [PATCH v3] RISC-V: Mark IORESOURCE_EXCLUSIVE for reserved mem instead of IORESOURCE_BUSY"
Previous message: Dongliang Mu: "[PATCH] fs: nfsd: make destory function more elegant"
In reply to: Michael Nazzareno Trimarchi: "Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]