From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 976ECC4363A for ; Tue, 20 Oct 2020 15:14:57 +0000 (UTC) Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 294B122247 for ; Tue, 20 Oct 2020 15:14:56 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="NZMb+McC" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 294B122247 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=driverdev-devel-bounces@linuxdriverproject.org Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id A3C502E1C7; Tue, 20 Oct 2020 15:14:56 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id DJcb9gSsuiFZ; Tue, 20 Oct 2020 15:14:51 +0000 (UTC) Received: from ash.osuosl.org (ash.osuosl.org [140.211.166.34]) by silver.osuosl.org (Postfix) with ESMTP id F2A6E2E1CA; Tue, 20 Oct 2020 15:14:50 +0000 (UTC) Received: from whitealder.osuosl.org (smtp1.osuosl.org [140.211.166.138]) by ash.osuosl.org (Postfix) with ESMTP id D408E1BF82F for ; Tue, 20 Oct 2020 15:14:49 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by whitealder.osuosl.org (Postfix) with ESMTP id D096086663 for ; Tue, 20 Oct 2020 15:14:49 +0000 (UTC) X-Virus-Scanned: amavisd-new at osuosl.org Received: from whitealder.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id RrsA42uhYnqJ for ; Tue, 20 Oct 2020 15:14:49 +0000 (UTC) X-Greylist: domain auto-whitelisted by SQLgrey-1.7.6 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by whitealder.osuosl.org (Postfix) with ESMTPS id 0998186661 for ; Tue, 20 Oct 2020 15:14:49 +0000 (UTC) Received: from mail-ot1-f48.google.com (mail-ot1-f48.google.com [209.85.210.48]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 63EDE22283 for ; Tue, 20 Oct 2020 15:14:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603206888; bh=0u4Fv1fqrTFQ3Wp/EIuAvFVLUv/j7BCakcnneOAU5xs=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=NZMb+McCVdq/q6SuDog4g30r0l0prtO/gCN9O6gVcsaPkBYbZzxJe6dZwLnFZWph2 M0351LwXFAFPqpU9MMwcNqhFS0QuVRVoKOoQZN62uuHbU7C0DFfQ43/cynxfCiq2I3 SzdZPNgixrYnmyKSYQ7VBaaXvm9F8NGbIFGYgg7c= Received: by mail-ot1-f48.google.com with SMTP id e20so1994897otj.11 for ; Tue, 20 Oct 2020 08:14:48 -0700 (PDT) X-Gm-Message-State: AOAM531ZCdgGVx7+Zi1pxt+P8lQlEHZOKcLbMOKDFxFdRG3oKtySSzxx 6nlDIaVGbHHCqQHJcLk+bRin9+be0XJwGqMr+w== X-Google-Smtp-Source: ABdhPJz41UgbNkwtnCErGlmIpBGGzSTn7CHQ0TMdTIes7cHjDhnkd7e5tesUj1dAccQoyfMFCtdYvIGbHA+Y3i2/bE0= X-Received: by 2002:a9d:5e14:: with SMTP id d20mr1941932oti.107.1603206887469; Tue, 20 Oct 2020 08:14:47 -0700 (PDT) MIME-Version: 1.0 References: <20200722155533.252844-1-helen.koike@collabora.com> <20200722155533.252844-6-helen.koike@collabora.com> <2dcdda41-bdb4-55a8-557f-8175983effb5@collabora.com> In-Reply-To: <2dcdda41-bdb4-55a8-557f-8175983effb5@collabora.com> From: Rob Herring Date: Tue, 20 Oct 2020 10:14:35 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 5/9] media: staging: rkisp1: remove unecessary clocks To: Helen Koike X-BeenThere: driverdev-devel@linuxdriverproject.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "open list:STAGING SUBSYSTEM" , devicetree@vger.kernel.org, Eddie Cai , Tomasz Figa , Dafna Hirschfeld , "heiko@sntech.de" , Shunqian Zheng , "linux-kernel@vger.kernel.org" , karthik.poduval@gmail.com, "open list:ARM/Rockchip SoC..." , Hans Verkuil , Robin Murphy , Mark Rutland , Collabora Kernel ML , Ezequiel Garcia , Johan Jonker , Linux Media Mailing List Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" On Wed, Oct 14, 2020 at 11:46 AM Helen Koike 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 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 > >> > >> --- > >> 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@collabora.com/#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@gmail.com/ 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 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel