Re: [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks

[Rob Herring]

On Wed, Jul 22, 2020 at 9:56 AM Helen Koike <helen.koike@xxxxxxxxxxxxx> wrote:
>
> aclk_isp_wrap is a child of aclk_isp, and hclk_isp_wrap is a child of
> hclk_isp, thus we can remove parents from the list.
>
> Also, for the isp0, we only need the ISP clock, ACLK and HCLK.
> In the future we'll need a pixel clock for RK3288 and RK3399, and a JPEG
> clock for RK3288.
>
> So with the goal to cleanup the dt-bindings and remove it from staging,
> simplify clock names to isp, aclk and hclk.
>
> Assigned clocks are meant to refer to the full path in the clock tree,
> i.e. the leaf in the tree.
> For instance, in RK3399, the clock responsible for ACLK (ISP AXI CLOCK)
> is aclk_isp0_wrapper.
>
> For reference, this is the isp clock topology on RK3399:
>
>  xin24m
>     pll_npll
>        npll
>           clk_isp1
>           clk_isp0
>     pll_cpll
>        cpll
>           aclk_isp1
>              aclk_isp1_noc
>              hclk_isp1
>                 aclk_isp1_wrapper
>                 hclk_isp1_noc
>           aclk_isp0
>              hclk_isp1_wrapper
>              aclk_isp0_wrapper
>              aclk_isp0_noc
>              hclk_isp0
>                 hclk_isp0_wrapper
>                 hclk_isp0_noc
>  pclkin_isp1_wrapper
>
> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx>
>
> ---
> Changes in V5:
> - Use if/then schema as suggested by Rob Herring on
> https://patchwork.linuxtv.org/project/linux-media/patch/20200702191322.2639681-6-helen.koike@xxxxxxxxxxxxx/#119729
>
> Changes in V4:
> - update binding according to suggestion by Robin Murphy
> on https://patchwork.kernel.org/patch/11475007/
>
> Changes in V3:
> - this is a new patch in the series
> ---
>  .../bindings/media/rockchip-isp1.yaml         | 50 ++++++++++++-------
>  drivers/staging/media/rkisp1/rkisp1-dev.c     |  8 ++-
>  2 files changed, 36 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> index 62a6b9c959498..23c677d15037a 100644
> --- a/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> +++ b/drivers/staging/media/rkisp1/Documentation/devicetree/bindings/media/rockchip-isp1.yaml
> @@ -24,20 +24,10 @@ properties:
>      maxItems: 1
>
>    clocks:
> -    items:
> -      - description: ISP clock
> -      - description: ISP AXI clock clock
> -      - description: ISP AXI clock  wrapper clock
> -      - description: ISP AHB clock clock
> -      - description: ISP AHB wrapper clock
> +    minItems: 3

You need maxItems here too or it will always be 3.

>
>    clock-names:
> -    items:
> -      - const: clk_isp
> -      - const: aclk_isp
> -      - const: aclk_isp_wrap
> -      - const: hclk_isp
> -      - const: hclk_isp_wrap
> +    minItems: 3
>
>    iommus:
>      maxItems: 1
> @@ -116,6 +106,34 @@ required:
>    - power-domains
>    - ports
>
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        const: rockchip,rk3399-cif-isp
> +then:
> +  properties:
> +    clocks:
> +      maxItems: 4
> +      minItems: 3

For a single compatible you shouldn't really have a variable number of clocks.

> +      items:
> +        # isp0 and isp1
> +        - description: ISP clock
> +        - description: ISP AXI clock
> +        - description: ISP AHB clock
> +        # only for isp1
> +        - description: ISP Pixel clock
> +    clock-names:
> +      maxItems: 4
> +      minItems: 3
> +      items:
> +        # isp0 and isp1
> +        - const: isp
> +        - const: aclk
> +        - const: hclk
> +        # only for isp1
> +        - const: pclk_isp

Don't you need an 'else' clause. For not rockchip,rk3399-cif-isp,
there's no definition of what clocks there are.

Rob