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 <jacopo+renesas@xxxxxxxxxx>
> > ---
> > .../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 100000 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 <include/dt-bindings/gpio/gpio.h>.
> > +
> > + 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 {
> > + #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 ?
>
Ah weird! I can add a patch before this one!
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
Thanks
j
> --
> Regards,
>
> Laurent Pinchart
