Hi Rob, On 10/20/20 12:14 PM, Rob Herring wrote: > On Wed, Oct 14, 2020 at 11:46 AM Helen Koike <helen.koike@xxxxxxxxxxxxx> wrote: >> >> Hi Rob, >> >> Thnaks for your reply. >> >> On 9/22/20 11:24 AM, Rob Herring wrote: >>> 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. >> >> I'm not entirely sure how to make this separation, since isp0 and isp1 (not yet supported) >> would use the same compatible. >> Unless if we separate in two compatibles, but maybe this is an overhead just for an extra clock. >> What do you think? > > In that case, it's fine. > >> >>> >>>> + 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. >> >> There is only one compatible defined for now, rk3288 will be added later. >> The idea to add if/then is to make it easier to add rk3288: >> >> https://patchwork.kernel.org/project/linux-media/patch/20200406073017.19462-4-karthik.poduval@xxxxxxxxx/ > > Hopefully, the clock names will be aligned? Looks like they are the > same with just 1 additional clock. Ideally, you define them all at the > top level and the if/then schema just defines how many clocks for each > compatible. I submitted another version, where I try to capture what you suggested here, please check if I got it right this time (or not). Maybe I misunderstood which kind of alignment you are expecting for the clock names, should they be each in a different line? https://patchwork.linuxtv.org/project/linux-media/patch/20201020193850.1460644-6-helen.koike@xxxxxxxxxxxxx/ Thanks Helen
