| Message ID | 20241128152338.4583-4-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: ov2740: Various improvements | expand |
On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The ov2740 sensor has both reset and power_down inputs according to > the datasheet one or the other should always be tied to DOVDD but on > some designs both are attached to GPIOs. > > Add support for controlling both a reset and a powerdown GPIO. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Ricardo Ribalda <ribalda@chromium.org> Same question as before :) > --- > drivers/media/i2c/ov2740.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c > index 998e1977978d..14d0a0588cc2 100644 > --- a/drivers/media/i2c/ov2740.c > +++ b/drivers/media/i2c/ov2740.c > @@ -525,6 +525,7 @@ struct ov2740 { > > /* GPIOs, clocks */ > struct gpio_desc *reset_gpio; > + struct gpio_desc *powerdown_gpio; > struct clk *clk; > > /* Current mode */ > @@ -1306,6 +1307,7 @@ static int ov2740_suspend(struct device *dev) > struct ov2740 *ov2740 = to_ov2740(sd); > > gpiod_set_value_cansleep(ov2740->reset_gpio, 1); > + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); > clk_disable_unprepare(ov2740->clk); > return 0; > } > @@ -1320,6 +1322,7 @@ static int ov2740_resume(struct device *dev) > if (ret) > return ret; > > + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); > gpiod_set_value_cansleep(ov2740->reset_gpio, 0); > msleep(20); > > @@ -1348,9 +1351,17 @@ static int ov2740_probe(struct i2c_client *client) > if (IS_ERR(ov2740->reset_gpio)) { > return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio), > "failed to get reset GPIO\n"); > - } else if (ov2740->reset_gpio) { > + } > + > + ov2740->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); > + if (IS_ERR(ov2740->powerdown_gpio)) { > + return dev_err_probe(dev, PTR_ERR(ov2740->powerdown_gpio), > + "failed to get powerdown GPIO\n"); > + } > + > + if (ov2740->reset_gpio || ov2740->powerdown_gpio) { > /* > - * Ensure reset is asserted for at least 20 ms before > + * Ensure reset/powerdown is asserted for at least 20 ms before > * ov2740_resume() deasserts it. > */ > msleep(20); > -- > 2.47.0 > >
On Thu, Nov 28, 2024 at 04:23:37PM +0100, Hans de Goede wrote: > The ov2740 sensor has both reset and power_down inputs according to > the datasheet one or the other should always be tied to DOVDD but on > some designs both are attached to GPIOs. > > Add support for controlling both a reset and a powerdown GPIO. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > --- > drivers/media/i2c/ov2740.c | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c > index 998e1977978d..14d0a0588cc2 100644 > --- a/drivers/media/i2c/ov2740.c > +++ b/drivers/media/i2c/ov2740.c > @@ -525,6 +525,7 @@ struct ov2740 { > > /* GPIOs, clocks */ > struct gpio_desc *reset_gpio; > + struct gpio_desc *powerdown_gpio; > struct clk *clk; > > /* Current mode */ > @@ -1306,6 +1307,7 @@ static int ov2740_suspend(struct device *dev) > struct ov2740 *ov2740 = to_ov2740(sd); > > gpiod_set_value_cansleep(ov2740->reset_gpio, 1); > + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); > clk_disable_unprepare(ov2740->clk); > return 0; > } > @@ -1320,6 +1322,7 @@ static int ov2740_resume(struct device *dev) > if (ret) > return ret; > > + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); > gpiod_set_value_cansleep(ov2740->reset_gpio, 0); > msleep(20); > > @@ -1348,9 +1351,17 @@ static int ov2740_probe(struct i2c_client *client) > if (IS_ERR(ov2740->reset_gpio)) { > return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio), > "failed to get reset GPIO\n"); > - } else if (ov2740->reset_gpio) { > + } > + > + ov2740->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); > + if (IS_ERR(ov2740->powerdown_gpio)) { > + return dev_err_probe(dev, PTR_ERR(ov2740->powerdown_gpio), > + "failed to get powerdown GPIO\n"); > + } > + > + if (ov2740->reset_gpio || ov2740->powerdown_gpio) { > /* > - * Ensure reset is asserted for at least 20 ms before > + * Ensure reset/powerdown is asserted for at least 20 ms before > * ov2740_resume() deasserts it. > */ > msleep(20); > -- > 2.47.0 > >
Hi, On 28-Nov-24 5:53 PM, Ricardo Ribalda Delgado wrote: > On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> The ov2740 sensor has both reset and power_down inputs according to >> the datasheet one or the other should always be tied to DOVDD but on >> some designs both are attached to GPIOs. >> >> Add support for controlling both a reset and a powerdown GPIO. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > Acked-by: Ricardo Ribalda <ribalda@chromium.org> > > Same question as before :) I assume that with this you mean if a call to v4l2_ctrl_handler_free() is necessary on errors ? That is not necessary in this case because the getting of the GPIO is done before ov2740_init_controls(). Regards, Hans > >> --- >> drivers/media/i2c/ov2740.c | 15 +++++++++++++-- >> 1 file changed, 13 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c >> index 998e1977978d..14d0a0588cc2 100644 >> --- a/drivers/media/i2c/ov2740.c >> +++ b/drivers/media/i2c/ov2740.c >> @@ -525,6 +525,7 @@ struct ov2740 { >> >> /* GPIOs, clocks */ >> struct gpio_desc *reset_gpio; >> + struct gpio_desc *powerdown_gpio; >> struct clk *clk; >> >> /* Current mode */ >> @@ -1306,6 +1307,7 @@ static int ov2740_suspend(struct device *dev) >> struct ov2740 *ov2740 = to_ov2740(sd); >> >> gpiod_set_value_cansleep(ov2740->reset_gpio, 1); >> + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); >> clk_disable_unprepare(ov2740->clk); >> return 0; >> } >> @@ -1320,6 +1322,7 @@ static int ov2740_resume(struct device *dev) >> if (ret) >> return ret; >> >> + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); >> gpiod_set_value_cansleep(ov2740->reset_gpio, 0); >> msleep(20); >> >> @@ -1348,9 +1351,17 @@ static int ov2740_probe(struct i2c_client *client) >> if (IS_ERR(ov2740->reset_gpio)) { >> return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio), >> "failed to get reset GPIO\n"); >> - } else if (ov2740->reset_gpio) { >> + } >> + >> + ov2740->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); >> + if (IS_ERR(ov2740->powerdown_gpio)) { >> + return dev_err_probe(dev, PTR_ERR(ov2740->powerdown_gpio), >> + "failed to get powerdown GPIO\n"); >> + } >> + >> + if (ov2740->reset_gpio || ov2740->powerdown_gpio) { >> /* >> - * Ensure reset is asserted for at least 20 ms before >> + * Ensure reset/powerdown is asserted for at least 20 ms before >> * ov2740_resume() deasserts it. >> */ >> msleep(20); >> -- >> 2.47.0 >> >> > >
On Fri, Dec 20, 2024 at 12:35 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Hi, > > On 28-Nov-24 5:53 PM, Ricardo Ribalda Delgado wrote: > > On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> The ov2740 sensor has both reset and power_down inputs according to > >> the datasheet one or the other should always be tied to DOVDD but on > >> some designs both are attached to GPIOs. > >> > >> Add support for controlling both a reset and a powerdown GPIO. > >> > >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > > Acked-by: Ricardo Ribalda <ribalda@chromium.org> > > > > Same question as before :) > > I assume that with this you mean if a call to v4l2_ctrl_handler_free() > is necessary on errors ? I actually meant if we needed to change a dt binding... but Sakari already clarified that this is an acpi only driver. Thanks! > > That is not necessary in this case because the getting of the > GPIO is done before ov2740_init_controls(). > > Regards, > > Hans > > > > > >> --- > >> drivers/media/i2c/ov2740.c | 15 +++++++++++++-- > >> 1 file changed, 13 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c > >> index 998e1977978d..14d0a0588cc2 100644 > >> --- a/drivers/media/i2c/ov2740.c > >> +++ b/drivers/media/i2c/ov2740.c > >> @@ -525,6 +525,7 @@ struct ov2740 { > >> > >> /* GPIOs, clocks */ > >> struct gpio_desc *reset_gpio; > >> + struct gpio_desc *powerdown_gpio; > >> struct clk *clk; > >> > >> /* Current mode */ > >> @@ -1306,6 +1307,7 @@ static int ov2740_suspend(struct device *dev) > >> struct ov2740 *ov2740 = to_ov2740(sd); > >> > >> gpiod_set_value_cansleep(ov2740->reset_gpio, 1); > >> + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); > >> clk_disable_unprepare(ov2740->clk); > >> return 0; > >> } > >> @@ -1320,6 +1322,7 @@ static int ov2740_resume(struct device *dev) > >> if (ret) > >> return ret; > >> > >> + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); > >> gpiod_set_value_cansleep(ov2740->reset_gpio, 0); > >> msleep(20); > >> > >> @@ -1348,9 +1351,17 @@ static int ov2740_probe(struct i2c_client *client) > >> if (IS_ERR(ov2740->reset_gpio)) { > >> return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio), > >> "failed to get reset GPIO\n"); > >> - } else if (ov2740->reset_gpio) { > >> + } > >> + > >> + ov2740->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); > >> + if (IS_ERR(ov2740->powerdown_gpio)) { > >> + return dev_err_probe(dev, PTR_ERR(ov2740->powerdown_gpio), > >> + "failed to get powerdown GPIO\n"); > >> + } > >> + > >> + if (ov2740->reset_gpio || ov2740->powerdown_gpio) { > >> /* > >> - * Ensure reset is asserted for at least 20 ms before > >> + * Ensure reset/powerdown is asserted for at least 20 ms before > >> * ov2740_resume() deasserts it. > >> */ > >> msleep(20); > >> -- > >> 2.47.0 > >> > >> > > > > >
diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c index 998e1977978d..14d0a0588cc2 100644 --- a/drivers/media/i2c/ov2740.c +++ b/drivers/media/i2c/ov2740.c @@ -525,6 +525,7 @@ struct ov2740 { /* GPIOs, clocks */ struct gpio_desc *reset_gpio; + struct gpio_desc *powerdown_gpio; struct clk *clk; /* Current mode */ @@ -1306,6 +1307,7 @@ static int ov2740_suspend(struct device *dev) struct ov2740 *ov2740 = to_ov2740(sd); gpiod_set_value_cansleep(ov2740->reset_gpio, 1); + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1); clk_disable_unprepare(ov2740->clk); return 0; } @@ -1320,6 +1322,7 @@ static int ov2740_resume(struct device *dev) if (ret) return ret; + gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0); gpiod_set_value_cansleep(ov2740->reset_gpio, 0); msleep(20); @@ -1348,9 +1351,17 @@ static int ov2740_probe(struct i2c_client *client) if (IS_ERR(ov2740->reset_gpio)) { return dev_err_probe(dev, PTR_ERR(ov2740->reset_gpio), "failed to get reset GPIO\n"); - } else if (ov2740->reset_gpio) { + } + + ov2740->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown", GPIOD_OUT_HIGH); + if (IS_ERR(ov2740->powerdown_gpio)) { + return dev_err_probe(dev, PTR_ERR(ov2740->powerdown_gpio), + "failed to get powerdown GPIO\n"); + } + + if (ov2740->reset_gpio || ov2740->powerdown_gpio) { /* - * Ensure reset is asserted for at least 20 ms before + * Ensure reset/powerdown is asserted for at least 20 ms before * ov2740_resume() deasserts it. */ msleep(20);
The ov2740 sensor has both reset and power_down inputs according to the datasheet one or the other should always be tied to DOVDD but on some designs both are attached to GPIOs. Add support for controlling both a reset and a powerdown GPIO. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/media/i2c/ov2740.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)