Thank you Tomasz and Robin for your comments, On 10/14/20 1:43 PM, Tomasz Figa wrote: > On Wed, Oct 14, 2020 at 6:27 PM Helen Koike <helen.koike@xxxxxxxxxxxxx> wrote: >> >> Hi Tomasz, >> >> On 9/26/20 10:00 AM, Tomasz Figa wrote: >>> Hi Helen, >>> >>> On Wed, Jul 22, 2020 at 12:55:32PM -0300, Helen Koike wrote: >>>> From: Shunqian Zheng <zhengsq@xxxxxxxxxxxxxx> >>>> >>>> RK3399 has two ISPs, but only isp0 was tested. >>>> Add isp0 node in rk3399 dtsi >>>> >>>> Verified with: >>>> make ARCH=arm64 dtbs_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/media/rockchip-isp1.yaml >>>> >>>> Signed-off-by: Shunqian Zheng <zhengsq@xxxxxxxxxxxxxx> >>>> Signed-off-by: Jacob Chen <jacob2.chen@xxxxxxxxxxxxxx> >>>> Signed-off-by: Helen Koike <helen.koike@xxxxxxxxxxxxx> >>>> >>>> --- >>>> >>>> V4: >>>> - update clock names >>>> >>>> V3: >>>> - clean up clocks >>>> >>>> V2: >>>> - re-order power-domains property >>>> >>>> V1: >>>> This patch was originally part of this patchset: >>>> >>>> https://patchwork.kernel.org/patch/10267431/ >>>> >>>> The only difference is: >>>> - add phy properties >>>> - add ports >>>> --- >>>> arch/arm64/boot/dts/rockchip/rk3399.dtsi | 25 ++++++++++++++++++++++++ >>>> 1 file changed, 25 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3399.dtsi b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>>> index dba9641947a3a..ed8ba75dbbce8 100644 >>>> --- a/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>>> +++ b/arch/arm64/boot/dts/rockchip/rk3399.dtsi >>>> @@ -1721,6 +1721,31 @@ vopb_mmu: iommu@ff903f00 { >>>> status = "disabled"; >>>> }; >>>> >>>> + isp0: isp0@ff910000 { >>>> + compatible = "rockchip,rk3399-cif-isp"; >>>> + reg = <0x0 0xff910000 0x0 0x4000>; >>>> + interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH 0>; >>>> + clocks = <&cru SCLK_ISP0>, >>>> + <&cru ACLK_ISP0_WRAPPER>, >>>> + <&cru HCLK_ISP0_WRAPPER>; >>>> + clock-names = "isp", "aclk", "hclk"; >>>> + iommus = <&isp0_mmu>; >>>> + phys = <&mipi_dphy_rx0>; >>>> + phy-names = "dphy"; >>>> + power-domains = <&power RK3399_PD_ISP0>; >>> >>> Should this have status = "disabled" too? The mipi_dphy_rx0 node is >>> disabled by default too, so in the default configuration the driver >>> would always fail to probe. >> >> I'm thinking what is the overall guideline here. >> Since isp and mipi_dphy are always present in the rk3399, shouldn't they always be enabled? >> Or since they are only useful if a sensor is present, we should let the dts of the board to >> enable it? > > I don't have a strong opinion. I'm fine with enabling both by default > as well, as it shouldn't hurt. > > That said, I recall some alternative CIF IP block being present on > this SoC as well (and patches posted recently), which AFAIR can't be > activated at the same time as the ISP, so perhaps both of the > alternatives should be disabled by default? > > Best regards, > Tomasz > Based on these two last emails, I think it make sense to disable them by default. I'll just wait for feedback on patch 5/9 to submit an updated version. Thanks Helen
