Hi,
This series fix camera probing issue of ov5695 camera driver for px30 evb.
Improve also the ov5695.c driver using using regulator_bulk_enable/regulatore_bulk_disable
function in __ov5695_power_on/__ov5695_power_off functions instead of for loop
1. use regulator_bulk_enable/regulator_bulk disable instead of for loop into
ov5695 power_on/power_off
2. max drive-strength for cif_clkout_m0
3. add mux for mipi-pdn pad
4. use rk gpio naming convention for reset-gpio of ov5695
Note:
- This series was tested on PX30_Mini_EVB_V11_20190507 board
Tommaso Merciai (4):
media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable
instead of for loop
arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0
arm64: dts: rockchip: px30: add mux for mipi-pdn pad
arm64: dts: rockchip: px30: use rk gpio naming convention into
reset-gpios
arch/arm64/boot/dts/rockchip/px30-evb.dts | 17 +++++++++++++--
drivers/media/i2c/ov5695.c | 25 +++++++----------------
2 files changed, 22 insertions(+), 20 deletions(-)
--
2.25.1
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 <[email protected]>
Co-Developed-by: Michael Trimarchi <[email protected]>
---
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.
*/
- 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);
+ 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);
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
Add right mux for mipi-pdn. Mux this pad as gpio2 14
Signed-off-by: Tommaso Merciai <[email protected]>
Tested-by: Tommaso Merciai <[email protected]>
---
arch/arm64/boot/dts/rockchip/px30-evb.dts | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/rockchip/px30-evb.dts b/arch/arm64/boot/dts/rockchip/px30-evb.dts
index 53930e28eadf..0d05a1b098bc 100644
--- a/arch/arm64/boot/dts/rockchip/px30-evb.dts
+++ b/arch/arm64/boot/dts/rockchip/px30-evb.dts
@@ -450,8 +450,8 @@ ov5695: ov5695@36 {
dvdd-supply = <&vcc1v5_dvp>;
dovdd-supply = <&vcc1v8_dvp>;
pinctrl-names = "default";
- pinctrl-0 = <&cif_clkout_m0>;
reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>;
+ pinctrl-0 = <&cif_clkout_m0 &mipi_pdn>;
port {
ucam_out: endpoint {
@@ -544,6 +544,12 @@ cif_clkout_m0: cif-clkout-m0 {
<2 RK_PB3 1 &pcfg_pull_none_12ma>;
};
};
+
+ mipi {
+ mipi_pdn: mipi-pdn {
+ rockchip,pins = <2 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
+ };
+ };
};
&pmu_io_domains {
--
2.25.1
Hi
On Thu, May 19, 2022 at 9:51 AM Tommaso Merciai
<[email protected]> wrote:
>
> Add right mux for mipi-pdn. Mux this pad as gpio2 14
>
> Signed-off-by: Tommaso Merciai <[email protected]>
> Tested-by: Tommaso Merciai <[email protected]>
> ---
> arch/arm64/boot/dts/rockchip/px30-evb.dts | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/px30-evb.dts b/arch/arm64/boot/dts/rockchip/px30-evb.dts
> index 53930e28eadf..0d05a1b098bc 100644
> --- a/arch/arm64/boot/dts/rockchip/px30-evb.dts
> +++ b/arch/arm64/boot/dts/rockchip/px30-evb.dts
> @@ -450,8 +450,8 @@ ov5695: ov5695@36 {
> dvdd-supply = <&vcc1v5_dvp>;
> dovdd-supply = <&vcc1v8_dvp>;
> pinctrl-names = "default";
> - pinctrl-0 = <&cif_clkout_m0>;
> reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>;
> + pinctrl-0 = <&cif_clkout_m0 &mipi_pdn>;
>
> port {
> ucam_out: endpoint {
> @@ -544,6 +544,12 @@ cif_clkout_m0: cif-clkout-m0 {
> <2 RK_PB3 1 &pcfg_pull_none_12ma>;
> };
> };
> +
> + mipi {
> + mipi_pdn: mipi-pdn {
> + rockchip,pins = <2 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
> + };
> + };
> };
>
Reviewed-by: Michael Trimarchi <[email protected]>
Michael
> &pmu_io_domains {
> --
> 2.25.1
>
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com
Hi
On Tue, May 31, 2022 at 5:40 PM Tommaso Merciai
<[email protected]> 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 <[email protected]>
> > > Co-Developed-by: Michael Trimarchi <[email protected]>
> > > ---
> > > 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
> >
> > 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?
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/[email protected]/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
> [email protected]
> __________________________________
>
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> T. +39 042 243 5310
> [email protected]
> http://www.amarulasolutions.com
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com
Hi
On Wed, Jun 1, 2022 at 10:11 AM Jacopo Mondi <[email protected]> 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
> > <[email protected]> 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 <[email protected]>
> > > > > Co-Developed-by: Michael Trimarchi <[email protected]>
> > > > > ---
> > > > > 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/[email protected]/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
> > > [email protected]
> > > __________________________________
> > >
> > > Amarula Solutions SRL
> > > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > > T. +39 042 243 5310
> > > [email protected]
> > > http://www.amarulasolutions.com
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > [email protected]
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > [email protected]
> > http://www.amarulasolutions.com
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com
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 <[email protected]>
> > Co-Developed-by: Michael Trimarchi <[email protected]>
> > ---
> > 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
>
> > - 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.
>
> 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]
>
> > + 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/[email protected]/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
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
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 <[email protected]>
> Co-Developed-by: Michael Trimarchi <[email protected]>
> ---
> 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
> - 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.
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 ?
> + 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
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
>
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
> <[email protected]> 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 <[email protected]>
> > > > Co-Developed-by: Michael Trimarchi <[email protected]>
> > > > ---
> > > > 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.
> > >
> > > 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/[email protected]/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
> > [email protected]
> > __________________________________
> >
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > T. +39 042 243 5310
> > [email protected]
> > http://www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> [email protected]
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> [email protected]
> http://www.amarulasolutions.com
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
<[email protected]> wrote:
>
> Hi
>
> On Wed, Jun 1, 2022 at 10:11 AM Jacopo Mondi <[email protected]> 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
> > > <[email protected]> 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 <[email protected]>
> > > > > > Co-Developed-by: Michael Trimarchi <[email protected]>
> > > > > > ---
> > > > > > 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/[email protected]/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
> > > > [email protected]
> > > > __________________________________
> > > >
> > > > Amarula Solutions SRL
> > > > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > > > T. +39 042 243 5310
> > > > [email protected]
> > > > http://www.amarulasolutions.com
> > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > [email protected]
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > [email protected]
> > > http://www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> [email protected]
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> [email protected]
> http://www.amarulasolutions.com
--
Michael Nazzareno Trimarchi
Co-Founder & Chief Executive Officer
M. +39 347 913 2170
[email protected]
__________________________________
Amarula Solutions BV
Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
T. +31 (0)85 111 9172
[email protected]
http://www.amarulasolutions.com
Hi All,
On Thu, Jun 02, 2022 at 07:57:02AM +0200, Michael Nazzareno Trimarchi wrote:
> 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?
Just a gentle ping on this point.
Thanks
Regards,
Tommaso
>
> On Wed, Jun 1, 2022 at 10:39 AM Michael Nazzareno Trimarchi
> <[email protected]> wrote:
> >
> > Hi
> >
> > On Wed, Jun 1, 2022 at 10:11 AM Jacopo Mondi <[email protected]> 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
> > > > <[email protected]> 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 <[email protected]>
> > > > > > > Co-Developed-by: Michael Trimarchi <[email protected]>
> > > > > > > ---
> > > > > > > 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/[email protected]/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
> > > > > [email protected]
> > > > > __________________________________
> > > > >
> > > > > Amarula Solutions SRL
> > > > > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > > > > T. +39 042 243 5310
> > > > > [email protected]
> > > > > http://www.amarulasolutions.com
> > > >
> > > >
> > > >
> > > > --
> > > > Michael Nazzareno Trimarchi
> > > > Co-Founder & Chief Executive Officer
> > > > M. +39 347 913 2170
> > > > [email protected]
> > > > __________________________________
> > > >
> > > > Amarula Solutions BV
> > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > T. +31 (0)85 111 9172
> > > > [email protected]
> > > > http://www.amarulasolutions.com
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > [email protected]
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > [email protected]
> > http://www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> [email protected]
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> [email protected]
> http://www.amarulasolutions.com
--
Tommaso Merciai
Embedded Linux Engineer
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com