[v5,21/25] dt-bindings: media: imx258: Add binding for powerdown-gpio

Message ID 20240501152442.1072627-22-git@luigi311.com (mailing list archive)
State Changes Requested
Delegated to: Sakari Ailus
Headers
Series imx258 improvement series |

Commit Message

Luis Garcia May 1, 2024, 3:24 p.m. UTC
  From: Ondrej Jirman <megi@xff.cz>

Add powerdown-gpio binding as it is required for some boards.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
Signed-off-by: Luis Garcia <git@luigi311.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Pavel Machek <pavel@ucw.cz>
---
 Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml | 4 ++++
 1 file changed, 4 insertions(+)
  

Comments

sakari.ailus@linux.intel.com May 17, 2024, 8:31 a.m. UTC | #1
Hi Luis,

On Wed, May 01, 2024 at 09:24:38AM -0600, git@luigi311.com wrote:
> From: Ondrej Jirman <megi@xff.cz>
> 
> Add powerdown-gpio binding as it is required for some boards.

I thought the conclusion was that this wasn't a property of the sensor? If
it needs to be controlled, then this should take place somewhere else than
in the sensor driver.

> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> Signed-off-by: Luis Garcia <git@luigi311.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Reviewed-by: Pavel Machek <pavel@ucw.cz>
> ---
>  Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> index c978abc0cdb3..33338139e6e8 100644
> --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> @@ -36,6 +36,10 @@ properties:
>    reg:
>      maxItems: 1
>  
> +  powerdown-gpios:
> +    description:
> +      Reference to the GPIO connected to the PWDN pin, if any.
> +
>    reset-gpios:
>      description: |-
>        Reference to the GPIO connected to the XCLR pin, if any.
  
sakari.ailus@linux.intel.com May 20, 2024, 12:50 p.m. UTC | #2
On Fri, May 17, 2024 at 08:31:35AM +0000, Sakari Ailus wrote:
> Hi Luis,
> 
> On Wed, May 01, 2024 at 09:24:38AM -0600, git@luigi311.com wrote:
> > From: Ondrej Jirman <megi@xff.cz>
> > 
> > Add powerdown-gpio binding as it is required for some boards.
> 
> I thought the conclusion was that this wasn't a property of the sensor? If
> it needs to be controlled, then this should take place somewhere else than
> in the sensor driver.

I'll take patches up to the previous and then 24 and 25 from this version.
The rest will need some rework.
  
Ondřej Jirman May 20, 2024, 12:55 p.m. UTC | #3
On Fri, May 17, 2024 at 08:31:35AM GMT, Sakari Ailus wrote:
> Hi Luis,
> 
> On Wed, May 01, 2024 at 09:24:38AM -0600, git@luigi311.com wrote:
> > From: Ondrej Jirman <megi@xff.cz>
> > 
> > Add powerdown-gpio binding as it is required for some boards.
> 
> I thought the conclusion was that this wasn't a property of the sensor? If
> it needs to be controlled, then this should take place somewhere else than
> in the sensor driver.

It's a property of the sensor modules. It's just optional on
some, eg. (pin 8):

  https://assets-global.website-files.com/63b65bd4974577341e1fe194/654290d4d0fb173e87f754ed_IMX_258_FF_drawing.png

Where else should it be so that the module is described properly in the 
DT and the powerdown signal can be used as part of powerup/down sequence
of the sensor?

regards,
	o.

> > 
> > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > Signed-off-by: Luis Garcia <git@luigi311.com>
> > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > ---
> >  Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > index c978abc0cdb3..33338139e6e8 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > @@ -36,6 +36,10 @@ properties:
> >    reg:
> >      maxItems: 1
> >  
> > +  powerdown-gpios:
> > +    description:
> > +      Reference to the GPIO connected to the PWDN pin, if any.
> > +
> >    reset-gpios:
> >      description: |-
> >        Reference to the GPIO connected to the XCLR pin, if any.
> 
> -- 
> Regards,
> 
> Sakari Ailus
  
sakari.ailus@linux.intel.com May 20, 2024, 1:19 p.m. UTC | #4
Hi Ondřej,

On Mon, May 20, 2024 at 02:55:26PM +0200, Ondřej Jirman wrote:
> On Fri, May 17, 2024 at 08:31:35AM GMT, Sakari Ailus wrote:
> > Hi Luis,
> > 
> > On Wed, May 01, 2024 at 09:24:38AM -0600, git@luigi311.com wrote:
> > > From: Ondrej Jirman <megi@xff.cz>
> > > 
> > > Add powerdown-gpio binding as it is required for some boards.
> > 
> > I thought the conclusion was that this wasn't a property of the sensor? If
> > it needs to be controlled, then this should take place somewhere else than
> > in the sensor driver.
> 
> It's a property of the sensor modules. It's just optional on
> some, eg. (pin 8):
> 
>   https://assets-global.website-files.com/63b65bd4974577341e1fe194/654290d4d0fb173e87f754ed_IMX_258_FF_drawing.png
> 
> Where else should it be so that the module is described properly in the 
> DT and the powerdown signal can be used as part of powerup/down sequence
> of the sensor?

That's an interesting case. The document does suggest the PWDN pin isn't
connected though.

Is this pin used for something? If so, what is it? The sensor doesn't have
it and generally the module in this respect just contains wiring.

Someone earlier suggested this could have been related to the VCM.
  
Dave Stevenson May 20, 2024, 1:20 p.m. UTC | #5
Hi Ondřej

On Mon, 20 May 2024 at 13:55, Ondřej Jirman <megi@xff.cz> wrote:
>
> On Fri, May 17, 2024 at 08:31:35AM GMT, Sakari Ailus wrote:
> > Hi Luis,
> >
> > On Wed, May 01, 2024 at 09:24:38AM -0600, git@luigi311.com wrote:
> > > From: Ondrej Jirman <megi@xff.cz>
> > >
> > > Add powerdown-gpio binding as it is required for some boards.
> >
> > I thought the conclusion was that this wasn't a property of the sensor? If
> > it needs to be controlled, then this should take place somewhere else than
> > in the sensor driver.
>
> It's a property of the sensor modules. It's just optional on
> some, eg. (pin 8):
>
>   https://assets-global.website-files.com/63b65bd4974577341e1fe194/654290d4d0fb173e87f754ed_IMX_258_FF_drawing.png
>
> Where else should it be so that the module is described properly in the
> DT and the powerdown signal can be used as part of powerup/down sequence
> of the sensor?

From v3 [1] Luis reported testing dropping the powerdown-gpio on a PPP
and it working fine.

I linked to the IMX258 datasheet in the same thread[2], and that
datasheet does not include such a signal on the imx258 sensor itself.

If your module has a powerdown gpio, then you'll have to ask the
module vendor what it is actually connected to. Potentially it relates
to the VCM driver rather than the sensor.

  Dave

[1] https://www.spinics.net/lists/linux-media/msg252519.html
[2] https://www.spinics.net/lists/linux-media/msg252496.html

> regards,
>         o.
>
> > >
> > > Signed-off-by: Ondrej Jirman <megi@xff.cz>
> > > Signed-off-by: Luis Garcia <git@luigi311.com>
> > > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> > > Reviewed-by: Pavel Machek <pavel@ucw.cz>
> > > ---
> > >  Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > > index c978abc0cdb3..33338139e6e8 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
> > > @@ -36,6 +36,10 @@ properties:
> > >    reg:
> > >      maxItems: 1
> > >
> > > +  powerdown-gpios:
> > > +    description:
> > > +      Reference to the GPIO connected to the PWDN pin, if any.
> > > +
> > >    reset-gpios:
> > >      description: |-
> > >        Reference to the GPIO connected to the XCLR pin, if any.
> >
> > --
> > Regards,
> >
> > Sakari Ailus
  
Ondřej Jirman May 20, 2024, 7:04 p.m. UTC | #6
Hi Dave,

On Mon, May 20, 2024 at 02:20:28PM GMT, Dave Stevenson wrote:
> Hi Ondřej
>
> [...]
> 
> From v3 [1] Luis reported testing dropping the powerdown-gpio on a PPP
> and it working fine.
> 
> I linked to the IMX258 datasheet in the same thread[2], and that
> datasheet does not include such a signal on the imx258 sensor itself.
> 
> If your module has a powerdown gpio, then you'll have to ask the
> module vendor what it is actually connected to. Potentially it relates
> to the VCM driver rather than the sensor.

I've tested it in various ways (inverting the signal, etc.) and it doesn't
seem to do anything. So these patches related to powerdown-gpio can
be dropped.

Kind regards,
	o.

>   Dave
> 
> [1] https://www.spinics.net/lists/linux-media/msg252519.html
> [2] https://www.spinics.net/lists/linux-media/msg252496.html
  
Luis Garcia June 2, 2024, 8:20 p.m. UTC | #7
On 5/20/24 06:50, Sakari Ailus wrote:
> On Fri, May 17, 2024 at 08:31:35AM +0000, Sakari Ailus wrote:
>> Hi Luis,
>>
>> On Wed, May 01, 2024 at 09:24:38AM -0600, git@luigi311.com wrote:
>>> From: Ondrej Jirman <megi@xff.cz>
>>>
>>> Add powerdown-gpio binding as it is required for some boards.
>>
>> I thought the conclusion was that this wasn't a property of the sensor? If
>> it needs to be controlled, then this should take place somewhere else than
>> in the sensor driver.
> 
> I'll take patches up to the previous and then 24 and 25 from this version.
> The rest will need some rework.
> 

Last time i looked at the comments for these two patch discussion was still
going on so i was submitting fixes for the other patches while this continued.
Looks like the final consensuses is for these two to be dropped so I just
submitted v6 with them both dropped. I also tested it on my PPP and it still
continues to work as expected just like it did before. Take a look at v6 and
let me know what else requires rework on there.
  

Patch

diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
index c978abc0cdb3..33338139e6e8 100644
--- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml
@@ -36,6 +36,10 @@  properties:
   reg:
     maxItems: 1
 
+  powerdown-gpios:
+    description:
+      Reference to the GPIO connected to the PWDN pin, if any.
+
   reset-gpios:
     description: |-
       Reference to the GPIO connected to the XCLR pin, if any.