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.
