Re: [PATCH v2 3/3] ARM: dts: rockchip: add rk3288 ISP and DPHY

[karthik poduval <karthik.poduval@gmail.com>]

I like the suggestion from Robin, drivers care about board clock
categories and don't need to specify the SoC specific clocks. But it
still looks like we are putting a union of all clocks based on current
and future versions of the ISP IP in the yaml. Is it necessary to list
them out at all ? Can't driver's simply get them from the device tree
as indexes instead of names using "of_clk_get". In that case does the
dts yaml need to check for the number of clocks per SoC variant ?

--
Regards,
Karthik Poduval

On Thu, Jul 2, 2020 at 12:17 PM Helen Koike <helen@koikeco.de> wrote:
>
> Hi Robin,
>
> On 7/2/20 2:32 PM, Robin Murphy wrote:
> > On 2020-07-02 17:27, Helen Koike wrote:
> > [...]
> >> I suggest this:
> >>
> >>    clocks:
> >>      maxItems: 5
> >>      minItems: 3
> >>      description:
> >>        rk3288 clocks
> >>          ISP clock
> >>          ISP AXI clock
> >>          ISP AHB clock
> >>          ISP Pixel clock
> >>          ISP JPEG source clock
> >>        rk3399 isp0 clocks
> >>          ISP clock
> >>          ISP AXI wrapper clock
> >>          ISP AHB wrapper clock
> >>        rk3399 isp1 clocks
> >>          ISP clock
> >>          ISP AXI wrapper clock
> >>          ISP AHB wrapper clock
> >>          ISP Pixel wrapper clock
> >>
> >>    clock-names:
> >>      oneOf:
> >>        # rk3288 clocks
> >>        - items:
> >>          - const: clk_isp
> >>          - const: aclk_isp
> >>          - const: hclk_isp
> >>          - const: pclk_isp_in
> >>          - const: sclk_isp_jpe
> >>        # rk3399 isp0 clocks
> >>        - items:
> >>          - const: clk_isp
> >>          - const: aclk_isp_wrap
> >>          - const: hclk_isp_wrap
> >>        # rk3399 isp1 clocks
> >>        - items:
> >>          - const: clk_isp
> >>          - const: aclk_isp_wrap
> >>          - const: hclk_isp_wrap
> >>          - const: pclk_isp_wrap
> >
> > FWIW this looks a little more involved than it might need to be. Ideally we're describing things from the point of view of what inputs the device itself wants, so the details of exactly *how* a particular SoC's clock tree delivers them shouldn't matter to the binding, only to the actual clock specifier values ultimately given in the DT.
> >
> > From the ISP's PoV, it seems like we've got the fairly standard core clock, ACLK and HCLK trio, plus a pixel clock for RK3288 and RK3399 ISP1, plus a JPEG source clock for RK3288. I'd be inclined to model that as simply something like:
> >
> >     clock-names:
> >       minItems: 3
> >       maxItems: 5
> >       items:
> >       - const: isp
> >       - const: aclk
> >       - const: hclk
> >       - const: pclk
> >       - const: sclk_jpe
> >
> > Plus then not only do we have a nice clean binding, but we avoid all the unnecessary faff of having to deal with the "same" clocks by different names in drivers, and sidestep the conundrum of what to do when the next SoC comes along providing the basic ISP clocks from yet again slightly-differently-named branches of its clock tree.
>
> I agree this is cleaner, thanks for this suggestions, I just submitted a new version
> following this https://patchwork.linuxtv.org/project/linux-media/list/?series=2844
>
> Thanks
> Helen
>
> >
> > Robin.



-- 
Regards,
Karthik Poduval