Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'
Hi Jacopo,
On Thu, Apr 15, 2021 at 8:53 AM Jacopo Mondi wrote:
> On Thu, Apr 15, 2021 at 02:47:12AM +0300, Laurent Pinchart wrote:
> > On Wed, Apr 14, 2021 at 03:51:24PM +0200, Jacopo Mondi wrote:
> > > Define a new vendor property in the maxim,max9286 binding schema.
> > >
> > > The new property allows to declare that the remote camera
> > > power-over-coax is controlled by one of the MAX9286 gpio lines.
> > >
> > > As it is currently not possible to establish a regulator as consumer
> > > of the MAX9286 gpio controller for this purpose, the property allows to
> > > declare that the camera power is controlled by the MAX9286 directly.
> > >
> > > The property accepts a gpio-index (0 or 1) and one line polarity
> > > flag as defined by dt-bindings/gpio/gpio.h.
> > >
> > > Signed-off-by: Jacopo Mondi
> > > ---
> > > .../bindings/media/i2c/maxim,max9286.yaml | 53 ++-
> > > 1 file changed, 52 insertions(+), 1 deletion(-)
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > index ee16102fdfe7..480a491f3744 100644
> > > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > > @@ -70,6 +70,24 @@ properties:
> > >a remote serializer whose high-threshold noise immunity is not
> > > enabled
> > >is 10 micro volts
> > >
> > > + maxim,gpio-poc:
> >
> > I would have written poc-gpio to match the order of the GPIO bindings
> > syntax.
> >
>
> That's what I had :) but then the property gets matched against the
> gpio schema and I get complains because it expects a phandle as first
> argument... Maybe there's a way I've missed to prevent the property to
> be matched with *-gpio ?
GPIO hogs also use gpio properties lacking the phandle.
Hence the way this is handled for hogs may (or may not, it's yaml after all ;-)
inspire you how to handle this here.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'
Hi Laurent,
On Thu, Apr 15, 2021 at 02:47:12AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Apr 14, 2021 at 03:51:24PM +0200, Jacopo Mondi wrote:
> > Define a new vendor property in the maxim,max9286 binding schema.
> >
> > The new property allows to declare that the remote camera
> > power-over-coax is controlled by one of the MAX9286 gpio lines.
> >
> > As it is currently not possible to establish a regulator as consumer
> > of the MAX9286 gpio controller for this purpose, the property allows to
> > declare that the camera power is controlled by the MAX9286 directly.
> >
> > The property accepts a gpio-index (0 or 1) and one line polarity
> > flag as defined by dt-bindings/gpio/gpio.h.
> >
> > Signed-off-by: Jacopo Mondi
> > ---
> > .../bindings/media/i2c/maxim,max9286.yaml | 53 ++-
> > 1 file changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > index ee16102fdfe7..480a491f3744 100644
> > --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> > @@ -70,6 +70,24 @@ properties:
> >a remote serializer whose high-threshold noise immunity is not
> > enabled
> >is 10 micro volts
> >
> > + maxim,gpio-poc:
>
> I would have written poc-gpio to match the order of the GPIO bindings
> syntax.
>
That's what I had :) but then the property gets matched against the
gpio schema and I get complains because it expects a phandle as first
argument... Maybe there's a way I've missed to prevent the property to
be matched with *-gpio ?
> > +$ref: '/schemas/types.yaml#/definitions/uint32-array'
> > +minItems: 2
> > +maxItems: 2
> > +description: |
> > + Identifier of gpio line that controls Power over Coax to the cameras
> > and
>
> I'd write "Index of the MAX9286 GPIO output that ..." to make it clear
> that this is not a generic GPIO.
>
Ack
> > + the associated polarity flag (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW)
> > + as defined in .
> > +
> > + When the remote cameras power is controlled by one of the MAX9286
> > gpio
> > + lines, this property has to be used to specify which line among the
> > two
> > + available ones controls the remote camera power enablement.
> > +
> > + When this property is used it is not possible to register a gpio
> > + controller as the gpio lines are controlled directly by the MAX9286
> > and
> > + not available for consumers, nor the 'poc-supply' property should be
> > + specified.
>
> Only one of the two lines would be controlled directly. Shouldn't we
> still register a GPIO controller for the other line ?
I considered that and thought it was a bit of an overkill (and I also
had a bit of troubles identifying how to register only gpio #1, as it
would be identified as gpio #0 if the actual #0 is not registered)
>
> Could you also mention somewhere that the first item in the array should
> be 0 or 1 ? It may be hard to express in a YAML schema, so I'm fine just
> documenting it in the description.
>
Sure, I tried identifying how to express that with yaml and failed :)
> I've been wondering whether this would be a common enough issue that it
> could justify support in the GPIO core to handle consumer-provider
> loops, but even if that happens at some point in the future, I think the
> proposal here is good enough and we won't need to switch.
>
Please note that with the suggestion offline from rob I will add to
the next version:
# If 'maxim,gpio-poc' is present, then 'poc-supply' and 'gpio-controller'
# are not allowed.
if:
required:
- maxim,gpio-poc
then:
allOf:
- not:
required:
- poc-supply
- not:
required:
- gpio-controller
> > +
> >ports:
> > $ref: /schemas/graph.yaml#/properties/ports
> >
> > @@ -182,7 +200,6 @@ required:
> >- reg
> >- ports
> >- i2c-mux
> > - - gpio-controller
> >
> > additionalProperties: false
> >
> > @@ -327,4 +344,38 @@ examples:
> >};
> > };
> >};
> > +
> > + /*
> > + * Example of a deserializer that controls the camera Power over Coax
> > + * through one of its gpio lines.
> > + */
> > + gmsl-deserializer@6c {
> > +compatible = "maxim,max9286";
> > +reg = <0x6c>;
> > +enable-gpios = <&gpio 14 GPIO_ACTIVE_HIGH>;
> > +
> > +/*
> > + * The remote camera power is controlled by MAX9286 GPIO line #0.
> > + * No 'poc-supply' nor 'gpio-controller' are specified.
> > + */
> > +maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
> > +
> > +/*
> > + * Do not describe connections as they're the same as in the
> > previous
> > + * example.
> > + */
> > +ports {
> > +
Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define 'maxim,gpio-poc'
Hi Jacopo,
Thank you for the patch.
On Wed, Apr 14, 2021 at 03:51:24PM +0200, Jacopo Mondi wrote:
> Define a new vendor property in the maxim,max9286 binding schema.
>
> The new property allows to declare that the remote camera
> power-over-coax is controlled by one of the MAX9286 gpio lines.
>
> As it is currently not possible to establish a regulator as consumer
> of the MAX9286 gpio controller for this purpose, the property allows to
> declare that the camera power is controlled by the MAX9286 directly.
>
> The property accepts a gpio-index (0 or 1) and one line polarity
> flag as defined by dt-bindings/gpio/gpio.h.
>
> Signed-off-by: Jacopo Mondi
> ---
> .../bindings/media/i2c/maxim,max9286.yaml | 53 ++-
> 1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> index ee16102fdfe7..480a491f3744 100644
> --- a/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> +++ b/Documentation/devicetree/bindings/media/i2c/maxim,max9286.yaml
> @@ -70,6 +70,24 @@ properties:
>a remote serializer whose high-threshold noise immunity is not enabled
>is 10 micro volts
>
> + maxim,gpio-poc:
I would have written poc-gpio to match the order of the GPIO bindings
syntax.
> +$ref: '/schemas/types.yaml#/definitions/uint32-array'
> +minItems: 2
> +maxItems: 2
> +description: |
> + Identifier of gpio line that controls Power over Coax to the cameras
> and
I'd write "Index of the MAX9286 GPIO output that ..." to make it clear
that this is not a generic GPIO.
> + the associated polarity flag (GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW)
> + as defined in .
> +
> + When the remote cameras power is controlled by one of the MAX9286 gpio
> + lines, this property has to be used to specify which line among the two
> + available ones controls the remote camera power enablement.
> +
> + When this property is used it is not possible to register a gpio
> + controller as the gpio lines are controlled directly by the MAX9286 and
> + not available for consumers, nor the 'poc-supply' property should be
> + specified.
Only one of the two lines would be controlled directly. Shouldn't we
still register a GPIO controller for the other line ?
Could you also mention somewhere that the first item in the array should
be 0 or 1 ? It may be hard to express in a YAML schema, so I'm fine just
documenting it in the description.
I've been wondering whether this would be a common enough issue that it
could justify support in the GPIO core to handle consumer-provider
loops, but even if that happens at some point in the future, I think the
proposal here is good enough and we won't need to switch.
> +
>ports:
> $ref: /schemas/graph.yaml#/properties/ports
>
> @@ -182,7 +200,6 @@ required:
>- reg
>- ports
>- i2c-mux
> - - gpio-controller
>
> additionalProperties: false
>
> @@ -327,4 +344,38 @@ examples:
>};
> };
>};
> +
> + /*
> + * Example of a deserializer that controls the camera Power over Coax
> + * through one of its gpio lines.
> + */
> + gmsl-deserializer@6c {
> +compatible = "maxim,max9286";
> +reg = <0x6c>;
> +enable-gpios = <&gpio 14 GPIO_ACTIVE_HIGH>;
> +
> +/*
> + * The remote camera power is controlled by MAX9286 GPIO line #0.
> + * No 'poc-supply' nor 'gpio-controller' are specified.
> + */
> +maxim,gpio-poc = <0 GPIO_ACTIVE_LOW>;
> +
> +/*
> + * Do not describe connections as they're the same as in the previous
> + * example.
> + */
> +ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@4 {
> +reg = <4>;
> + };
> +};
> +
> +i2c-mux {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +};
> + };
> };
It's customary to indent DT examples with 4 spaces. The existing
examples use two spaces, so maybe a patch on top of this would be useful
to increase readability ?
Reviewed-by: Laurent Pinchart
--
Regards,
Laurent Pinchart
