* [PATCH 0/4] arm64: dts: rockchip: px30: fix ov5695 camera probe @ 2022-05-19 7:51 Tommaso Merciai 2022-05-19 7:51 ` [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop Tommaso Merciai ` (3 more replies) 0 siblings, 4 replies; 15+ messages in thread From: Tommaso Merciai @ 2022-05-19 7:51 UTC (permalink / raw) Cc: tommaso.merciai, linuxfancy, linux-amarula, michael, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop 2022-05-19 7:51 [PATCH 0/4] arm64: dts: rockchip: px30: fix ov5695 camera probe Tommaso Merciai @ 2022-05-19 7:51 ` Tommaso Merciai 2022-05-31 13:14 ` Jacopo Mondi 2022-05-19 7:51 ` [PATCH 2/4] arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0 Tommaso Merciai ` (2 subsequent siblings) 3 siblings, 1 reply; 15+ messages in thread From: Tommaso Merciai @ 2022-05-19 7:51 UTC (permalink / raw) Cc: tommaso.merciai, linuxfancy, linux-amarula, michael, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop 2022-05-19 7:51 ` [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop Tommaso Merciai @ 2022-05-31 13:14 ` Jacopo Mondi 2022-05-31 15:40 ` Tommaso Merciai 0 siblings, 1 reply; 15+ messages in thread From: Jacopo Mondi @ 2022-05-31 13:14 UTC (permalink / raw) To: Tommaso Merciai Cc: linuxfancy, linux-amarula, michael, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop 2022-05-31 13:14 ` Jacopo Mondi @ 2022-05-31 15:40 ` Tommaso Merciai 2022-05-31 15:50 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 15+ messages in thread From: Tommaso Merciai @ 2022-05-31 15:40 UTC (permalink / raw) To: Jacopo Mondi Cc: linuxfancy, linux-amarula, michael, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop 2022-05-31 15:40 ` Tommaso Merciai @ 2022-05-31 15:50 ` Michael Nazzareno Trimarchi 2022-06-01 8:11 ` Jacopo Mondi 0 siblings, 1 reply; 15+ messages in thread From: Michael Nazzareno Trimarchi @ 2022-05-31 15:50 UTC (permalink / raw) To: Tommaso Merciai Cc: Jacopo Mondi, linuxfancy, linux-amarula, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop 2022-05-31 15:50 ` Michael Nazzareno Trimarchi @ 2022-06-01 8:11 ` Jacopo Mondi 2022-06-01 8:39 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 15+ messages in thread From: Jacopo Mondi @ 2022-06-01 8:11 UTC (permalink / raw) To: Michael Nazzareno Trimarchi Cc: Tommaso Merciai, linuxfancy, linux-amarula, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop 2022-06-01 8:11 ` Jacopo Mondi @ 2022-06-01 8:39 ` Michael Nazzareno Trimarchi 2022-06-02 5:57 ` Michael Nazzareno Trimarchi 0 siblings, 1 reply; 15+ messages in thread From: Michael Nazzareno Trimarchi @ 2022-06-01 8:39 UTC (permalink / raw) To: Jacopo Mondi Cc: Tommaso Merciai, linuxfancy, linux-amarula, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop 2022-06-01 8:39 ` Michael Nazzareno Trimarchi @ 2022-06-02 5:57 ` Michael Nazzareno Trimarchi 2022-06-09 10:11 ` Tommaso Merciai 0 siblings, 1 reply; 15+ messages in thread From: Michael Nazzareno Trimarchi @ 2022-06-02 5:57 UTC (permalink / raw) To: Jacopo Mondi, Dongchun Zhu, Mark Brown Cc: Tommaso Merciai, linuxfancy, linux-amarula, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop 2022-06-02 5:57 ` Michael Nazzareno Trimarchi @ 2022-06-09 10:11 ` Tommaso Merciai 0 siblings, 0 replies; 15+ messages in thread From: Tommaso Merciai @ 2022-06-09 10:11 UTC (permalink / raw) To: Michael Nazzareno Trimarchi Cc: Jacopo Mondi, Dongchun Zhu, Mark Brown, linuxfancy, linux-amarula, Shunqian Zheng, Mauro Carvalho Chehab, linux-media, linux-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0 2022-05-19 7:51 [PATCH 0/4] arm64: dts: rockchip: px30: fix ov5695 camera probe Tommaso Merciai 2022-05-19 7:51 ` [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop Tommaso Merciai @ 2022-05-19 7:51 ` Tommaso Merciai 2022-05-23 8:17 ` Michael Nazzareno Trimarchi 2022-05-19 7:51 ` [PATCH 3/4] arm64: dts: rockchip: px30: add mux for mipi-pdn pad Tommaso Merciai 2022-05-19 7:51 ` [PATCH 4/4] arm64: dts: rockchip: px30: use rk gpio naming convention into reset-gpios Tommaso Merciai 3 siblings, 1 reply; 15+ messages in thread From: Tommaso Merciai @ 2022-05-19 7:51 UTC (permalink / raw) Cc: tommaso.merciai, linuxfancy, linux-amarula, michael, Rob Herring, Krzysztof Kozlowski, Heiko Stuebner, Shunqian Zheng, Mauro Carvalho Chehab, devicetree, linux-arm-kernel, linux-rockchip, linux-kernel, linux-media 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/4] arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0 2022-05-19 7:51 ` [PATCH 2/4] arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0 Tommaso Merciai @ 2022-05-23 8:17 ` Michael Nazzareno Trimarchi 0 siblings, 0 replies; 15+ messages in thread From: Michael Nazzareno Trimarchi @ 2022-05-23 8:17 UTC (permalink / raw) To: Tommaso Merciai Cc: linuxfancy, linux-amarula, Rob Herring, Krzysztof Kozlowski, Heiko Stuebner, Shunqian Zheng, Mauro Carvalho Chehab, devicetree, linux-arm-kernel, linux-rockchip, linux-kernel, linux-media 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] arm64: dts: rockchip: px30: add mux for mipi-pdn pad 2022-05-19 7:51 [PATCH 0/4] arm64: dts: rockchip: px30: fix ov5695 camera probe Tommaso Merciai 2022-05-19 7:51 ` [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop Tommaso Merciai 2022-05-19 7:51 ` [PATCH 2/4] arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0 Tommaso Merciai @ 2022-05-19 7:51 ` Tommaso Merciai 2022-05-23 8:18 ` Michael Nazzareno Trimarchi 2022-05-19 7:51 ` [PATCH 4/4] arm64: dts: rockchip: px30: use rk gpio naming convention into reset-gpios Tommaso Merciai 3 siblings, 1 reply; 15+ messages in thread From: Tommaso Merciai @ 2022-05-19 7:51 UTC (permalink / raw) Cc: tommaso.merciai, linuxfancy, linux-amarula, michael, Rob Herring, Krzysztof Kozlowski, Heiko Stuebner, Shunqian Zheng, Mauro Carvalho Chehab, devicetree, linux-arm-kernel, linux-rockchip, linux-kernel, linux-media 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/4] arm64: dts: rockchip: px30: add mux for mipi-pdn pad 2022-05-19 7:51 ` [PATCH 3/4] arm64: dts: rockchip: px30: add mux for mipi-pdn pad Tommaso Merciai @ 2022-05-23 8:18 ` Michael Nazzareno Trimarchi 0 siblings, 0 replies; 15+ messages in thread From: Michael Nazzareno Trimarchi @ 2022-05-23 8:18 UTC (permalink / raw) To: Tommaso Merciai Cc: linuxfancy, linux-amarula, Rob Herring, Krzysztof Kozlowski, Heiko Stuebner, Shunqian Zheng, Mauro Carvalho Chehab, devicetree, linux-arm-kernel, linux-rockchip, linux-kernel, linux-media 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] arm64: dts: rockchip: px30: use rk gpio naming convention into reset-gpios 2022-05-19 7:51 [PATCH 0/4] arm64: dts: rockchip: px30: fix ov5695 camera probe Tommaso Merciai ` (2 preceding siblings ...) 2022-05-19 7:51 ` [PATCH 3/4] arm64: dts: rockchip: px30: add mux for mipi-pdn pad Tommaso Merciai @ 2022-05-19 7:51 ` Tommaso Merciai 2022-05-23 8:16 ` Michael Nazzareno Trimarchi 3 siblings, 1 reply; 15+ messages in thread From: Tommaso Merciai @ 2022-05-19 7:51 UTC (permalink / raw) Cc: tommaso.merciai, linuxfancy, linux-amarula, michael, Rob Herring, Krzysztof Kozlowski, Heiko Stuebner, Shunqian Zheng, Mauro Carvalho Chehab, devicetree, linux-arm-kernel, linux-rockchip, linux-kernel, linux-media 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] arm64: dts: rockchip: px30: use rk gpio naming convention into reset-gpios 2022-05-19 7:51 ` [PATCH 4/4] arm64: dts: rockchip: px30: use rk gpio naming convention into reset-gpios Tommaso Merciai @ 2022-05-23 8:16 ` Michael Nazzareno Trimarchi 0 siblings, 0 replies; 15+ messages in thread From: Michael Nazzareno Trimarchi @ 2022-05-23 8:16 UTC (permalink / raw) To: Tommaso Merciai Cc: linuxfancy, linux-amarula, Rob Herring, Krzysztof Kozlowski, Heiko Stuebner, Shunqian Zheng, Mauro Carvalho Chehab, devicetree, linux-arm-kernel, linux-rockchip, linux-kernel, linux-media 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2022-06-09 10:12 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-19 7:51 [PATCH 0/4] arm64: dts: rockchip: px30: fix ov5695 camera probe Tommaso Merciai 2022-05-19 7:51 ` [PATCH 1/4] media: i2c: ov5695: use regulator_bulk_enable/regulator_bulk disable instead of for loop Tommaso Merciai 2022-05-31 13:14 ` Jacopo Mondi 2022-05-31 15:40 ` Tommaso Merciai 2022-05-31 15:50 ` Michael Nazzareno Trimarchi 2022-06-01 8:11 ` Jacopo Mondi 2022-06-01 8:39 ` Michael Nazzareno Trimarchi 2022-06-02 5:57 ` Michael Nazzareno Trimarchi 2022-06-09 10:11 ` Tommaso Merciai 2022-05-19 7:51 ` [PATCH 2/4] arm64: dts: rockchip: px30: max drive-strength for cif_clkout_m0 Tommaso Merciai 2022-05-23 8:17 ` Michael Nazzareno Trimarchi 2022-05-19 7:51 ` [PATCH 3/4] arm64: dts: rockchip: px30: add mux for mipi-pdn pad Tommaso Merciai 2022-05-23 8:18 ` Michael Nazzareno Trimarchi 2022-05-19 7:51 ` [PATCH 4/4] arm64: dts: rockchip: px30: use rk gpio naming convention into reset-gpios Tommaso Merciai 2022-05-23 8:16 ` Michael Nazzareno Trimarchi
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).