Re: [PATCH v2 2/2] arm64: dts: mediatek: add mt8173 elm and hana board

From: Matthias Kaehlcke <mka@chromium.org>
To: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	devicetree@vger.kernel.org,
	Nicolas Boichat <drinkcat@chromium.org>,
	linux-kernel@vger.kernel.org, Daniel Kurtz <djkurtz@chromium.org>,
	Doug Anderson <dianders@google.com>,
	Rob Herring <robh+dt@kernel.org>,
	linux-mediatek@lists.infradead.org,
	Hsin-Yi Wang <hsinyi@chromium.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 2/2] arm64: dts: mediatek: add mt8173 elm and hana board
Date: Mon, 13 Jan 2020 10:18:07 -0800	[thread overview]
Message-ID: <20200113181807.GH89495@google.com> (raw)
In-Reply-To: <cce7b5a4-7e5e-99c7-c647-fbb2d58ff3e0@collabora.com>

Hi,

On Mon, Jan 13, 2020 at 10:29:51AM +0100, Enric Balletbo i Serra wrote:
> Hi Hsin-Yi,
> 
> Apart from the maintainer comments, a few more comments ...
> 
> cc'ing Matthias and Doug who I know played a bit with the pwm-backlight for the
> uprev of the veyron devices, and you might be interested in his feedback in general.
> 
> On 10/1/20 8:37, Hsin-Yi Wang wrote:
> > Elm is Acer Chromebook R13. Hana is Lenovo Chromebook. Both uses mt8173
> > SoC.
> > 
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > Changes in v2:
> > - remove downstream nodes and unused nodes
> > - use GPIO_ACTIVE_LOW for ps8640 gpios
> > - move trackpad to hana
> > ---
> >  arch/arm64/boot/dts/mediatek/Makefile         |    3 +
> >  .../dts/mediatek/mt8173-elm-hana-rev7.dts     |   27 +
> >  .../boot/dts/mediatek/mt8173-elm-hana.dts     |   16 +
> >  .../boot/dts/mediatek/mt8173-elm-hana.dtsi    |   60 +
> >  arch/arm64/boot/dts/mediatek/mt8173-elm.dts   |   15 +
> >  arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi  | 1040 +++++++++++++++++
> >  6 files changed, 1161 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8173-elm-hana-rev7.dts
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dts
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8173-elm-hana.dtsi
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8173-elm.dts
> >  create mode 100644 arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi

<snip>

> > diff --git a/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > new file mode 100644
> > index 000000000000..2ac738bebe04
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/mediatek/mt8173-elm.dtsi
> > @@ -0,0 +1,1040 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright 2016 MediaTek Inc.
> > + */
> > +
> > +#include <dt-bindings/input/input.h>
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include "mt8173.dtsi"
> > +
> > +/ {
> > +	aliases {
> > +		serial0 = &uart0;
> > +		serial1 = &uart1;
> > +		serial2 = &uart2;
> > +		serial3 = &uart3;
> > +	};
> 
> I think this should be in mt8173.dtsi?
> 
> > +
> > +	memory@40000000 {
> > +		device_type = "memory";
> > +		reg = <0 0x40000000 0 0x80000000>;
> > +	};
> > +
> > +	backlight_lcd: backlight_lcd {
> 
> nit: Not sure if you need to change or not, but this node is usually called
> backlight: backlight without the _lcd. Note that this imply a userspace change
> but /sys/class/backlight/backlight is more aligned with other boards (I guess)
> when you have only one backlight. See for example:
> 
>  git grep backlight arch/arm64/boot/*
> 
> > +		compatible = "pwm-backlight";
> > +		pwms = <&pwm0 0 1000000>;
> > +		brightness-levels = <
> > +			  0  16  32  48  64  80  96 112
> > +			128 144 160 176 192 208 224 240
> > +			255
> > +		>;
> > +		default-brightness-level = <9>;
> 
> The reason you see the display too dark is because userspace sets and remembers
> the default value you have using the above configuration which is only 16
> levels. So after removing the two above properties you should set a new
> brightness from userspace.
> 
> AFAIK 16 levels is not enough to have fancy backlight effects on Chrome OS
> userspace, at least is not enough for Rockchip devices so I suppose is the same
> for Mediatek.
> 
> You have to options here.
> 
> A) Remove brightness-levels and default-brightness-level. This will end with a
> non-linear brightness curve for the backlight up to 4095 levels.
> 
> # cat max_brightness
> 4095
> 
> B) Use the num-interpolated-steps and default-brightness-level properties. This
> will end with a linear curve brightness. Similar to what is done for veyron
> devices but in this case with more levels because the PWM to control the
> backlight is 16-bits instead of 8-bits for veyron devices.
> 
> *
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/rk3288-veyron-edp.dtsi#n42
> 
> My guess is that you're interested on option B here to be coherent with other
> Chromebooks.

You could evaluate either way. One of the reasons to chose the interpolated-steps
for veyron was that some veyron panels require a minimum backlight level to
work properly. This can be easily configured with the linear curve, but
currently not with the non-linear one. Another reason was to keep the backlight
curve of a released device unaltered, though I think that's not strictly
required as long as the behavior with the non-linear curve is close/good enough.

Chrome OS user space supports both types of curves, for a new device I would
probably start with a non-linear curve, and only change that if the resulting
behavior doesn't meet expectations for some reason.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel