Re: [PATCH v3 2/2] media: i2c: imx412: Add bulk regulator support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 14/04/2022 15:16, Sakari Ailus wrote:
On Thu, Apr 14, 2022 at 03:04:10PM +0100, Bryan O'Donoghue wrote:
On 14/04/2022 14:56, Sakari Ailus wrote:
On Thu, Apr 14, 2022 at 02:44:00PM +0100, Bryan O'Donoghue wrote:
On 14/04/2022 14:00, Sakari Ailus wrote:
    	ret = clk_prepare_enable(imx412->inclk);
    	if (ret) {
+		regulator_bulk_disable(imx412->num_supplies,
+				       imx412->supplies);
As the function already has an error handling section using labels, this
should go there as well.

Are you asking to move regulator_bulk_disable() to error_reset ?

No. You'll need another label.


Hmm.

I think another label is not required, have a look at V4.

Ah, yes, indeed. There's just a single location where this will be needed.

On another note, gpiod_set_value_cansleep() seems to enable reset in
resume and disable it in suspend. I.e. the polarity is wrong.


Agreed, the polarity looks wrong - in my DTS right now I have ACTIVE_HIGH for the relevant GPIO.

For example if I do this

@@ -1363,7 +1363,7 @@ camera@1a {
                compatible = "sony,imx412";
                reg = <0x1a>;

-               reset-gpios = <&tlmm 78 GPIO_ACTIVE_HIGH>;
+               reset-gpios = <&tlmm 78 GPIO_ACTIVE_LOW>;
                pinctrl-names = "default", "suspend";
                pinctrl-0 = <&cam2_default>;
                pinctrl-1 = <&cam2_suspend>;
diff --git a/drivers/media/i2c/imx412.c b/drivers/media/i2c/imx412.c
index a9cdf4694d58..1442b416f5aa 100644
--- a/drivers/media/i2c/imx412.c
+++ b/drivers/media/i2c/imx412.c
@@ -1036,7 +1036,7 @@ static int imx412_power_on(struct device *dev)
                return 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) {
@@ -1049,7 +1049,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);
        regulator_bulk_disable(imx412->num_supplies, imx412->supplies);

        return ret;
@@ -1068,7 +1068,7 @@ static int imx412_power_off(struct device *dev)

        clk_disable_unprepare(imx412->inclk);

-       gpiod_set_value_cansleep(imx412->reset_gpio, 0);
+       gpiod_set_value_cansleep(imx412->reset_gpio, 1);

Seems like changing the logic would negatively affect the Intel people. Might have to churn ACPI to change that logic..

Easier probably to leave as is and define as ACTIVE_HIGH in DTS



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux