Re: [PATCH v2 2/4] dt-bindings: media: Add bindings for raspberrypi,rp1-cfe

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Wed, Aug 28, 2024 at 03:14:47PM +0300, Tomi Valkeinen wrote:
> On 28/08/2024 14:12, Laurent Pinchart wrote:
> > On Wed, Aug 28, 2024 at 11:06:23AM +0000, Sakari Ailus wrote:
> >> On Thu, Jun 20, 2024 at 02:07:51PM +0300, Tomi Valkeinen wrote:
> >>> Add DT bindings for raspberrypi,rp1-cfe.
> >>>
> >>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>> ---
> >>>   .../bindings/media/raspberrypi,rp1-cfe.yaml        | 98 ++++++++++++++++++++++
> >>>   1 file changed, 98 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml
> >>> new file mode 100644
> >>> index 000000000000..851533de2305
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/media/raspberrypi,rp1-cfe.yaml
> >>> @@ -0,0 +1,98 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/media/raspberrypi,rp1-cfe.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Raspberry Pi PiSP Camera Front End
> >>> +
> >>> +maintainers:
> >>> +  - Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >>> +  - Raspberry Pi Kernel Maintenance <kernel-list@raspberrypi.com>
> >>> +
> >>> +description: |
> >>> +  The Raspberry Pi PiSP Camera Front End is a module in Raspberrypi 5's RP1 I/O
> >>> +  controller, that contains:
> >>> +  - MIPI D-PHY
> >>> +  - MIPI CSI-2 receiver
> >>> +  - Simple image processor (called PiSP Front End, or FE)
> >>> +
> >>> +  The FE documentation is available at:
> >>> +  https://datasheets.raspberrypi.com/camera/raspberry-pi-image-signal-processor-specification.pdf
> >>> +
> >>> +  The PHY and CSI-2 receiver part have no public documentation.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    items:
> >>> +      - const: raspberrypi,rp1-cfe
> >>> +
> >>> +  reg:
> >>> +    items:
> >>> +      - description: CSI-2 registers
> >>> +      - description: D-PHY registers
> >>> +      - description: MIPI CFG (a simple top-level mux) registers
> >>> +      - description: FE registers
> >>> +
> >>> +  interrupts:
> >>> +    maxItems: 1
> >>> +
> >>> +  clocks:
> >>> +    maxItems: 1
> >>> +
> >>> +  port:
> >>> +    $ref: /schemas/graph.yaml#/$defs/port-base
> >>> +    additionalProperties: false
> >>> +    description: CSI-2 RX Port
> >>> +
> >>> +    properties:
> >>> +      endpoint:
> >>> +        $ref: video-interfaces.yaml#
> >>> +        unevaluatedProperties: false
> >>> +
> >>> +        properties:
> >>> +          data-lanes:
> >>> +            minItems: 1
> >>> +            maxItems: 4
> >>> +
> >>> +          clock-lanes:
> >>> +            maxItems: 1
> >>
> >> minItems needs to be 1 as well.
> 
> Hmm, I see a lot of
> 
> clock-lanes:
>    maxItems: 1
> 
> in the device tree bindings. And 
> https://docs.kernel.org/devicetree/bindings/writing-schema.html says 
> "Cases that have only a single entry just need to express that with 
> maxItems".

The rules may have changed recently, I'm not entirely sure. I've asked
in https://lore.kernel.org/dri-devel/20240818173003.122025-1-krzysztof.kozlowski@linaro.org/T/#m7669ca56543567e36733af745798713ae0293654

> >> Or... is this actually configurable in hardware?
> > 
> > Looking at the driver, lane reordering is not supported, so we could
> > drop this property. If the hardware is found to support it later, it can
> > easily be added back without any backward compatibility issue.
> 
> Re-ordering is not supported. I guess clock-lanes can be dropped, 
> although I feel that if we have the clock lane in the hardware, and the 
> numbering of data-lanes must take that into account, then:
> 
> clock-lanes = <0>;
> data-lanes = <1 2>;
> 
> looks better than:
> 
> data-lanes = <1 2>; /* and implicit clk lane 0 */
> 
> But I can't think of any practical benefit it brings...

-- 
Regards,

Laurent Pinchart