[PATCH 0/4] arm64: dts: rockchip: px30: fix ov5695 camera probe

[PATCH 0/4] arm64: dts: rockchip: px30: fix ov5695 camera probe

[Tommaso Merciai]

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

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

[Tommaso Merciai]

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@amarulasolutions.com>
Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
---
 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

[PATCH 2/4] arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0

[Tommaso Merciai]

Add max drive-strength for cif_clkout_m0. This fix the issue that
sometimes camera ov5695 is not probed correctly.
Tested on PX30_Mini_EVB_V11_20190507

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
Tested-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
 arch/arm64/boot/dts/rockchip/px30-evb.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/px30-evb.dts b/arch/arm64/boot/dts/rockchip/px30-evb.dts
index 848bc39cf86a..53930e28eadf 100644
--- a/arch/arm64/boot/dts/rockchip/px30-evb.dts
+++ b/arch/arm64/boot/dts/rockchip/px30-evb.dts
@@ -537,6 +537,13 @@ wifi_enable_h: wifi-enable-h {
 				<0 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
 	};
+
+	cif-m0 {
+		cif_clkout_m0: cif-clkout-m0 {
+			rockchip,pins =
+				<2 RK_PB3 1 &pcfg_pull_none_12ma>;
+		};
+	};
 };
 
 &pmu_io_domains {
-- 
2.25.1

[PATCH 3/4] arm64: dts: rockchip: px30: add mux for mipi-pdn pad

[Tommaso Merciai]

Add right mux for mipi-pdn. Mux this pad as gpio2 14

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
Tested-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
 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

[PATCH 4/4] arm64: dts: rockchip: px30: use rk gpio naming convention into reset-gpios

[Tommaso Merciai]

Use rk gpio naming convention into reset-gpios of ov5695 camera

Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
Tested-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
---
 arch/arm64/boot/dts/rockchip/px30-evb.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/px30-evb.dts b/arch/arm64/boot/dts/rockchip/px30-evb.dts
index 0d05a1b098bc..07008d84434c 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";
-		reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>;
 		pinctrl-0 = <&cif_clkout_m0 &mipi_pdn>;
+		reset-gpios = <&gpio2 RK_PB6 GPIO_ACTIVE_LOW>;
 
 		port {
 			ucam_out: endpoint {
-- 
2.25.1

Re: [PATCH 4/4] arm64: dts: rockchip: px30: use rk gpio naming convention into reset-gpios

[Michael Nazzareno Trimarchi]

Hi

On Thu, May 19, 2022 at 9:51 AM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> wrote:
>
> Use rk gpio naming convention into reset-gpios of ov5695 camera
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Tested-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
>  arch/arm64/boot/dts/rockchip/px30-evb.dts | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/px30-evb.dts b/arch/arm64/boot/dts/rockchip/px30-evb.dts
> index 0d05a1b098bc..07008d84434c 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";
> -               reset-gpios = <&gpio2 14 GPIO_ACTIVE_LOW>;
>                 pinctrl-0 = <&cif_clkout_m0 &mipi_pdn>;
> +               reset-gpios = <&gpio2 RK_PB6 GPIO_ACTIVE_LOW>;
>

Reviewed-by: Michael Trimarchi <michael@amarulasolutions.com>

>                 port {
>                         ucam_out: endpoint {
> --
> 2.25.1
>


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

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

Re: [PATCH 2/4] arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0

[Michael Nazzareno Trimarchi]

Hi

On Thu, May 19, 2022 at 9:51 AM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> wrote:
>
> Add max drive-strength for cif_clkout_m0. This fix the issue that
> sometimes camera ov5695 is not probed correctly.
> Tested on PX30_Mini_EVB_V11_20190507
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Tested-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
>  arch/arm64/boot/dts/rockchip/px30-evb.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/rockchip/px30-evb.dts b/arch/arm64/boot/dts/rockchip/px30-evb.dts
> index 848bc39cf86a..53930e28eadf 100644
> --- a/arch/arm64/boot/dts/rockchip/px30-evb.dts
> +++ b/arch/arm64/boot/dts/rockchip/px30-evb.dts
> @@ -537,6 +537,13 @@ wifi_enable_h: wifi-enable-h {
>                                 <0 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
>                 };
>         };
> +
> +       cif-m0 {
> +               cif_clkout_m0: cif-clkout-m0 {
> +                       rockchip,pins =
> +                               <2 RK_PB3 1 &pcfg_pull_none_12ma>;
> +               };
> +       };
>  };

This is the same now on rockchip bsp

Reviewed-by: Michael Trimarchi <michael@amarulasolutios.com>

>
>  &pmu_io_domains {
> --
> 2.25.1
>


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

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

Re: [PATCH 3/4] arm64: dts: rockchip: px30: add mux for mipi-pdn pad

[Michael Nazzareno Trimarchi]

Hi

On Thu, May 19, 2022 at 9:51 AM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> wrote:
>
> Add right mux for mipi-pdn. Mux this pad as gpio2 14
>
> Signed-off-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> Tested-by: Tommaso Merciai <tommaso.merciai@amarulasolutions.com>
> ---
>  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 <michael@amarulasolutions.com>

Michael
>  &pmu_io_domains {
> --
> 2.25.1
>


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

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

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

[Jacopo Mondi]

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@amarulasolutions.com>
> Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
>  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
>

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

[Tommaso Merciai]

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@amarulasolutions.com>
> > Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > ---
> >  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/kernel@openeuler.org/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@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com

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

[Michael Nazzareno Trimarchi]

Hi

On Tue, May 31, 2022 at 5:40 PM Tommaso Merciai
<tommaso.merciai@amarulasolutions.com> 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@amarulasolutions.com>
> > > Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > ---
> > >  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/kernel@openeuler.org/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@amarulasolutions.com
> __________________________________
>
> Amarula Solutions SRL
> Via Le Canevare 30, 31100 Treviso, Veneto, IT
> T. +39 042 243 5310
> info@amarulasolutions.com
> www.amarulasolutions.com



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

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

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

[Jacopo Mondi]

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@amarulasolutions.com> 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@amarulasolutions.com>
> > > > Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > ---
> > > >  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/kernel@openeuler.org/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@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions SRL
> > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > T. +39 042 243 5310
> > info@amarulasolutions.com
> > www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

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

[Michael Nazzareno Trimarchi]

Hi

On Wed, Jun 1, 2022 at 10:11 AM Jacopo Mondi <jacopo@jmondi.org> 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@amarulasolutions.com> 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@amarulasolutions.com>
> > > > > Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > ---
> > > > >  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@openeuler.org/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@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions SRL
> > > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > > T. +39 042 243 5310
> > > info@amarulasolutions.com
> > > www.amarulasolutions.com
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > www.amarulasolutions.com



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

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

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

[Michael Nazzareno Trimarchi]

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@amarulasolutions.com> wrote:
>
> Hi
>
> On Wed, Jun 1, 2022 at 10:11 AM Jacopo Mondi <jacopo@jmondi.org> 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@amarulasolutions.com> 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@amarulasolutions.com>
> > > > > > Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > > ---
> > > > > >  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@openeuler.org/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@amarulasolutions.com
> > > > __________________________________
> > > >
> > > > Amarula Solutions SRL
> > > > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > > > T. +39 042 243 5310
> > > > info@amarulasolutions.com
> > > > www.amarulasolutions.com
> > >
> > >
> > >
> > > --
> > > Michael Nazzareno Trimarchi
> > > Co-Founder & Chief Executive Officer
> > > M. +39 347 913 2170
> > > michael@amarulasolutions.com
> > > __________________________________
> > >
> > > Amarula Solutions BV
> > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > T. +31 (0)85 111 9172
> > > info@amarulasolutions.com
> > > www.amarulasolutions.com
>
>
>
> --
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
>
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com



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

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

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

[Tommaso Merciai]

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
> <michael@amarulasolutions.com> wrote:
> >
> > Hi
> >
> > On Wed, Jun 1, 2022 at 10:11 AM Jacopo Mondi <jacopo@jmondi.org> 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@amarulasolutions.com> 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@amarulasolutions.com>
> > > > > > > Co-Developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> > > > > > > ---
> > > > > > >  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@openeuler.org/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@amarulasolutions.com
> > > > > __________________________________
> > > > >
> > > > > Amarula Solutions SRL
> > > > > Via Le Canevare 30, 31100 Treviso, Veneto, IT
> > > > > T. +39 042 243 5310
> > > > > info@amarulasolutions.com
> > > > > www.amarulasolutions.com
> > > >
> > > >
> > > >
> > > > --
> > > > Michael Nazzareno Trimarchi
> > > > Co-Founder & Chief Executive Officer
> > > > M. +39 347 913 2170
> > > > michael@amarulasolutions.com
> > > > __________________________________
> > > >
> > > > Amarula Solutions BV
> > > > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > > > T. +31 (0)85 111 9172
> > > > info@amarulasolutions.com
> > > > www.amarulasolutions.com
> >
> >
> >
> > --
> > Michael Nazzareno Trimarchi
> > Co-Founder & Chief Executive Officer
> > M. +39 347 913 2170
> > michael@amarulasolutions.com
> > __________________________________
> >
> > Amarula Solutions BV
> > Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> > T. +31 (0)85 111 9172
> > info@amarulasolutions.com
> > www.amarulasolutions.com
> 
> 
> 
> -- 
> Michael Nazzareno Trimarchi
> Co-Founder & Chief Executive Officer
> M. +39 347 913 2170
> michael@amarulasolutions.com
> __________________________________
> 
> Amarula Solutions BV
> Joop Geesinkweg 125, 1114 AB, Amsterdam, NL
> T. +31 (0)85 111 9172
> info@amarulasolutions.com
> www.amarulasolutions.com

-- 
Tommaso Merciai
Embedded Linux Engineer
tommaso.merciai@amarulasolutions.com
__________________________________

Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
info@amarulasolutions.com
www.amarulasolutions.com