[PATCH 0/2] media: Add regulator support to ov9282

[PATCH 0/2] media: Add regulator support to ov9282

[Dave Stevenson]

The sensor takes 3 voltage supply rails, so add the relevant
configuration to the device tree bindings and driver.

Dave Stevenson (2):
  dt-bindings: media: ovti,ov9282: Add optional regulators
  media: i2c: ov9282: Add support for regulators.

 .../bindings/media/i2c/ovti,ov9282.yaml       |  9 +++++
 drivers/media/i2c/ov9282.c                    | 38 +++++++++++++++++++
 2 files changed, 47 insertions(+)

-- 
2.34.1

[PATCH 1/2] dt-bindings: media: ovti,ov9282: Add optional regulators

[Dave Stevenson]

The OV9282 image sensor takes 3 voltage supplies, so
define the relevant regulators.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 .../devicetree/bindings/media/i2c/ovti,ov9282.yaml       | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
index bf115ab9d926..652b4cfeded7 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
@@ -36,6 +36,15 @@ properties:
     description: Reference to the GPIO connected to the XCLR pin, if any.
     maxItems: 1
 
+  avdd-supply:
+    description: Analog voltage supply, 2.8 volts
+
+  dvdd-supply:
+    description: Digital core voltage supply, 1.2 volts
+
+  dovdd-supply:
+    description: Digital I/O voltage supply, 1.8 volts
+
   port:
     additionalProperties: false
     $ref: /schemas/graph.yaml#/$defs/port-base
-- 
2.34.1

[PATCH 2/2] media: i2c: ov9282: Add support for regulators.

[Dave Stevenson]

The sensor takes 3 supply rails - AVDD, DVDD, and DOVDD.

Add hooks into the regulator framework for each of these
regulators.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 2e0b315801e5..699fc5b753b4 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -11,6 +11,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
@@ -55,6 +56,14 @@
 #define OV9282_REG_MIN		0x00
 #define OV9282_REG_MAX		0xfffff
 
+static const char * const ov9282_supply_names[] = {
+	"avdd",		/* Analog power */
+	"dovdd",	/* Digital I/O power */
+	"dvdd",		/* Digital core power */
+};
+
+#define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names)
+
 /**
  * struct ov9282_reg - ov9282 sensor register
  * @address: Register address
@@ -128,6 +137,7 @@ struct ov9282 {
 	struct media_pad pad;
 	struct gpio_desc *reset_gpio;
 	struct clk *inclk;
+	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *link_freq_ctrl;
 	struct v4l2_ctrl *pclk_ctrl;
@@ -767,6 +777,18 @@ static int ov9282_detect(struct ov9282 *ov9282)
 	return 0;
 }
 
+static int ov9282_configure_regulators(struct ov9282 *ov9282)
+{
+	unsigned int i;
+
+	for (i = 0; i < OV9282_NUM_SUPPLIES; i++)
+		ov9282->supplies[i].supply = ov9282_supply_names[i];
+
+	return devm_regulator_bulk_get(ov9282->dev,
+				       OV9282_NUM_SUPPLIES,
+				       ov9282->supplies);
+}
+
 /**
  * ov9282_parse_hw_config() - Parse HW configuration and check if supported
  * @ov9282: pointer to ov9282 device
@@ -803,6 +825,12 @@ static int ov9282_parse_hw_config(struct ov9282 *ov9282)
 		return PTR_ERR(ov9282->inclk);
 	}
 
+	ret = ov9282_configure_regulators(ov9282);
+	if (ret) {
+		dev_err(ov9282->dev, "Failed to get power regulators\n");
+		return ret;
+	}
+
 	rate = clk_get_rate(ov9282->inclk);
 	if (rate != OV9282_INCLK_RATE) {
 		dev_err(ov9282->dev, "inclk frequency mismatch");
@@ -874,6 +902,12 @@ static int ov9282_power_on(struct device *dev)
 	struct ov9282 *ov9282 = to_ov9282(sd);
 	int ret;
 
+	ret = regulator_bulk_enable(OV9282_NUM_SUPPLIES, ov9282->supplies);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable regulators\n");
+		return ret;
+	}
+
 	usleep_range(400, 600);
 
 	gpiod_set_value_cansleep(ov9282->reset_gpio, 1);
@@ -891,6 +925,8 @@ static int ov9282_power_on(struct device *dev)
 error_reset:
 	gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
 
+	regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
+
 	return ret;
 }
 
@@ -909,6 +945,8 @@ static int ov9282_power_off(struct device *dev)
 
 	clk_disable_unprepare(ov9282->inclk);
 
+	regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 1/2] dt-bindings: media: ovti,ov9282: Add optional regulators

[Rob Herring]

On Wed, 05 Oct 2022 16:20:17 +0100, Dave Stevenson wrote:
> The OV9282 image sensor takes 3 voltage supplies, so
> define the relevant regulators.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  .../devicetree/bindings/media/i2c/ovti,ov9282.yaml       | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 2/2] media: i2c: ov9282: Add support for regulators.

[Alexander Stein]

Hello Dave,

Am Mittwoch, 5. Oktober 2022, 17:20:18 CET schrieb Dave Stevenson:
> The sensor takes 3 supply rails - AVDD, DVDD, and DOVDD.
> 
> Add hooks into the regulator framework for each of these
> regulators.
> 
> Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> ---
>  drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 2e0b315801e5..699fc5b753b4 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -11,6 +11,7 @@
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> 
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fwnode.h>
> @@ -55,6 +56,14 @@
>  #define OV9282_REG_MIN		0x00
>  #define OV9282_REG_MAX		0xfffff
> 
> +static const char * const ov9282_supply_names[] = {
> +	"avdd",		/* Analog power */
> +	"dovdd",	/* Digital I/O power */
> +	"dvdd",		/* Digital core power */
> +};
> +
> +#define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names)
> +
>  /**
>   * struct ov9282_reg - ov9282 sensor register
>   * @address: Register address
> @@ -128,6 +137,7 @@ struct ov9282 {
>  	struct media_pad pad;
>  	struct gpio_desc *reset_gpio;
>  	struct clk *inclk;
> +	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];

Please add documentation for supplies.

>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_ctrl *link_freq_ctrl;
>  	struct v4l2_ctrl *pclk_ctrl;
> @@ -767,6 +777,18 @@ static int ov9282_detect(struct ov9282 *ov9282)
>  	return 0;
>  }
> 
> +static int ov9282_configure_regulators(struct ov9282 *ov9282)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < OV9282_NUM_SUPPLIES; i++)
> +		ov9282->supplies[i].supply = ov9282_supply_names[i];
> +
> +	return devm_regulator_bulk_get(ov9282->dev,
> +				       OV9282_NUM_SUPPLIES,
> +				       ov9282->supplies);
> +}
> +
>  /**
>   * ov9282_parse_hw_config() - Parse HW configuration and check if supported
> * @ov9282: pointer to ov9282 device
> @@ -803,6 +825,12 @@ static int ov9282_parse_hw_config(struct ov9282
> *ov9282) return PTR_ERR(ov9282->inclk);
>  	}
> 
> +	ret = ov9282_configure_regulators(ov9282);
> +	if (ret) {
> +		dev_err(ov9282->dev, "Failed to get power regulators\n");

dev_err_probe seems sensible here.

> +		return ret;
> +	}
> +
>  	rate = clk_get_rate(ov9282->inclk);
>  	if (rate != OV9282_INCLK_RATE) {
>  		dev_err(ov9282->dev, "inclk frequency mismatch");
> @@ -874,6 +902,12 @@ static int ov9282_power_on(struct device *dev)
>  	struct ov9282 *ov9282 = to_ov9282(sd);
>  	int ret;
> 
> +	ret = regulator_bulk_enable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable regulators\n");
> +		return ret;
> +	}
> +
>  	usleep_range(400, 600);
> 
>  	gpiod_set_value_cansleep(ov9282->reset_gpio, 1);
> @@ -891,6 +925,8 @@ static int ov9282_power_on(struct device *dev)
>  error_reset:
>  	gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
> 
> +	regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> +
>  	return ret;
>  }
> 
> @@ -909,6 +945,8 @@ static int ov9282_power_off(struct device *dev)
> 
>  	clk_disable_unprepare(ov9282->inclk);
> 
> +	regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> +
>  	return 0;
>  }

Despite the nits above
Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Re: [PATCH 2/2] media: i2c: ov9282: Add support for regulators.

[Dave Stevenson]

Hi Alexander

Thanks for the review.

Sakari has already picked this up and included it in a pull to Mauro for 6.2.
https://www.spinics.net/lists/linux-media/msg222346.html

On Thu, 24 Nov 2022 at 09:31, Alexander Stein
<alexander.stein@ew.tq-group.com> wrote:
>
> Hello Dave,
>
> Am Mittwoch, 5. Oktober 2022, 17:20:18 CET schrieb Dave Stevenson:
> > The sensor takes 3 supply rails - AVDD, DVDD, and DOVDD.
> >
> > Add hooks into the regulator framework for each of these
> > regulators.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 2e0b315801e5..699fc5b753b4 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-fwnode.h>
> > @@ -55,6 +56,14 @@
> >  #define OV9282_REG_MIN               0x00
> >  #define OV9282_REG_MAX               0xfffff
> >
> > +static const char * const ov9282_supply_names[] = {
> > +     "avdd",         /* Analog power */
> > +     "dovdd",        /* Digital I/O power */
> > +     "dvdd",         /* Digital core power */
> > +};
> > +
> > +#define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names)
> > +
> >  /**
> >   * struct ov9282_reg - ov9282 sensor register
> >   * @address: Register address
> > @@ -128,6 +137,7 @@ struct ov9282 {
> >       struct media_pad pad;
> >       struct gpio_desc *reset_gpio;
> >       struct clk *inclk;
> > +     struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
>
> Please add documentation for supplies.

Is it the place for the driver to document the supplies beyond the
comments in ov9282_supply_names with regard to which sensor rail they
relate to?
Some drivers include the typical values for each supply, but those are
technically inaccurate as each will have a min and max value.

Anyone interfacing with a sensor is going to have the datasheet for it
and should be referring to that for the characteristics of supply
rails. Duplicating some of that in the driver seems redundant, and has
the potential to be incorrect.

> >       struct v4l2_ctrl_handler ctrl_handler;
> >       struct v4l2_ctrl *link_freq_ctrl;
> >       struct v4l2_ctrl *pclk_ctrl;
> > @@ -767,6 +777,18 @@ static int ov9282_detect(struct ov9282 *ov9282)
> >       return 0;
> >  }
> >
> > +static int ov9282_configure_regulators(struct ov9282 *ov9282)
> > +{
> > +     unsigned int i;
> > +
> > +     for (i = 0; i < OV9282_NUM_SUPPLIES; i++)
> > +             ov9282->supplies[i].supply = ov9282_supply_names[i];
> > +
> > +     return devm_regulator_bulk_get(ov9282->dev,
> > +                                    OV9282_NUM_SUPPLIES,
> > +                                    ov9282->supplies);
> > +}
> > +
> >  /**
> >   * ov9282_parse_hw_config() - Parse HW configuration and check if supported
> > * @ov9282: pointer to ov9282 device
> > @@ -803,6 +825,12 @@ static int ov9282_parse_hw_config(struct ov9282
> > *ov9282) return PTR_ERR(ov9282->inclk);
> >       }
> >
> > +     ret = ov9282_configure_regulators(ov9282);
> > +     if (ret) {
> > +             dev_err(ov9282->dev, "Failed to get power regulators\n");
>
> dev_err_probe seems sensible here.

That would have been good - sorry. I must get into the habit of
remembering to use dev_err_probe.

  Dave

> > +             return ret;
> > +     }
> > +
> >       rate = clk_get_rate(ov9282->inclk);
> >       if (rate != OV9282_INCLK_RATE) {
> >               dev_err(ov9282->dev, "inclk frequency mismatch");
> > @@ -874,6 +902,12 @@ static int ov9282_power_on(struct device *dev)
> >       struct ov9282 *ov9282 = to_ov9282(sd);
> >       int ret;
> >
> > +     ret = regulator_bulk_enable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> > +     if (ret < 0) {
> > +             dev_err(dev, "Failed to enable regulators\n");
> > +             return ret;
> > +     }
> > +
> >       usleep_range(400, 600);
> >
> >       gpiod_set_value_cansleep(ov9282->reset_gpio, 1);
> > @@ -891,6 +925,8 @@ static int ov9282_power_on(struct device *dev)
> >  error_reset:
> >       gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
> >
> > +     regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> > +
> >       return ret;
> >  }
> >
> > @@ -909,6 +945,8 @@ static int ov9282_power_off(struct device *dev)
> >
> >       clk_disable_unprepare(ov9282->inclk);
> >
> > +     regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> > +
> >       return 0;
> >  }
>
> Despite the nits above
> Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Re: [PATCH 2/2] media: i2c: ov9282: Add support for regulators.

[Alexander Stein]

Hi Dave,

Am Donnerstag, 24. November 2022, 12:58:08 CET schrieb Dave Stevenson:
> Hi Alexander
> 
> Thanks for the review.
> 
> Sakari has already picked this up and included it in a pull to Mauro for
> 6.2. https://www.spinics.net/lists/linux-media/msg222346.html

A quite recent, I wasn't aware of that. Thanks for the hint.

> On Thu, 24 Nov 2022 at 09:31, Alexander Stein
> 
> <alexander.stein@ew.tq-group.com> wrote:
> > Hello Dave,
> > 
> > Am Mittwoch, 5. Oktober 2022, 17:20:18 CET schrieb Dave Stevenson:
> > > The sensor takes 3 supply rails - AVDD, DVDD, and DOVDD.
> > > 
> > > Add hooks into the regulator framework for each of these
> > > regulators.
> > > 
> > > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > ---
> > > 
> > >  drivers/media/i2c/ov9282.c | 38 ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 38 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index 2e0b315801e5..699fc5b753b4 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -11,6 +11,7 @@
> > > 
> > >  #include <linux/i2c.h>
> > >  #include <linux/module.h>
> > >  #include <linux/pm_runtime.h>
> > > 
> > > +#include <linux/regulator/consumer.h>
> > > 
> > >  #include <media/v4l2-ctrls.h>
> > >  #include <media/v4l2-fwnode.h>
> > > 
> > > @@ -55,6 +56,14 @@
> > > 
> > >  #define OV9282_REG_MIN               0x00
> > >  #define OV9282_REG_MAX               0xfffff
> > > 
> > > +static const char * const ov9282_supply_names[] = {
> > > +     "avdd",         /* Analog power */
> > > +     "dovdd",        /* Digital I/O power */
> > > +     "dvdd",         /* Digital core power */
> > > +};
> > > +
> > > +#define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names)
> > > +
> > > 
> > >  /**
> > >  
> > >   * struct ov9282_reg - ov9282 sensor register
> > >   * @address: Register address
> > > 
> > > @@ -128,6 +137,7 @@ struct ov9282 {
> > > 
> > >       struct media_pad pad;
> > >       struct gpio_desc *reset_gpio;
> > >       struct clk *inclk;
> > > 
> > > +     struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
> > 
> > Please add documentation for supplies.
> 
> Is it the place for the driver to document the supplies beyond the
> comments in ov9282_supply_names with regard to which sensor rail they
> relate to?
> Some drivers include the typical values for each supply, but those are
> technically inaccurate as each will have a min and max value.
> 
> Anyone interfacing with a sensor is going to have the datasheet for it
> and should be referring to that for the characteristics of supply
> rails. Duplicating some of that in the driver seems redundant, and has
> the potential to be incorrect.

What I meant was adding " @supplies: power supply regulators" to the doxygen 
(?) documentation directly above.
I agree that no details about those supplies should be added to driver code 
though.

Alexander

> 
> > >       struct v4l2_ctrl_handler ctrl_handler;
> > >       struct v4l2_ctrl *link_freq_ctrl;
> > >       struct v4l2_ctrl *pclk_ctrl;
> > > 
> > > @@ -767,6 +777,18 @@ static int ov9282_detect(struct ov9282 *ov9282)
> > > 
> > >       return 0;
> > >  
> > >  }
> > > 
> > > +static int ov9282_configure_regulators(struct ov9282 *ov9282)
> > > +{
> > > +     unsigned int i;
> > > +
> > > +     for (i = 0; i < OV9282_NUM_SUPPLIES; i++)
> > > +             ov9282->supplies[i].supply = ov9282_supply_names[i];
> > > +
> > > +     return devm_regulator_bulk_get(ov9282->dev,
> > > +                                    OV9282_NUM_SUPPLIES,
> > > +                                    ov9282->supplies);
> > > +}
> > > +
> > > 
> > >  /**
> > >  
> > >   * ov9282_parse_hw_config() - Parse HW configuration and check if
> > >   supported
> > > 
> > > * @ov9282: pointer to ov9282 device
> > > @@ -803,6 +825,12 @@ static int ov9282_parse_hw_config(struct ov9282
> > > *ov9282) return PTR_ERR(ov9282->inclk);
> > > 
> > >       }
> > > 
> > > +     ret = ov9282_configure_regulators(ov9282);
> > > +     if (ret) {
> > > +             dev_err(ov9282->dev, "Failed to get power regulators\n");
> > 
> > dev_err_probe seems sensible here.
> 
> That would have been good - sorry. I must get into the habit of
> remembering to use dev_err_probe.
> 
>   Dave
> 
> > > +             return ret;
> > > +     }
> > > +
> > > 
> > >       rate = clk_get_rate(ov9282->inclk);
> > >       if (rate != OV9282_INCLK_RATE) {
> > >       
> > >               dev_err(ov9282->dev, "inclk frequency mismatch");
> > > 
> > > @@ -874,6 +902,12 @@ static int ov9282_power_on(struct device *dev)
> > > 
> > >       struct ov9282 *ov9282 = to_ov9282(sd);
> > >       int ret;
> > > 
> > > +     ret = regulator_bulk_enable(OV9282_NUM_SUPPLIES,
> > > ov9282->supplies);
> > > +     if (ret < 0) {
> > > +             dev_err(dev, "Failed to enable regulators\n");
> > > +             return ret;
> > > +     }
> > > +
> > > 
> > >       usleep_range(400, 600);
> > >       
> > >       gpiod_set_value_cansleep(ov9282->reset_gpio, 1);
> > > 
> > > @@ -891,6 +925,8 @@ static int ov9282_power_on(struct device *dev)
> > > 
> > >  error_reset:
> > >       gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
> > > 
> > > +     regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> > > +
> > > 
> > >       return ret;
> > >  
> > >  }
> > > 
> > > @@ -909,6 +945,8 @@ static int ov9282_power_off(struct device *dev)
> > > 
> > >       clk_disable_unprepare(ov9282->inclk);
> > > 
> > > +     regulator_bulk_disable(OV9282_NUM_SUPPLIES, ov9282->supplies);
> > > +
> > > 
> > >       return 0;
> > >  
> > >  }
> > 
> > Despite the nits above
> > Acked-by: Alexander Stein <alexander.stein@ew.tq-group.com>