| Message ID | 20240501152442.1072627-22-git@luigi311.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | imx258 improvement series | expand |
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.
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.
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
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.
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
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
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.
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.