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. Rob
