All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/4] media: i2c: imx412: Add regulator control to imx412
@ 2022-04-15 11:59 Bryan O'Donoghue
  2022-04-15 11:59 ` [PATCH v5 1/4] media: i2c: imx412: Fix reset GPIO polarity Bryan O'Donoghue
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Bryan O'Donoghue @ 2022-04-15 11:59 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media, devicetree
  Cc: robert.foss, hfink, jgrahsl, dmitry.baryshkov,
	vladimir.zapolskiy, bryan.odonoghue

V5:
- Adds in a fix for the GPIO reset. I was a bit worried that this change
  would negatively affect the Intel people. Looking around on Google I
  found that the imx412 is/was used on a system called Keem Bay which is a
  DTS based system so TL;DR its both safe and right to make this change.
- Front loads the clock/gpio ordering fix.
- Adds "Fixes" and Cc: stable for the two associated fixes
- Changes num_supplies to ARRAY_SIZE(supplies) - Jacopo

V4:
- Further asks on placing arguments on the same line - Saraki
- Moves regulator_bulk_disable() to common error path - Saraki
- Fixes power_off ordering so that power_off and power_on have the same
  flow - Bryan

V3:
- int num_supplies -> unsigned int num_supplied - Saraki
- Move imx412->num_supplies to same line as devm_regulator_bulk_get - Saraki
- Return ret on regulator_bulk_enable() - Saraki
- Remember to regulator_bulk_disable() on !clk_prepare_enable() - Saraki

V2:
- Drops redundant verbage "Definition of the" from each of the newly defined
  regulators - Krzysztof Kozlowski
- Adds Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
- "description: thing described" to the same line in new additions
  following the established way of doing it in this file - bod

V1:
The imx412 has three regulators which depending on platform may need to be
individually enabled.

- dovdd
- avdd
- dvdd

Existing code for an Intel platform doesn't appear to need to enable these
rails directly. On my reference hardware, a Qualcomm Thundercomm RB5 some
of these rails do need to be enabled individually.

Add in the above named rails as optional in the YAML in the first patch
along with code to switch them on in the second patch.

Existing imx412 users should get dummy regulators populated which the
regulator_bulk_enable()/regulator_bulk_disable() code can safely ignore.

Bryan O'Donoghue (4):
  media: i2c: imx412: Fix reset GPIO polarity
  media: i2c: imx412: Fix power_off ordering
  media: dt-bindings: imx412: Add regulator descriptions
  media: i2c: imx412: Add bulk regulator support

 .../bindings/media/i2c/sony,imx412.yaml       |  9 +++++
 drivers/media/i2c/imx412.c                    | 38 +++++++++++++++++--
 2 files changed, 43 insertions(+), 4 deletions(-)

-- 
2.35.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v5 1/4] media: i2c: imx412: Fix reset GPIO polarity
  2022-04-15 11:59 [PATCH v5 0/4] media: i2c: imx412: Add regulator control to imx412 Bryan O'Donoghue
@ 2022-04-15 11:59 ` Bryan O'Donoghue
  2022-04-25 14:41   ` Alessandrelli, Daniele
  2022-04-15 11:59 ` [PATCH v5 2/4] media: i2c: imx412: Fix power_off ordering Bryan O'Donoghue
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Bryan O'Donoghue @ 2022-04-15 11:59 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media, devicetree
  Cc: robert.foss, hfink, jgrahsl, dmitry.baryshkov,
	vladimir.zapolskiy, bryan.odonoghue, stable

The imx412/imx577 sensor has a reset line that is active low not active
high. Currently the logic for this is inverted.

The right way to define the reset line is to declare it active low in the
DTS and invert the logic currently contained in the driver.

The DTS should represent the hardware does i.e. reset is active low.
So:
+               reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
not:
-               reset-gpios = <&tlmm 78 GPIO_ACTIVE_HIGH>;

I was a bit reticent about changing this logic since I thought it might
negatively impact @intel.com users. Googling a bit though I believe this
sensor is used on "Keem Bay" which is clearly a DTS based system and is not
upstream yet.

Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/i2c/imx412.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index be3f6ea55559..e6be6b4250f5 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1011,7 +1011,7 @@ static int imx412_power_on(struct device *dev)
 	struct imx412 *imx412 = to_imx412(sd);
 	int ret;
 
-	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
+	gpiod_set_value_cansleep(imx412->reset_gpio, 0);
 
 	ret = clk_prepare_enable(imx412->inclk);
 	if (ret) {
@@ -1024,7 +1024,7 @@ static int imx412_power_on(struct device *dev)
 	return 0;
 
 error_reset:
-	gpiod_set_value_cansleep(imx412->reset_gpio, 0);
+	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
 
 	return ret;
 }
@@ -1040,7 +1040,7 @@ static int imx412_power_off(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct imx412 *imx412 = to_imx412(sd);
 
-	gpiod_set_value_cansleep(imx412->reset_gpio, 0);
+	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
 
 	clk_disable_unprepare(imx412->inclk);
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 2/4] media: i2c: imx412: Fix power_off ordering
  2022-04-15 11:59 [PATCH v5 0/4] media: i2c: imx412: Add regulator control to imx412 Bryan O'Donoghue
  2022-04-15 11:59 ` [PATCH v5 1/4] media: i2c: imx412: Fix reset GPIO polarity Bryan O'Donoghue
@ 2022-04-15 11:59 ` Bryan O'Donoghue
  2022-04-25 14:43   ` Alessandrelli, Daniele
  2022-04-15 11:59 ` [PATCH v5 3/4] media: dt-bindings: imx412: Add regulator descriptions Bryan O'Donoghue
  2022-04-15 11:59 ` [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support Bryan O'Donoghue
  3 siblings, 1 reply; 10+ messages in thread
From: Bryan O'Donoghue @ 2022-04-15 11:59 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media, devicetree
  Cc: robert.foss, hfink, jgrahsl, dmitry.baryshkov,
	vladimir.zapolskiy, bryan.odonoghue, stable

The enable path does
- gpio
- clock

The disable path does
- gpio
- clock

Fix the order on the power-off path so that power-off and power-on have the
same ordering for clock and gpio.

Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver")
Cc: stable@vger.kernel.org
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/i2c/imx412.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index e6be6b4250f5..84279a680873 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1040,10 +1040,10 @@ static int imx412_power_off(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct imx412 *imx412 = to_imx412(sd);
 
-	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
-
 	clk_disable_unprepare(imx412->inclk);
 
+	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
+
 	return 0;
 }
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 3/4] media: dt-bindings: imx412: Add regulator descriptions
  2022-04-15 11:59 [PATCH v5 0/4] media: i2c: imx412: Add regulator control to imx412 Bryan O'Donoghue
  2022-04-15 11:59 ` [PATCH v5 1/4] media: i2c: imx412: Fix reset GPIO polarity Bryan O'Donoghue
  2022-04-15 11:59 ` [PATCH v5 2/4] media: i2c: imx412: Fix power_off ordering Bryan O'Donoghue
@ 2022-04-15 11:59 ` Bryan O'Donoghue
  2022-04-15 11:59 ` [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support Bryan O'Donoghue
  3 siblings, 0 replies; 10+ messages in thread
From: Bryan O'Donoghue @ 2022-04-15 11:59 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media, devicetree
  Cc: robert.foss, hfink, jgrahsl, dmitry.baryshkov,
	vladimir.zapolskiy, bryan.odonoghue, Rob Herring,
	Krzysztof Kozlowski, Krzysztof Kozlowski

The imx412 like many I2C camera sensors has three voltage rails which
depending on platform may be necessary to switch power onto directly.

Add in as optional rails so as not to break anything for existing users.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 .../devicetree/bindings/media/i2c/sony,imx412.yaml       | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
index afcf70947f7e..26d1807d0bb6 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml
@@ -32,6 +32,15 @@ properties:
     description: Clock frequency 6MHz, 12MHz, 18MHz, 24MHz or 27MHz
     maxItems: 1
 
+  dovdd-supply:
+    description: Interface power supply.
+
+  avdd-supply:
+    description: Analog power supply.
+
+  dvdd-supply:
+    description: Digital power supply.
+
   reset-gpios:
     description: Reference to the GPIO connected to the XCLR pin, if any.
     maxItems: 1
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support
  2022-04-15 11:59 [PATCH v5 0/4] media: i2c: imx412: Add regulator control to imx412 Bryan O'Donoghue
                   ` (2 preceding siblings ...)
  2022-04-15 11:59 ` [PATCH v5 3/4] media: dt-bindings: imx412: Add regulator descriptions Bryan O'Donoghue
@ 2022-04-15 11:59 ` Bryan O'Donoghue
  2022-04-15 15:12   ` Jacopo Mondi
                     ` (2 more replies)
  3 siblings, 3 replies; 10+ messages in thread
From: Bryan O'Donoghue @ 2022-04-15 11:59 UTC (permalink / raw)
  To: sakari.ailus, jacopo, paul.j.murphy, daniele.alessandrelli,
	mchehab, linux-media, devicetree
  Cc: robert.foss, hfink, jgrahsl, dmitry.baryshkov,
	vladimir.zapolskiy, bryan.odonoghue

Depending on the platform we may need to enable and disable three separate
regulators for the imx412.

- DOVDD
Digital I/O power

- AVDD
Analog power

- DVDD
Digital core power

The addition of these regulators shouldn't affect existing users using
fixed-on/firmware-controlled regulators.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/media/i2c/imx412.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index 84279a680873..1795a6180d60 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.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>
@@ -101,6 +102,12 @@ struct imx412_mode {
 	struct imx412_reg_list reg_list;
 };
 
+static const char * const imx412_supply_names[] = {
+	"dovdd",	/* Digital I/O power */
+	"avdd",		/* Analog power */
+	"dvdd",		/* Digital core power */
+};
+
 /**
  * struct imx412 - imx412 sensor device structure
  * @dev: Pointer to generic device
@@ -128,6 +135,7 @@ struct imx412 {
 	struct media_pad pad;
 	struct gpio_desc *reset_gpio;
 	struct clk *inclk;
+	struct regulator_bulk_data supplies[ARRAY_SIZE(imx412_supply_names)];
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *link_freq_ctrl;
 	struct v4l2_ctrl *pclk_ctrl;
@@ -946,6 +954,16 @@ static int imx412_parse_hw_config(struct imx412 *imx412)
 		return -EINVAL;
 	}
 
+	/* Get optional DT defined regulators */
+	for (i = 0; i < ARRAY_SIZE(imx412_supply_names); i++)
+		imx412->supplies[i].supply = imx412_supply_names[i];
+
+	ret = devm_regulator_bulk_get(imx412->dev,
+				      ARRAY_SIZE(imx412_supply_names),
+				      imx412->supplies);
+	if (ret)
+		return ret;
+
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep)
 		return -ENXIO;
@@ -1011,6 +1029,13 @@ static int imx412_power_on(struct device *dev)
 	struct imx412 *imx412 = to_imx412(sd);
 	int ret;
 
+	ret = regulator_bulk_enable(ARRAY_SIZE(imx412_supply_names),
+				    imx412->supplies);
+	if (ret < 0) {
+		dev_err(dev, "failed to enable regulators\n");
+		return ret;
+	}
+
 	gpiod_set_value_cansleep(imx412->reset_gpio, 0);
 
 	ret = clk_prepare_enable(imx412->inclk);
@@ -1025,6 +1050,8 @@ static int imx412_power_on(struct device *dev)
 
 error_reset:
 	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
+	regulator_bulk_disable(ARRAY_SIZE(imx412_supply_names),
+			       imx412->supplies);
 
 	return ret;
 }
@@ -1044,6 +1071,9 @@ static int imx412_power_off(struct device *dev)
 
 	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
 
+	regulator_bulk_disable(ARRAY_SIZE(imx412_supply_names),
+			       imx412->supplies);
+
 	return 0;
 }
 
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support
  2022-04-15 11:59 ` [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support Bryan O'Donoghue
@ 2022-04-15 15:12   ` Jacopo Mondi
  2022-04-26 16:52   ` Alessandrelli, Daniele
  2022-05-06 14:26   ` Sakari Ailus
  2 siblings, 0 replies; 10+ messages in thread
From: Jacopo Mondi @ 2022-04-15 15:12 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: sakari.ailus, paul.j.murphy, daniele.alessandrelli, mchehab,
	linux-media, devicetree, robert.foss, hfink, jgrahsl,
	dmitry.baryshkov, vladimir.zapolskiy

Hello Bryan

On Fri, Apr 15, 2022 at 12:59:54PM +0100, Bryan O'Donoghue wrote:
> Depending on the platform we may need to enable and disable three separate
> regulators for the imx412.
>
> - DOVDD
> Digital I/O power
>
> - AVDD
> Analog power
>
> - DVDD
> Digital core power
>
> The addition of these regulators shouldn't affect existing users using
> fixed-on/firmware-controlled regulators.
>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Thanks
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Cheers
   j

> ---
>  drivers/media/i2c/imx412.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index 84279a680873..1795a6180d60 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.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>
> @@ -101,6 +102,12 @@ struct imx412_mode {
>  	struct imx412_reg_list reg_list;
>  };
>
> +static const char * const imx412_supply_names[] = {
> +	"dovdd",	/* Digital I/O power */
> +	"avdd",		/* Analog power */
> +	"dvdd",		/* Digital core power */
> +};
> +
>  /**
>   * struct imx412 - imx412 sensor device structure
>   * @dev: Pointer to generic device
> @@ -128,6 +135,7 @@ struct imx412 {
>  	struct media_pad pad;
>  	struct gpio_desc *reset_gpio;
>  	struct clk *inclk;
> +	struct regulator_bulk_data supplies[ARRAY_SIZE(imx412_supply_names)];
>  	struct v4l2_ctrl_handler ctrl_handler;
>  	struct v4l2_ctrl *link_freq_ctrl;
>  	struct v4l2_ctrl *pclk_ctrl;
> @@ -946,6 +954,16 @@ static int imx412_parse_hw_config(struct imx412 *imx412)
>  		return -EINVAL;
>  	}
>
> +	/* Get optional DT defined regulators */
> +	for (i = 0; i < ARRAY_SIZE(imx412_supply_names); i++)
> +		imx412->supplies[i].supply = imx412_supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(imx412->dev,
> +				      ARRAY_SIZE(imx412_supply_names),
> +				      imx412->supplies);
> +	if (ret)
> +		return ret;
> +
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!ep)
>  		return -ENXIO;
> @@ -1011,6 +1029,13 @@ static int imx412_power_on(struct device *dev)
>  	struct imx412 *imx412 = to_imx412(sd);
>  	int ret;
>
> +	ret = regulator_bulk_enable(ARRAY_SIZE(imx412_supply_names),
> +				    imx412->supplies);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to enable regulators\n");
> +		return ret;
> +	}
> +
>  	gpiod_set_value_cansleep(imx412->reset_gpio, 0);
>
>  	ret = clk_prepare_enable(imx412->inclk);
> @@ -1025,6 +1050,8 @@ static int imx412_power_on(struct device *dev)
>
>  error_reset:
>  	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
> +	regulator_bulk_disable(ARRAY_SIZE(imx412_supply_names),
> +			       imx412->supplies);
>
>  	return ret;
>  }
> @@ -1044,6 +1071,9 @@ static int imx412_power_off(struct device *dev)
>
>  	gpiod_set_value_cansleep(imx412->reset_gpio, 1);
>
> +	regulator_bulk_disable(ARRAY_SIZE(imx412_supply_names),
> +			       imx412->supplies);
> +
>  	return 0;
>  }
>
> --
> 2.35.1
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 1/4] media: i2c: imx412: Fix reset GPIO polarity
  2022-04-15 11:59 ` [PATCH v5 1/4] media: i2c: imx412: Fix reset GPIO polarity Bryan O'Donoghue
@ 2022-04-25 14:41   ` Alessandrelli, Daniele
  0 siblings, 0 replies; 10+ messages in thread
From: Alessandrelli, Daniele @ 2022-04-25 14:41 UTC (permalink / raw)
  To: Murphy, Paul J, jacopo, mchehab, bryan.odonoghue, linux-media,
	devicetree, sakari.ailus
  Cc: hfink, robert.foss, vladimir.zapolskiy, dmitry.baryshkov, stable,
	jgrahsl

Hi Bryan,

On Fri, 2022-04-15 at 12:59 +0100, Bryan O'Donoghue wrote:
> The imx412/imx577 sensor has a reset line that is active low not active
> high. Currently the logic for this is inverted.
> 
> The right way to define the reset line is to declare it active low in the
> DTS and invert the logic currently contained in the driver.
> 
> The DTS should represent the hardware does i.e. reset is active low.
> So:
> +               reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
> not:
> -               reset-gpios = <&tlmm 78 GPIO_ACTIVE_HIGH>;
> 
> I was a bit reticent about changing this logic since I thought it might
> negatively impact @intel.com users. Googling a bit though I believe this
> sensor is used on "Keem Bay" which is clearly a DTS based system and is not
> upstream yet.

Thanks for the fix and sorry for the late reply (I've been off for the
last 2 weeks).

The fix looks good to me and it will not negatively affect us (I
appreciate the consideration!).

Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>

> 
> Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/i2c/imx412.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index be3f6ea55559..e6be6b4250f5 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1011,7 +1011,7 @@ static int imx412_power_on(struct device *dev)
>         struct imx412 *imx412 = to_imx412(sd);
>         int ret;
>  
> -       gpiod_set_value_cansleep(imx412->reset_gpio, 1);
> +       gpiod_set_value_cansleep(imx412->reset_gpio, 0);
>  
>         ret = clk_prepare_enable(imx412->inclk);
>         if (ret) {
> @@ -1024,7 +1024,7 @@ static int imx412_power_on(struct device *dev)
>         return 0;
>  
>  error_reset:
> -       gpiod_set_value_cansleep(imx412->reset_gpio, 0);
> +       gpiod_set_value_cansleep(imx412->reset_gpio, 1);
>  
>         return ret;
>  }
> @@ -1040,7 +1040,7 @@ static int imx412_power_off(struct device *dev)
>         struct v4l2_subdev *sd = dev_get_drvdata(dev);
>         struct imx412 *imx412 = to_imx412(sd);
>  
> -       gpiod_set_value_cansleep(imx412->reset_gpio, 0);
> +       gpiod_set_value_cansleep(imx412->reset_gpio, 1);
>  
>         clk_disable_unprepare(imx412->inclk);
>  


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 2/4] media: i2c: imx412: Fix power_off ordering
  2022-04-15 11:59 ` [PATCH v5 2/4] media: i2c: imx412: Fix power_off ordering Bryan O'Donoghue
@ 2022-04-25 14:43   ` Alessandrelli, Daniele
  0 siblings, 0 replies; 10+ messages in thread
From: Alessandrelli, Daniele @ 2022-04-25 14:43 UTC (permalink / raw)
  To: Murphy, Paul J, jacopo, mchehab, bryan.odonoghue, linux-media,
	devicetree, sakari.ailus
  Cc: hfink, robert.foss, vladimir.zapolskiy, dmitry.baryshkov, stable,
	jgrahsl

On Fri, 2022-04-15 at 12:59 +0100, Bryan O'Donoghue wrote:
> The enable path does
> - gpio
> - clock
> 
> The disable path does
> - gpio
> - clock
> 
> Fix the order on the power-off path so that power-off and power-on have
> the
> same ordering for clock and gpio.
> 
> Fixes: 9214e86c0cc1 ("media: i2c: Add imx412 camera sensor driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/media/i2c/imx412.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index e6be6b4250f5..84279a680873 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.c
> @@ -1040,10 +1040,10 @@ static int imx412_power_off(struct device *dev)
>         struct v4l2_subdev *sd = dev_get_drvdata(dev);
>         struct imx412 *imx412 = to_imx412(sd);
>  
> -       gpiod_set_value_cansleep(imx412->reset_gpio, 1);
> -
>         clk_disable_unprepare(imx412->inclk);
>  
> +       gpiod_set_value_cansleep(imx412->reset_gpio, 1);
> +
>         return 0;
>  }
>  

LGTM, thanks for the patch!

Reviewed-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support
  2022-04-15 11:59 ` [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support Bryan O'Donoghue
  2022-04-15 15:12   ` Jacopo Mondi
@ 2022-04-26 16:52   ` Alessandrelli, Daniele
  2022-05-06 14:26   ` Sakari Ailus
  2 siblings, 0 replies; 10+ messages in thread
From: Alessandrelli, Daniele @ 2022-04-26 16:52 UTC (permalink / raw)
  To: Murphy, Paul J, jacopo, mchehab, bryan.odonoghue, linux-media,
	devicetree, sakari.ailus
  Cc: hfink, robert.foss, vladimir.zapolskiy, dmitry.baryshkov, jgrahsl

On Fri, 2022-04-15 at 12:59 +0100, Bryan O'Donoghue wrote:
> Depending on the platform we may need to enable and disable three separate
> regulators for the imx412.
> 
> - DOVDD
> Digital I/O power
> 
> - AVDD
> Analog power
> 
> - DVDD
> Digital core power
> 
> The addition of these regulators shouldn't affect existing users using
> fixed-on/firmware-controlled regulators.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>

> ---
>  drivers/media/i2c/imx412.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
> index 84279a680873..1795a6180d60 100644
> --- a/drivers/media/i2c/imx412.c
> +++ b/drivers/media/i2c/imx412.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>
> @@ -101,6 +102,12 @@ struct imx412_mode {
>         struct imx412_reg_list reg_list;
>  };
>  
> +static const char * const imx412_supply_names[] = {
> +       "dovdd",        /* Digital I/O power */
> +       "avdd",         /* Analog power */
> +       "dvdd",         /* Digital core power */
> +};
> +
>  /**
>   * struct imx412 - imx412 sensor device structure
>   * @dev: Pointer to generic device
> @@ -128,6 +135,7 @@ struct imx412 {
>         struct media_pad pad;
>         struct gpio_desc *reset_gpio;
>         struct clk *inclk;
> +       struct regulator_bulk_data supplies[ARRAY_SIZE(imx412_supply_names)];
>         struct v4l2_ctrl_handler ctrl_handler;
>         struct v4l2_ctrl *link_freq_ctrl;
>         struct v4l2_ctrl *pclk_ctrl;
> @@ -946,6 +954,16 @@ static int imx412_parse_hw_config(struct imx412 *imx412)
>                 return -EINVAL;
>         }
>  
> +       /* Get optional DT defined regulators */
> +       for (i = 0; i < ARRAY_SIZE(imx412_supply_names); i++)
> +               imx412->supplies[i].supply = imx412_supply_names[i];
> +
> +       ret = devm_regulator_bulk_get(imx412->dev,
> +                                     ARRAY_SIZE(imx412_supply_names),
> +                                     imx412->supplies);
> +       if (ret)
> +               return ret;
> +
>         ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>         if (!ep)
>                 return -ENXIO;
> @@ -1011,6 +1029,13 @@ static int imx412_power_on(struct device *dev)
>         struct imx412 *imx412 = to_imx412(sd);
>         int ret;
>  
> +       ret = regulator_bulk_enable(ARRAY_SIZE(imx412_supply_names),
> +                                   imx412->supplies);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to enable regulators\n");
> +               return ret;
> +       }
> +
>         gpiod_set_value_cansleep(imx412->reset_gpio, 0);
>  
>         ret = clk_prepare_enable(imx412->inclk);
> @@ -1025,6 +1050,8 @@ static int imx412_power_on(struct device *dev)
>  
>  error_reset:
>         gpiod_set_value_cansleep(imx412->reset_gpio, 1);
> +       regulator_bulk_disable(ARRAY_SIZE(imx412_supply_names),
> +                              imx412->supplies);
>  
>         return ret;
>  }
> @@ -1044,6 +1071,9 @@ static int imx412_power_off(struct device *dev)
>  
>         gpiod_set_value_cansleep(imx412->reset_gpio, 1);
>  
> +       regulator_bulk_disable(ARRAY_SIZE(imx412_supply_names),
> +                              imx412->supplies);
> +
>         return 0;
>  }
>  


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support
  2022-04-15 11:59 ` [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support Bryan O'Donoghue
  2022-04-15 15:12   ` Jacopo Mondi
  2022-04-26 16:52   ` Alessandrelli, Daniele
@ 2022-05-06 14:26   ` Sakari Ailus
  2 siblings, 0 replies; 10+ messages in thread
From: Sakari Ailus @ 2022-05-06 14:26 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: jacopo, paul.j.murphy, daniele.alessandrelli, mchehab,
	linux-media, devicetree, robert.foss, hfink, jgrahsl,
	dmitry.baryshkov, vladimir.zapolskiy

On Fri, Apr 15, 2022 at 12:59:54PM +0100, Bryan O'Donoghue wrote:
> Depending on the platform we may need to enable and disable three separate
> regulators for the imx412.
> 
> - DOVDD
> Digital I/O power
> 
> - AVDD
> Analog power
> 
> - DVDD
> Digital core power
> 
> The addition of these regulators shouldn't affect existing users using
> fixed-on/firmware-controlled regulators.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Squashed this bit into the patch:

diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index 1795a6180d60d..a1394d6c14320 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -116,6 +116,7 @@ static const char * const imx412_supply_names[] = {
  * @pad: Media pad. Only one pad supported
  * @reset_gpio: Sensor reset gpio
  * @inclk: Sensor input clock
+ * @supplies: Regulator supplies
  * @ctrl_handler: V4L2 control handler
  * @link_freq_ctrl: Pointer to link frequency control
  * @pclk_ctrl: Pointer to pixel clock control

-- 
Sakari Ailus

^ permalink raw reply related	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-05-06 14:26 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-15 11:59 [PATCH v5 0/4] media: i2c: imx412: Add regulator control to imx412 Bryan O'Donoghue
2022-04-15 11:59 ` [PATCH v5 1/4] media: i2c: imx412: Fix reset GPIO polarity Bryan O'Donoghue
2022-04-25 14:41   ` Alessandrelli, Daniele
2022-04-15 11:59 ` [PATCH v5 2/4] media: i2c: imx412: Fix power_off ordering Bryan O'Donoghue
2022-04-25 14:43   ` Alessandrelli, Daniele
2022-04-15 11:59 ` [PATCH v5 3/4] media: dt-bindings: imx412: Add regulator descriptions Bryan O'Donoghue
2022-04-15 11:59 ` [PATCH v5 4/4] media: i2c: imx412: Add bulk regulator support Bryan O'Donoghue
2022-04-15 15:12   ` Jacopo Mondi
2022-04-26 16:52   ` Alessandrelli, Daniele
2022-05-06 14:26   ` Sakari Ailus

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.