Re: [PATCH v10 1/3] media: dt-bindings: media: add bindings for Rockchip CIF

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 5131 bytes --]

Hi,

On Thu 09 Nov 23, 17:24, Conor Dooley wrote:
> On Wed, Nov 08, 2023 at 05:38:56PM +0100, Mehdi Djait wrote:
> > Add a documentation for the Rockchip Camera Interface binding.
> > 
> > the name of the file rk3066 is the first Rockchip SoC generation that uses cif
> > instead of the px30 which is just one of the many iterations of the unit.
> 
> I think this is becoming ridiculous. You've now removed the compatible
> for the rk3066 but kept it in the filename. I don't understand the
> hangup about naming the file after the px30-vip, but naming it after
> something that is not documented here at all makes no sense to me.
> Either document the rk3066 properly, or remove all mention of it IMO.

I think the opposite is ridiculous. We have spent some time investigating the
history of this unit, to find out that RK3066 is the first occurence where
it exists. Since we want the binding to cover all generations of the same unit
and give it a name that reflects this, rk3066 is the natural choice that comes
to mind. As far as I understand, this is the normal thing to do to name
bindings: name after the earliest known occurence of the unit.

What is the rationale behind naming the file after a generation of the unit
that happens to be the one introducing the binding? This is neither the first
nor the last one to include this unit. The binding will be updated later to
cover other generations. Do we want to rename the file each time an a generation
earlier than px30 is introduced? That sounds quite ridiculous too.

We've done the research work to give it the most relevant name here.
I'd expect some strong arguments not to use it. Can you ellaborate?

Cheers,

Paul


> Cheers,
> Conor.
> 
> > 
> > Signed-off-by: Mehdi Djait <mehdi.djait@bootlin.com>
> > ---
> >  .../bindings/media/rockchip,rk3066-cif.yaml   | 94 +++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml b/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> > new file mode 100644
> > index 000000000000..c3a5cd2baf71
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/media/rockchip,rk3066-cif.yaml
> > @@ -0,0 +1,94 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/media/rockchip,rk3066-cif.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip CIF Camera Interface
> > +
> > +maintainers:
> > +  - Mehdi Djait <mehdi.djait@bootlin.com>
> > +
> > +description:
> > +  CIF is a camera interface present on some rockchip SoCs. It receives the data
> > +  from Camera sensor or CCIR656 encoder and transfers it into system main memory
> > +  by AXI bus.
> > +
> > +properties:
> > +  compatible:
> > +    const: rockchip,px30-vip
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: ACLK
> > +      - description: HCLK
> > +      - description: PCLK
> > +
> > +  clock-names:
> > +    items:
> > +      - const: aclk
> > +      - const: hclk
> > +      - const: pclk
> > +
> > +  resets:
> > +    items:
> > +      - description: AXI
> > +      - description: AHB
> > +      - description: PCLK IN
> > +
> > +  reset-names:
> > +    items:
> > +      - const: axi
> > +      - const: ahb
> > +      - const: pclkin
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/properties/port
> > +    description: A connection to a sensor or decoder
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - clocks
> > +  - port
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/clock/px30-cru.h>
> > +    #include <dt-bindings/power/px30-power.h>
> > +
> > +    parent {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        video-capture@ff490000 {
> > +            compatible = "rockchip,px30-vip";
> > +            reg = <0x0 0xff490000 0x0 0x200>;
> > +            interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
> > +            clocks = <&cru ACLK_CIF>, <&cru HCLK_CIF>, <&cru PCLK_CIF>;
> > +            clock-names = "aclk", "hclk", "pclk";
> > +            resets = <&cru SRST_CIF_A>, <&cru SRST_CIF_H>, <&cru SRST_CIF_PCLKIN>;
> > +            reset-names = "axi", "ahb", "pclkin";
> > +            power-domains = <&power PX30_PD_VI>;
> > +
> > +            port {
> > +                endpoint {
> > +                    remote-endpoint = <&tw9900_out>;
> > +                };
> > +            };
> > +        };
> > +    };
> > +...
> > -- 
> > 2.41.0
> > 



-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]