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

From: Sakari Ailus <sakari.ailus@iki.fi>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: paul.j.murphy@intel.com, daniele.alessandrelli@intel.com,
	mchehab@kernel.org, linux-media@vger.kernel.org,
	devicetree@vger.kernel.org, robert.foss@linaro.org,
	hfink@snap.com, jgrahsl@snap.com, dmitry.baryshkov@linaro.org,
	vladimir.zapolskiy@linaro.org
Subject: Re: [PATCH v3 2/2] media: i2c: imx412: Add bulk regulator support
Date: Fri, 15 Apr 2022 00:58:58 +0300	[thread overview]
Message-ID: <YliZIqg201pDH1aH@valkosipuli.retiisi.eu> (raw)
In-Reply-To: <784b5caf-1e1a-e57c-126e-4b28bc6ff7f8@linaro.org>

Hi Bryan,

On Thu, Apr 14, 2022 at 03:50:50PM +0100, Bryan O'Donoghue wrote:
> 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

It's still wrong and should be fixed. It seems there are no boards in the
DT source in the kernel using this sensor. These changes seem fine to me.

I'm not really worried about ACPI: it's unlikely the GPIO is even declared
for the sensor, and instead is controlled in AML. There can of course be
bugs in ACPI tables, too...

-- 
Sakari Ailus