From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matthias Kaehlcke Subject: Re: [PATCH v2 2/2] ARM: dts: rockchip: consolidate veyron panel and backlight settings Date: Wed, 24 Jul 2019 15:38:23 -0700 Message-ID: <20190724223823.GC250418@google.com> References: <20190711223455.12210-1-mka@chromium.org> <20190711223455.12210-2-mka@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Doug Anderson Cc: Mark Rutland , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Heiko Stuebner , "open list:ARM/Rockchip SoC..." , Rob Herring , Linux ARM List-Id: devicetree@vger.kernel.org On Wed, Jul 24, 2019 at 02:46:30PM -0700, Doug Anderson wrote: > Hi, > > On Thu, Jul 11, 2019 at 3:35 PM Matthias Kaehlcke wrote: > > > > diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts b/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > index 4cc7d3659484..2b0801a539c9 100644 > > --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > @@ -15,40 +15,6 @@ > > "google,veyron-minnie-rev0", "google,veyron-minnie", > > "google,veyron", "rockchip,rk3288"; > > > > - backlight_regulator: backlight-regulator { > > - compatible = "regulator-fixed"; > > - enable-active-high; > > - gpio = <&gpio2 RK_PB4 GPIO_ACTIVE_HIGH>; > > - pinctrl-names = "default"; > > - pinctrl-0 = <&bl_pwr_en>; > > - regulator-name = "backlight_regulator"; > > - vin-supply = <&vcc33_sys>; > > - startup-delay-us = <15000>; > > - }; > > - > > - panel_regulator: panel-regulator { > > - compatible = "regulator-fixed"; > > - enable-active-high; > > - gpio = <&gpio7 RK_PB6 GPIO_ACTIVE_HIGH>; > > - pinctrl-names = "default"; > > - pinctrl-0 = <&lcd_enable_h>; > > - regulator-name = "panel_regulator"; > > - startup-delay-us = <100000>; > > - vin-supply = <&vcc33_sys>; > > - }; > > - > > - vcc18_lcd: vcc18-lcd { > > - compatible = "regulator-fixed"; > > - enable-active-high; > > - gpio = <&gpio2 RK_PB5 GPIO_ACTIVE_HIGH>; > > - pinctrl-names = "default"; > > - pinctrl-0 = <&avdd_1v8_disp_en>; > > - regulator-name = "vcc18_lcd"; > > - regulator-always-on; > > - regulator-boot-on; > > - vin-supply = <&vcc18_wl>; > > - }; > > - > > volume_buttons: volume-buttons { > > compatible = "gpio-keys"; > > pinctrl-names = "default"; > > You forgot to remove the line: > > power-supply = <&backlight_regulator>; > > ...from minnie. good catch, thanks! > > diff --git a/arch/arm/boot/dts/rk3288-veyron-pinky.dts b/arch/arm/boot/dts/rk3288-veyron-pinky.dts > > index 9b6f4d9b03b6..06af58e37a4b 100644 > > --- a/arch/arm/boot/dts/rk3288-veyron-pinky.dts > > +++ b/arch/arm/boot/dts/rk3288-veyron-pinky.dts > > @@ -14,7 +14,14 @@ > > compatible = "google,veyron-pinky-rev2", "google,veyron-pinky", > > "google,veyron", "rockchip,rk3288"; > > > > + /delete-node/backlight-regulator; > > + /delete-node/panel-regulator; > > /delete-node/emmc-pwrseq; > > + /delete-node/vcc18-lcd; > > +}; > > + > > +&backlight { > > + /delete-property/power-supply; > > }; > > > > &emmc { > > @@ -52,7 +59,17 @@ > > i2c-scl-rising-time-ns = <300>; > > }; > > > > +&panel { > > + power-supply= <&vcc33_lcd>; > > Might as well put a space before the "="? will do > > &pinctrl { > > + /delete-node/ lcd; > > + > > + backlight { > > + /delete-node/ bl_pwr_en; > > + }; > > I general as the defender of "pinky", I'll let Heiko confirm he's OK > with the color of this bikeshed. Sometimes a bit of repetition is > preferred over a bunch of confusing /delete-node/ statements since > those tend to make things harder to reason about in general. In this > case I think things are cleaner after your patch but I won't say it's > 100% clear cut. I agree that some repetition can be preferrable over /delete-node/ statements. In this case repetition is above my personal threshold, especially since I'm about to add another device with eDP display and would have to repeat the mostly redundant config yet another time. If Heiko prefer's the repetition so be it :) > Other than nits I have double-checked this patch, so feel free to add > my Reviewed-by after nits are fixed. Thanks for the review! 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=-5.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham 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 13D62C76186 for ; Wed, 24 Jul 2019 22:38:53 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (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 D46FD2189F for ; Wed, 24 Jul 2019 22:38:52 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="uV2f3QoX"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="h7bxbA9o" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D46FD2189F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=C/DQavGO9BeHHY/QonrhDrw3c1z1fIleVceyTBoR3Ps=; b=uV2f3QoXRGPaWx kVvno2BBSaF6AyjivWbT9MbztUv+xyBnHGqeNmuE/QgOvx9HOGwXXaK9rcIIQdYkMW3MBzvadNYW0 UKFvx1XpAHpA7Mfu3fFmFg8HMObNI7U0E14SQgY5n7xsbGY5yeZ+HoSaD89ZdzeD07AWbzt/aafin yxBG15LcojIj4uT+m5QiN8xlsKAQM6RBVu45FcRVOVDsbSSoOYq8Ukw8aSGzizKnaoiRDQbY/Xzbc Rf+9xD9regAf1tGvsKssmlWqXS621aNFlqDmP6f8UjYjl0wbGotI9bPl9SEeKe6UlM8LtQpcoRczI kTqImT7sanMclVMQacPQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92 #3 (Red Hat Linux)) id 1hqPuO-0006oR-C4; Wed, 24 Jul 2019 22:38:52 +0000 Received: from mail-pg1-x544.google.com ([2607:f8b0:4864:20::544]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hqPty-0006Zh-9F for linux-arm-kernel@lists.infradead.org; Wed, 24 Jul 2019 22:38:29 +0000 Received: by mail-pg1-x544.google.com with SMTP id t132so21910528pgb.9 for ; Wed, 24 Jul 2019 15:38:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=9Dc2qcjKZtC43VFkg+AkTg9oc60SQQXWdODT+iWj4Ig=; b=h7bxbA9onAD2dz2ar/lKtvI8U78LMFyUb0g0d+U6xZe0yA3oMMFnKOzblILDCft4YP RH3briYvP964xAXtME0s1rYg4P4VePZNvghmMiscYvXfL7PC8dwSHgZcNgfSwpbjRM4D rwM5CprkVXN1T0IA4M1iMetyWKchd7E6etbTM= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=9Dc2qcjKZtC43VFkg+AkTg9oc60SQQXWdODT+iWj4Ig=; b=NfHMJLHwYYs/C5KuKNZTciMPyQGE+p3OOeZrNAPcfybHWqojakO+pj2i4cvIHKXQGF I9vsKcevqr5lb/sFEq8xmirz8JbVOj6g+zrmdTpjxHzZkJKcJv2A5Vjr9aA+VjdZs9aH AMEU+pvspH734HWwZvGukj54cI2zm+Ygyfy8v0jbXeq/tSW30e/QLuzWPW/Gyp+7TJuF X9FU2gYt8XZ3vRppxTELwy21s7PYRcYBa+/oiYzPcd47OZ59KatdgVkNzQXU4c1EmEGh QTFzV6ZGGyg3dtX+BdurObcQa3woqdvbW2xa2oK9bfAUvCeub5MiAsuVtE2Ni7lsALg/ fgqg== X-Gm-Message-State: APjAAAWN0MY77xBvnHR74syn5WhQuZYKA8eLLIRYBY47sErSEk+PiABq rTFA+d9wCMfJ2ANq21ah+U22Ag== X-Google-Smtp-Source: APXvYqwROjs9pfNddDIyH0kjsUo4epMtSE+vqw102VBAtHJmpqhjw64Rmq9Jc/LIZW5Qf9E/JdehJQ== X-Received: by 2002:a62:874d:: with SMTP id i74mr13211045pfe.94.1564007905394; Wed, 24 Jul 2019 15:38:25 -0700 (PDT) Received: from localhost ([2620:15c:202:1:75a:3f6e:21d:9374]) by smtp.gmail.com with ESMTPSA id p7sm51433378pfp.131.2019.07.24.15.38.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 24 Jul 2019 15:38:24 -0700 (PDT) Date: Wed, 24 Jul 2019 15:38:23 -0700 From: Matthias Kaehlcke To: Doug Anderson Subject: Re: [PATCH v2 2/2] ARM: dts: rockchip: consolidate veyron panel and backlight settings Message-ID: <20190724223823.GC250418@google.com> References: <20190711223455.12210-1-mka@chromium.org> <20190711223455.12210-2-mka@chromium.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190724_153826_376584_B4C03426 X-CRM114-Status: GOOD ( 17.00 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , devicetree@vger.kernel.org, Heiko Stuebner , "open list:ARM/Rockchip SoC..." , Rob Herring , Linux ARM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Jul 24, 2019 at 02:46:30PM -0700, Doug Anderson wrote: > Hi, > > On Thu, Jul 11, 2019 at 3:35 PM Matthias Kaehlcke wrote: > > > > diff --git a/arch/arm/boot/dts/rk3288-veyron-minnie.dts b/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > index 4cc7d3659484..2b0801a539c9 100644 > > --- a/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > +++ b/arch/arm/boot/dts/rk3288-veyron-minnie.dts > > @@ -15,40 +15,6 @@ > > "google,veyron-minnie-rev0", "google,veyron-minnie", > > "google,veyron", "rockchip,rk3288"; > > > > - backlight_regulator: backlight-regulator { > > - compatible = "regulator-fixed"; > > - enable-active-high; > > - gpio = <&gpio2 RK_PB4 GPIO_ACTIVE_HIGH>; > > - pinctrl-names = "default"; > > - pinctrl-0 = <&bl_pwr_en>; > > - regulator-name = "backlight_regulator"; > > - vin-supply = <&vcc33_sys>; > > - startup-delay-us = <15000>; > > - }; > > - > > - panel_regulator: panel-regulator { > > - compatible = "regulator-fixed"; > > - enable-active-high; > > - gpio = <&gpio7 RK_PB6 GPIO_ACTIVE_HIGH>; > > - pinctrl-names = "default"; > > - pinctrl-0 = <&lcd_enable_h>; > > - regulator-name = "panel_regulator"; > > - startup-delay-us = <100000>; > > - vin-supply = <&vcc33_sys>; > > - }; > > - > > - vcc18_lcd: vcc18-lcd { > > - compatible = "regulator-fixed"; > > - enable-active-high; > > - gpio = <&gpio2 RK_PB5 GPIO_ACTIVE_HIGH>; > > - pinctrl-names = "default"; > > - pinctrl-0 = <&avdd_1v8_disp_en>; > > - regulator-name = "vcc18_lcd"; > > - regulator-always-on; > > - regulator-boot-on; > > - vin-supply = <&vcc18_wl>; > > - }; > > - > > volume_buttons: volume-buttons { > > compatible = "gpio-keys"; > > pinctrl-names = "default"; > > You forgot to remove the line: > > power-supply = <&backlight_regulator>; > > ...from minnie. good catch, thanks! > > diff --git a/arch/arm/boot/dts/rk3288-veyron-pinky.dts b/arch/arm/boot/dts/rk3288-veyron-pinky.dts > > index 9b6f4d9b03b6..06af58e37a4b 100644 > > --- a/arch/arm/boot/dts/rk3288-veyron-pinky.dts > > +++ b/arch/arm/boot/dts/rk3288-veyron-pinky.dts > > @@ -14,7 +14,14 @@ > > compatible = "google,veyron-pinky-rev2", "google,veyron-pinky", > > "google,veyron", "rockchip,rk3288"; > > > > + /delete-node/backlight-regulator; > > + /delete-node/panel-regulator; > > /delete-node/emmc-pwrseq; > > + /delete-node/vcc18-lcd; > > +}; > > + > > +&backlight { > > + /delete-property/power-supply; > > }; > > > > &emmc { > > @@ -52,7 +59,17 @@ > > i2c-scl-rising-time-ns = <300>; > > }; > > > > +&panel { > > + power-supply= <&vcc33_lcd>; > > Might as well put a space before the "="? will do > > &pinctrl { > > + /delete-node/ lcd; > > + > > + backlight { > > + /delete-node/ bl_pwr_en; > > + }; > > I general as the defender of "pinky", I'll let Heiko confirm he's OK > with the color of this bikeshed. Sometimes a bit of repetition is > preferred over a bunch of confusing /delete-node/ statements since > those tend to make things harder to reason about in general. In this > case I think things are cleaner after your patch but I won't say it's > 100% clear cut. I agree that some repetition can be preferrable over /delete-node/ statements. In this case repetition is above my personal threshold, especially since I'm about to add another device with eDP display and would have to repeat the mostly redundant config yet another time. If Heiko prefer's the repetition so be it :) > Other than nits I have double-checked this patch, so feel free to add > my Reviewed-by after nits are fixed. Thanks for the review! _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel