Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 15 Apr 2021 02:47:12 +0300
From:   Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To:     Jacopo Mondi <jacopo+renesas@jmondi.org>
Cc:     Geert Uytterhoeven <geert+renesas@glider.be>,
        Magnus Damm <magnus.damm@gmail.com>,
        Kieran Bingham <kieran.bingham@ideasonboard.com>,
        Rob Herring <robh+dt@kernel.org>,
        linux-renesas-soc@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/5] dt-bindings: media: max9286: Define
 'maxim,gpio-poc'
Message-ID: <YHd/AKIfxIHhTjWO@pendragon.ideasonboard.com>
References: <20210414135128.180980-1-jacopo+renesas@jmondi.org>
 <20210414135128.180980-2-jacopo+renesas@jmondi.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20210414135128.180980-2-jacopo+renesas@jmondi.org>
Precedence: bulk

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@jmondi.org>
> ---
>  .../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.

> +    $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 <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 ?

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 <laurent.pinchart@ideasonboard.com>

-- 
Regards,

Laurent Pinchart