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

[Bryan O'Donoghue <bryan.odonoghue@linaro.org>]

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