[PATCH 0/4] media: ov2740: Various improvements

[PATCH 0/4] media: ov2740: Various improvements

[Hans de Goede]

Hi All,

I have been working on getting the ov2740 front and back sensors on a Terra
Pad 1262 v2 with IPU6 to work.

Patches 1-2 are nice to have improvements and this tablet needs power-down
GPIO support for the sensors to work.

The regulator support turns out to not be necessary, so we could hold of
on merging it until some design actually needs it, OTOH it is nice to
already have it in place.

Regards,

Hans


Hans de Goede (4):
  media: ov2740: Debug log chip ID
  media: ov2740: Add camera orientation and sensor rotation controls
  media: ov2740: Add powerdown GPIO support
  media: ov2740: Add regulator support

 drivers/media/i2c/ov2740.c | 58 ++++++++++++++++++++++++++++++++++----
 1 file changed, 52 insertions(+), 6 deletions(-)

-- 
2.47.0

[PATCH 1/4] media: ov2740: Debug log chip ID

[Hans de Goede]

Calling the identify function may get delayed till the first stream-on,
add a dev_dbg() to it so that we know when it has run. This is useful
to debug bring-up problems related to regulators / clks / GPIOs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2740.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 387c529d9736..e7a611967b40 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -644,6 +644,8 @@ static int ov2740_identify_module(struct ov2740 *ov2740)
 		return -ENXIO;
 	}
 
+	dev_dbg(&client->dev, "chip id: %x\n", val);
+
 	ov2740->identified = true;
 
 	return 0;
-- 
2.47.0

[PATCH 2/4] media: ov2740: Add camera orientation and sensor rotation controls

[Hans de Goede]

Add camera orientation and sensor rotation controls using
the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
helpers.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2740.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index e7a611967b40..998e1977978d 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -755,15 +755,17 @@ static const struct v4l2_ctrl_ops ov2740_ctrl_ops = {
 
 static int ov2740_init_controls(struct ov2740 *ov2740)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	const struct ov2740_mode *cur_mode;
 	s64 exposure_max, h_blank, pixel_rate;
 	u32 vblank_min, vblank_max, vblank_default;
+	struct v4l2_fwnode_device_properties props;
 	int size;
 	int ret;
 
 	ctrl_hdlr = &ov2740->ctrl_handler;
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
 	if (ret)
 		return ret;
 
@@ -813,6 +815,13 @@ static int ov2740_init_controls(struct ov2740 *ov2740)
 				     V4L2_CID_TEST_PATTERN,
 				     ARRAY_SIZE(ov2740_test_pattern_menu) - 1,
 				     0, 0, ov2740_test_pattern_menu);
+
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		return ret;
+
+	v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2740_ctrl_ops, &props);
+
 	if (ctrl_hdlr->error) {
 		v4l2_ctrl_handler_free(ctrl_hdlr);
 		return ctrl_hdlr->error;
-- 
2.47.0

[PATCH 3/4] media: ov2740: Add powerdown GPIO support

[Hans de Goede]

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(-)

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

[PATCH 4/4] media: ov2740: Add regulator support

[Hans de Goede]

On some designs the regulators for the AVDD / DOVDD / DVDD power rails
are controlled by Linux.

Add support to the driver for getting regulators for these 3 rails and
for enabling these regulators when necessary.

The datasheet specifies a delay of 0ns between enabling the regulators,
IOW they can all 3 be enabled at the same time. This allows using the bulk
regulator API.

The regulator core will provide dummy regulators for the 3 power-rails
when necessary.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
index 14d0a0588cc2..c766c1f83e12 100644
--- a/drivers/media/i2c/ov2740.c
+++ b/drivers/media/i2c/ov2740.c
@@ -11,6 +11,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/nvmem-provider.h>
 #include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -76,6 +77,14 @@
 /* OTP registers from sensor */
 #define OV2740_REG_OTP_CUSTOMER		0x7010
 
+static const char * const ov2740_supply_name[] = {
+	"AVDD",
+	"DOVDD",
+	"DVDD",
+};
+
+#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name)
+
 struct nvm_data {
 	struct nvmem_device *nvmem;
 	struct regmap *regmap;
@@ -523,10 +532,11 @@ struct ov2740 {
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *exposure;
 
-	/* GPIOs, clocks */
+	/* GPIOs, clocks, regulators */
 	struct gpio_desc *reset_gpio;
 	struct gpio_desc *powerdown_gpio;
 	struct clk *clk;
+	struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES];
 
 	/* Current mode */
 	const struct ov2740_mode *cur_mode;
@@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev)
 	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
 	gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1);
 	clk_disable_unprepare(ov2740->clk);
+	regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
 	return 0;
 }
 
@@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev)
 	struct ov2740 *ov2740 = to_ov2740(sd);
 	int ret;
 
-	ret = clk_prepare_enable(ov2740->clk);
+	ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies);
 	if (ret)
 		return ret;
 
+	ret = clk_prepare_enable(ov2740->clk);
+	if (ret) {
+		regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
+		return ret;
+	}
+
 	gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0);
 	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
 	msleep(20);
@@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client)
 	struct device *dev = &client->dev;
 	struct ov2740 *ov2740;
 	bool full_power;
-	int ret;
+	int i, ret;
 
 	ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL);
 	if (!ov2740)
@@ -1372,6 +1389,13 @@ static int ov2740_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(ov2740->clk),
 				     "failed to get clock\n");
 
+	for (i = 0; i < OV2740_NUM_SUPPLIES; i++)
+		ov2740->supplies[i].supply = ov2740_supply_name[i];
+
+	ret = devm_regulator_bulk_get(dev, OV2740_NUM_SUPPLIES, ov2740->supplies);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to get regulators\n");
+
 	full_power = acpi_dev_state_d0(&client->dev);
 	if (full_power) {
 		/* ACPI does not always clear the reset GPIO / enable the clock */
-- 
2.47.0

Re: [PATCH 1/4] media: ov2740: Debug log chip ID

[Ricardo Ribalda Delgado]

On Thu, Nov 28, 2024 at 4:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Calling the identify function may get delayed till the first stream-on,
> add a dev_dbg() to it so that we know when it has run. This is useful
> to debug bring-up problems related to regulators / clks / GPIOs.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/i2c/ov2740.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 387c529d9736..e7a611967b40 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -644,6 +644,8 @@ static int ov2740_identify_module(struct ov2740 *ov2740)
>                 return -ENXIO;
>         }
>
> +       dev_dbg(&client->dev, "chip id: %x\n", val);
> +
nit: I would have written 0x%x instead, but the previous lines there
is %x != %x..
So up to you :)

>         ov2740->identified = true;
>
>         return 0;
> --
> 2.47.0
>
>


-- 
Ricardo Ribalda

Re: [PATCH 2/4] media: ov2740: Add camera orientation and sensor rotation controls

[Ricardo Ribalda Delgado]

Hi

On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Add camera orientation and sensor rotation controls using
> the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
> helpers.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2740.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index e7a611967b40..998e1977978d 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -755,15 +755,17 @@ static const struct v4l2_ctrl_ops ov2740_ctrl_ops = {
>
>  static int ov2740_init_controls(struct ov2740 *ov2740)
>  {
> +       struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
nit: dont you prefer to pass the i2c_client as a parameter?
>         struct v4l2_ctrl_handler *ctrl_hdlr;
>         const struct ov2740_mode *cur_mode;
>         s64 exposure_max, h_blank, pixel_rate;
>         u32 vblank_min, vblank_max, vblank_default;
> +       struct v4l2_fwnode_device_properties props;
>         int size;
>         int ret;
>
>         ctrl_hdlr = &ov2740->ctrl_handler;
> -       ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +       ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
>         if (ret)
>                 return ret;
>
> @@ -813,6 +815,13 @@ static int ov2740_init_controls(struct ov2740 *ov2740)
>                                      V4L2_CID_TEST_PATTERN,
>                                      ARRAY_SIZE(ov2740_test_pattern_menu) - 1,
>                                      0, 0, ov2740_test_pattern_menu);
> +
> +       ret = v4l2_fwnode_device_parse(&client->dev, &props);
> +       if (ret)
There is no need to v4l2_ctrl_handler_free(ctrl_hdlr); here?

> +               return ret;
> +
> +       v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2740_ctrl_ops, &props);
> +
>         if (ctrl_hdlr->error) {
>                 v4l2_ctrl_handler_free(ctrl_hdlr);
>                 return ctrl_hdlr->error;
> --
> 2.47.0
>
>


-- 
Ricardo Ribalda

Re: [PATCH 4/4] media: ov2740: Add regulator support

[Ricardo Ribalda Delgado]

On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> On some designs the regulators for the AVDD / DOVDD / DVDD power rails
> are controlled by Linux.
>
> Add support to the driver for getting regulators for these 3 rails and
> for enabling these regulators when necessary.
>
> The datasheet specifies a delay of 0ns between enabling the regulators,
> IOW they can all 3 be enabled at the same time. This allows using the bulk
> regulator API.
>
> The regulator core will provide dummy regulators for the 3 power-rails
> when necessary.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Ricardo Ribalda <ribalda@chromium.org>

Do we need to update the device tree with this change? (honest
question :), I have no idea)

> ---
>  drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 14d0a0588cc2..c766c1f83e12 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -11,6 +11,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -76,6 +77,14 @@
>  /* OTP registers from sensor */
>  #define OV2740_REG_OTP_CUSTOMER                0x7010
>
> +static const char * const ov2740_supply_name[] = {
> +       "AVDD",
> +       "DOVDD",
> +       "DVDD",
> +};
> +
> +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name)
> +
>  struct nvm_data {
>         struct nvmem_device *nvmem;
>         struct regmap *regmap;
> @@ -523,10 +532,11 @@ struct ov2740 {
>         struct v4l2_ctrl *hblank;
>         struct v4l2_ctrl *exposure;
>
> -       /* GPIOs, clocks */
> +       /* GPIOs, clocks, regulators */
>         struct gpio_desc *reset_gpio;
>         struct gpio_desc *powerdown_gpio;
>         struct clk *clk;
> +       struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES];
>
>         /* Current mode */
>         const struct ov2740_mode *cur_mode;
> @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev)
>         gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
>         gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1);
>         clk_disable_unprepare(ov2740->clk);
> +       regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>         return 0;
>  }
>
> @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev)
>         struct ov2740 *ov2740 = to_ov2740(sd);
>         int ret;
>
> -       ret = clk_prepare_enable(ov2740->clk);
> +       ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>         if (ret)
>                 return ret;
>
> +       ret = clk_prepare_enable(ov2740->clk);
> +       if (ret) {
> +               regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
> +               return ret;
> +       }
> +
>         gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0);
>         gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>         msleep(20);
> @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client)
>         struct device *dev = &client->dev;
>         struct ov2740 *ov2740;
>         bool full_power;
> -       int ret;
> +       int i, ret;
>
>         ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL);
>         if (!ov2740)
> @@ -1372,6 +1389,13 @@ static int ov2740_probe(struct i2c_client *client)
>                 return dev_err_probe(dev, PTR_ERR(ov2740->clk),
>                                      "failed to get clock\n");
>
> +       for (i = 0; i < OV2740_NUM_SUPPLIES; i++)
> +               ov2740->supplies[i].supply = ov2740_supply_name[i];
> +
> +       ret = devm_regulator_bulk_get(dev, OV2740_NUM_SUPPLIES, ov2740->supplies);
> +       if (ret)
> +               return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
>         full_power = acpi_dev_state_d0(&client->dev);
>         if (full_power) {
>                 /* ACPI does not always clear the reset GPIO / enable the clock */
> --
> 2.47.0
>
>


-- 
Ricardo Ribalda

Re: [PATCH 3/4] media: ov2740: Add powerdown GPIO support

[Ricardo Ribalda Delgado]

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
>
>


-- 
Ricardo Ribalda

Re: [PATCH 1/4] media: ov2740: Debug log chip ID

[Ricardo Ribalda Delgado]

On Thu, Nov 28, 2024 at 5:38 PM Ricardo Ribalda Delgado
<ricardo.ribalda@gmail.com> wrote:
>
> On Thu, Nov 28, 2024 at 4:23 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > Calling the identify function may get delayed till the first stream-on,
> > add a dev_dbg() to it so that we know when it has run. This is useful
> > to debug bring-up problems related to regulators / clks / GPIOs.
> >
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed by: Ricardo Ribalda <ribalda@chromium.org>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>

> > ---
> >  drivers/media/i2c/ov2740.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> > index 387c529d9736..e7a611967b40 100644
> > --- a/drivers/media/i2c/ov2740.c
> > +++ b/drivers/media/i2c/ov2740.c
> > @@ -644,6 +644,8 @@ static int ov2740_identify_module(struct ov2740 *ov2740)
> >                 return -ENXIO;
> >         }
> >
> > +       dev_dbg(&client->dev, "chip id: %x\n", val);
> > +
> nit: I would have written 0x%x instead, but the previous lines there
> is %x != %x..
> So up to you :)
>
> >         ov2740->identified = true;
> >
> >         return 0;
> > --
> > 2.47.0
> >
> >
>
>
> --
> Ricardo Ribalda



-- 
Ricardo Ribalda

Re: [PATCH 1/4] media: ov2740: Debug log chip ID

[Stanislaw Gruszka]

On Thu, Nov 28, 2024 at 04:23:35PM +0100, Hans de Goede wrote:
> Calling the identify function may get delayed till the first stream-on,
> add a dev_dbg() to it so that we know when it has run. This is useful
> to debug bring-up problems related to regulators / clks / GPIOs.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/media/i2c/ov2740.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 387c529d9736..e7a611967b40 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -644,6 +644,8 @@ static int ov2740_identify_module(struct ov2740 *ov2740)
>  		return -ENXIO;
>  	}
>  
> +	dev_dbg(&client->dev, "chip id: %x\n", val);
> +
>  	ov2740->identified = true;
>  
>  	return 0;
> -- 
> 2.47.0
> 
> 

Re: [PATCH 2/4] media: ov2740: Add camera orientation and sensor rotation controls

[Stanislaw Gruszka]

On Thu, Nov 28, 2024 at 04:23:36PM +0100, Hans de Goede wrote:
> Add camera orientation and sensor rotation controls using
> the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
> helpers.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

> ---
>  drivers/media/i2c/ov2740.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index e7a611967b40..998e1977978d 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -755,15 +755,17 @@ static const struct v4l2_ctrl_ops ov2740_ctrl_ops = {
>  
>  static int ov2740_init_controls(struct ov2740 *ov2740)
>  {
> +	struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
>  	const struct ov2740_mode *cur_mode;
>  	s64 exposure_max, h_blank, pixel_rate;
>  	u32 vblank_min, vblank_max, vblank_default;
> +	struct v4l2_fwnode_device_properties props;
>  	int size;
>  	int ret;
>  
>  	ctrl_hdlr = &ov2740->ctrl_handler;
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
>  	if (ret)
>  		return ret;
>  
> @@ -813,6 +815,13 @@ static int ov2740_init_controls(struct ov2740 *ov2740)
>  				     V4L2_CID_TEST_PATTERN,
>  				     ARRAY_SIZE(ov2740_test_pattern_menu) - 1,
>  				     0, 0, ov2740_test_pattern_menu);
> +
> +	ret = v4l2_fwnode_device_parse(&client->dev, &props);
> +	if (ret)
> +		return ret;
> +
> +	v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2740_ctrl_ops, &props);
> +
>  	if (ctrl_hdlr->error) {
>  		v4l2_ctrl_handler_free(ctrl_hdlr);
>  		return ctrl_hdlr->error;
> -- 
> 2.47.0
> 
> 

Re: [PATCH 3/4] media: ov2740: Add powerdown GPIO support

[Stanislaw Gruszka]

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
> 
> 

Re: [PATCH 4/4] media: ov2740: Add regulator support

[Stanislaw Gruszka]

On Thu, Nov 28, 2024 at 04:23:38PM +0100, Hans de Goede wrote:
> On some designs the regulators for the AVDD / DOVDD / DVDD power rails
> are controlled by Linux.
> 
> Add support to the driver for getting regulators for these 3 rails and
> for enabling these regulators when necessary.
> 
> The datasheet specifies a delay of 0ns between enabling the regulators,
> IOW they can all 3 be enabled at the same time. This allows using the bulk
> regulator API.
> 
> The regulator core will provide dummy regulators for the 3 power-rails
> when necessary.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

>  drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 14d0a0588cc2..c766c1f83e12 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -11,6 +11,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -76,6 +77,14 @@
>  /* OTP registers from sensor */
>  #define OV2740_REG_OTP_CUSTOMER		0x7010
>  
> +static const char * const ov2740_supply_name[] = {
> +	"AVDD",
> +	"DOVDD",
> +	"DVDD",
> +};
> +
> +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name)
> +
>  struct nvm_data {
>  	struct nvmem_device *nvmem;
>  	struct regmap *regmap;
> @@ -523,10 +532,11 @@ struct ov2740 {
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *exposure;
>  
> -	/* GPIOs, clocks */
> +	/* GPIOs, clocks, regulators */
>  	struct gpio_desc *reset_gpio;
>  	struct gpio_desc *powerdown_gpio;
>  	struct clk *clk;
> +	struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES];
>  
>  	/* Current mode */
>  	const struct ov2740_mode *cur_mode;
> @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev)
>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
>  	gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1);
>  	clk_disable_unprepare(ov2740->clk);
> +	regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>  	return 0;
>  }
>  
> @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev)
>  	struct ov2740 *ov2740 = to_ov2740(sd);
>  	int ret;
>  
> -	ret = clk_prepare_enable(ov2740->clk);
> +	ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>  	if (ret)
>  		return ret;
>  
> +	ret = clk_prepare_enable(ov2740->clk);
> +	if (ret) {
> +		regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
> +		return ret;
> +	}
> +
>  	gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0);
>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>  	msleep(20);
> @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client)
>  	struct device *dev = &client->dev;
>  	struct ov2740 *ov2740;
>  	bool full_power;
> -	int ret;
> +	int i, ret;
>  
>  	ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL);
>  	if (!ov2740)
> @@ -1372,6 +1389,13 @@ static int ov2740_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, PTR_ERR(ov2740->clk),
>  				     "failed to get clock\n");
>  
> +	for (i = 0; i < OV2740_NUM_SUPPLIES; i++)
> +		ov2740->supplies[i].supply = ov2740_supply_name[i];
> +
> +	ret = devm_regulator_bulk_get(dev, OV2740_NUM_SUPPLIES, ov2740->supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		/* ACPI does not always clear the reset GPIO / enable the clock */
> -- 
> 2.47.0
> 
> 

Re: [PATCH 4/4] media: ov2740: Add regulator support

[Sakari Ailus]

Hi Hans,

Thanks for the set.

On Thu, Nov 28, 2024 at 04:23:38PM +0100, Hans de Goede wrote:
> On some designs the regulators for the AVDD / DOVDD / DVDD power rails
> are controlled by Linux.
> 
> Add support to the driver for getting regulators for these 3 rails and
> for enabling these regulators when necessary.
> 
> The datasheet specifies a delay of 0ns between enabling the regulators,
> IOW they can all 3 be enabled at the same time. This allows using the bulk
> regulator API.
> 
> The regulator core will provide dummy regulators for the 3 power-rails
> when necessary.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
> index 14d0a0588cc2..c766c1f83e12 100644
> --- a/drivers/media/i2c/ov2740.c
> +++ b/drivers/media/i2c/ov2740.c
> @@ -11,6 +11,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-fwnode.h>
> @@ -76,6 +77,14 @@
>  /* OTP registers from sensor */
>  #define OV2740_REG_OTP_CUSTOMER		0x7010
>  
> +static const char * const ov2740_supply_name[] = {
> +	"AVDD",
> +	"DOVDD",
> +	"DVDD",
> +};
> +
> +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name)
> +
>  struct nvm_data {
>  	struct nvmem_device *nvmem;
>  	struct regmap *regmap;
> @@ -523,10 +532,11 @@ struct ov2740 {
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *exposure;
>  
> -	/* GPIOs, clocks */
> +	/* GPIOs, clocks, regulators */
>  	struct gpio_desc *reset_gpio;
>  	struct gpio_desc *powerdown_gpio;
>  	struct clk *clk;
> +	struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES];

This would be cleaner with plain ARRAY_SIZE(). Same below. I.e. the macro
is redundant.

>  
>  	/* Current mode */
>  	const struct ov2740_mode *cur_mode;
> @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev)
>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
>  	gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1);
>  	clk_disable_unprepare(ov2740->clk);
> +	regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>  	return 0;
>  }
>  
> @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev)
>  	struct ov2740 *ov2740 = to_ov2740(sd);
>  	int ret;
>  
> -	ret = clk_prepare_enable(ov2740->clk);
> +	ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>  	if (ret)
>  		return ret;
>  
> +	ret = clk_prepare_enable(ov2740->clk);
> +	if (ret) {
> +		regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
> +		return ret;
> +	}
> +
>  	gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0);
>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>  	msleep(20);
> @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client)
>  	struct device *dev = &client->dev;
>  	struct ov2740 *ov2740;
>  	bool full_power;
> -	int ret;
> +	int i, ret;

unsigned int i

>  
>  	ov2740 = devm_kzalloc(&client->dev, sizeof(*ov2740), GFP_KERNEL);
>  	if (!ov2740)
> @@ -1372,6 +1389,13 @@ static int ov2740_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, PTR_ERR(ov2740->clk),
>  				     "failed to get clock\n");
>  
> +	for (i = 0; i < OV2740_NUM_SUPPLIES; i++)
> +		ov2740->supplies[i].supply = ov2740_supply_name[i];
> +
> +	ret = devm_regulator_bulk_get(dev, OV2740_NUM_SUPPLIES, ov2740->supplies);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to get regulators\n");
> +
>  	full_power = acpi_dev_state_d0(&client->dev);
>  	if (full_power) {
>  		/* ACPI does not always clear the reset GPIO / enable the clock */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 4/4] media: ov2740: Add regulator support

[Sakari Ailus]

Hi Ricardo,

On Thu, Nov 28, 2024 at 05:50:33PM +0100, Ricardo Ribalda Delgado wrote:
> On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >
> > On some designs the regulators for the AVDD / DOVDD / DVDD power rails
> > are controlled by Linux.
> >
> > Add support to the driver for getting regulators for these 3 rails and
> > for enabling these regulators when necessary.
> >
> > The datasheet specifies a delay of 0ns between enabling the regulators,
> > IOW they can all 3 be enabled at the same time. This allows using the bulk
> > regulator API.
> >
> > The regulator core will provide dummy regulators for the 3 power-rails
> > when necessary.
> >
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> Do we need to update the device tree with this change? (honest
> question :), I have no idea)

This driver seems to be ACPI-only.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/4] media: ov2740: Add camera orientation and sensor rotation controls

[Hans de Goede]

Hi,

On 28-Nov-24 5:44 PM, Ricardo Ribalda Delgado wrote:
> Hi
> 
> On Thu, Nov 28, 2024 at 4:24 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Add camera orientation and sensor rotation controls using
>> the v4l2_fwnode_device_parse() and v4l2_ctrl_new_fwnode_properties()
>> helpers.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/ov2740.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
>> index e7a611967b40..998e1977978d 100644
>> --- a/drivers/media/i2c/ov2740.c
>> +++ b/drivers/media/i2c/ov2740.c
>> @@ -755,15 +755,17 @@ static const struct v4l2_ctrl_ops ov2740_ctrl_ops = {
>>
>>  static int ov2740_init_controls(struct ov2740 *ov2740)
>>  {
>> +       struct i2c_client *client = v4l2_get_subdevdata(&ov2740->sd);
> nit: dont you prefer to pass the i2c_client as a parameter?

The client is retrieved from the sub-device in the same way in other
places.

>>         struct v4l2_ctrl_handler *ctrl_hdlr;
>>         const struct ov2740_mode *cur_mode;
>>         s64 exposure_max, h_blank, pixel_rate;
>>         u32 vblank_min, vblank_max, vblank_default;
>> +       struct v4l2_fwnode_device_properties props;
>>         int size;
>>         int ret;
>>
>>         ctrl_hdlr = &ov2740->ctrl_handler;
>> -       ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
>> +       ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
>>         if (ret)
>>                 return ret;
>>
>> @@ -813,6 +815,13 @@ static int ov2740_init_controls(struct ov2740 *ov2740)
>>                                      V4L2_CID_TEST_PATTERN,
>>                                      ARRAY_SIZE(ov2740_test_pattern_menu) - 1,
>>                                      0, 0, ov2740_test_pattern_menu);
>> +
>> +       ret = v4l2_fwnode_device_parse(&client->dev, &props);
>> +       if (ret)
> There is no need to v4l2_ctrl_handler_free(ctrl_hdlr); here?

There is, good catch. I'll fix this for v2.

Regards,

Hans


> 
>> +               return ret;
>> +
>> +       v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &ov2740_ctrl_ops, &props);
>> +
>>         if (ctrl_hdlr->error) {
>>                 v4l2_ctrl_handler_free(ctrl_hdlr);
>>                 return ctrl_hdlr->error;
>> --
>> 2.47.0
>>
>>
> 
> 

Re: [PATCH 3/4] media: ov2740: Add powerdown GPIO support

[Hans de Goede]

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
>>
>>
> 
> 

Re: [PATCH 4/4] media: ov2740: Add regulator support

[Hans de Goede]

Hi,

On 13-Dec-24 10:04 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the set.
> 
> On Thu, Nov 28, 2024 at 04:23:38PM +0100, Hans de Goede wrote:
>> On some designs the regulators for the AVDD / DOVDD / DVDD power rails
>> are controlled by Linux.
>>
>> Add support to the driver for getting regulators for these 3 rails and
>> for enabling these regulators when necessary.
>>
>> The datasheet specifies a delay of 0ns between enabling the regulators,
>> IOW they can all 3 be enabled at the same time. This allows using the bulk
>> regulator API.
>>
>> The regulator core will provide dummy regulators for the 3 power-rails
>> when necessary.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/media/i2c/ov2740.c | 30 +++++++++++++++++++++++++++---
>>  1 file changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov2740.c b/drivers/media/i2c/ov2740.c
>> index 14d0a0588cc2..c766c1f83e12 100644
>> --- a/drivers/media/i2c/ov2740.c
>> +++ b/drivers/media/i2c/ov2740.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/pm_runtime.h>
>>  #include <linux/nvmem-provider.h>
>>  #include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>>  #include <media/v4l2-ctrls.h>
>>  #include <media/v4l2-device.h>
>>  #include <media/v4l2-fwnode.h>
>> @@ -76,6 +77,14 @@
>>  /* OTP registers from sensor */
>>  #define OV2740_REG_OTP_CUSTOMER		0x7010
>>  
>> +static const char * const ov2740_supply_name[] = {
>> +	"AVDD",
>> +	"DOVDD",
>> +	"DVDD",
>> +};
>> +
>> +#define OV2740_NUM_SUPPLIES ARRAY_SIZE(ov2740_supply_name)
>> +
>>  struct nvm_data {
>>  	struct nvmem_device *nvmem;
>>  	struct regmap *regmap;
>> @@ -523,10 +532,11 @@ struct ov2740 {
>>  	struct v4l2_ctrl *hblank;
>>  	struct v4l2_ctrl *exposure;
>>  
>> -	/* GPIOs, clocks */
>> +	/* GPIOs, clocks, regulators */
>>  	struct gpio_desc *reset_gpio;
>>  	struct gpio_desc *powerdown_gpio;
>>  	struct clk *clk;
>> +	struct regulator_bulk_data supplies[OV2740_NUM_SUPPLIES];
> 
> This would be cleaner with plain ARRAY_SIZE(). Same below. I.e. the macro
> is redundant.

Ok, I will change this for v2.



> 
>>  
>>  	/* Current mode */
>>  	const struct ov2740_mode *cur_mode;
>> @@ -1309,6 +1319,7 @@ static int ov2740_suspend(struct device *dev)
>>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 1);
>>  	gpiod_set_value_cansleep(ov2740->powerdown_gpio, 1);
>>  	clk_disable_unprepare(ov2740->clk);
>> +	regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>>  	return 0;
>>  }
>>  
>> @@ -1318,10 +1329,16 @@ static int ov2740_resume(struct device *dev)
>>  	struct ov2740 *ov2740 = to_ov2740(sd);
>>  	int ret;
>>  
>> -	ret = clk_prepare_enable(ov2740->clk);
>> +	ret = regulator_bulk_enable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>>  	if (ret)
>>  		return ret;
>>  
>> +	ret = clk_prepare_enable(ov2740->clk);
>> +	if (ret) {
>> +		regulator_bulk_disable(OV2740_NUM_SUPPLIES, ov2740->supplies);
>> +		return ret;
>> +	}
>> +
>>  	gpiod_set_value_cansleep(ov2740->powerdown_gpio, 0);
>>  	gpiod_set_value_cansleep(ov2740->reset_gpio, 0);
>>  	msleep(20);
>> @@ -1334,7 +1351,7 @@ static int ov2740_probe(struct i2c_client *client)
>>  	struct device *dev = &client->dev;
>>  	struct ov2740 *ov2740;
>>  	bool full_power;
>> -	int ret;
>> +	int i, ret;
> 
> unsigned int i

ack.

Regards,

Hans

Re: [PATCH 3/4] media: ov2740: Add powerdown GPIO support

[Ricardo Ribalda Delgado]

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
> >>
> >>
> >
> >
>


-- 
Ricardo Ribalda