Message-ID: <d8de7d2237a74388ac8c8152eded3477e7fd56a4.camel@paulk.fr>
Date:   Mon, 07 May 2018 22:15:46 +0200
From:   Paul Kocialkowski <contact@...lk.fr>
To:     Maxime Ripard <maxime.ripard@...tlin.com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        dri-devel@...ts.freedesktop.org, Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Chen-Yu Tsai <wens@...e.org>,
        Thierry Reding <thierry.reding@...il.com>,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v3 3/3] ARM: dts: sun7i: Add support for the Ainol AW1
 tablet

Hi,

Le lundi 07 mai 2018 à 09:19 +0200, Maxime Ripard a écrit :
> Hi,
> 
> On Sun, May 06, 2018 at 11:49:01PM +0200, Paul Kocialkowski wrote:
> > This adds support for the Ainol AW1, an A20-based 7" tablet from Ainol.
> > 
> > The following board-specific features are supported:
> > * LCD panel
> > * Backlight
> > * USB OTG
> > * Buttons
> > * Touchscreen (doesn't work without non-free firmware)
> > * Accelerometer
> > * Battery
> > 
> > The following are untested:
> > * Audio output
> > * Audio speakers
> > * USB via SPCI connector
> > 
> > The following are not supported:
> > * Wi-Fi
> > * Bluetooth
> > * NAND
> > * Audio via SPCI connector
> > * Audio via Bluetooth I2S
> > 
> > Signed-off-by: Paul Kocialkowski <contact@...lk.fr>
> > ---
> >  arch/arm/boot/dts/Makefile                |   1 +
> >  arch/arm/boot/dts/sun7i-a20-ainol-aw1.dts | 275 ++++++++++++++++++++++
> >  2 files changed, 276 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/sun7i-a20-ainol-aw1.dts
> > 
> > diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> > index 7e2424957809..4a80971f2bc9 100644
> > --- a/arch/arm/boot/dts/Makefile
> > +++ b/arch/arm/boot/dts/Makefile
> > @@ -946,6 +946,7 @@ dtb-$(CONFIG_MACH_SUN6I) += \
> >  	sun6i-a31s-sinovoip-bpi-m2.dtb \
> >  	sun6i-a31s-yones-toptech-bs1078-v2.dtb
> >  dtb-$(CONFIG_MACH_SUN7I) += \
> > +	sun7i-a20-ainol-aw1.dtb \
> >  	sun7i-a20-bananapi.dtb \
> >  	sun7i-a20-bananapi-m1-plus.dtb \
> >  	sun7i-a20-bananapro.dtb \
> > diff --git a/arch/arm/boot/dts/sun7i-a20-ainol-aw1.dts b/arch/arm/boot/dts/sun7i-a20-ainol-aw1.dts
> > new file mode 100644
> > index 000000000000..9a1d54a9f9a0
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/sun7i-a20-ainol-aw1.dts
> > @@ -0,0 +1,275 @@
> > +/*
> > + * Copyright (C) 2018 Paul Kocialkowski <contact@...lk.fr>
> > + *
> > + * SPDX-License-Identifier: GPL-2.0+
> 
> This should be your first line.

Alright then. For reference, I based this off sun8i-h3-libretech-all-h3-
cc.dts, that has the copyright first.

> Also, we usually license our DT under
> a dual license (GPL and MIT) so that other projects (like FreeBSD) can
> use them as well, instead of duplicating them. It would be great if
> you'd consider it.

Ah, I didn't think of BSD-licensed systems like FreeBSD off-hand.

Since they are apparently working on ARM support, it's probably worth
publishing this under a non-copyleft license.

> > + */
> > +
> > +/dts-v1/;
> > +#include "sun7i-a20.dtsi"
> > +#include "sunxi-common-regulators.dtsi"
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +#include <dt-bindings/input/input.h>
> > +#include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/pwm/pwm.h>
> > +
> > +/ {
> > +	model = "Ainol AW1";
> > +	compatible = "ainol,ainol-aw1", "allwinner,sun7i-a20";
> > +
> > +	aliases {
> > +		serial0 = &uart0;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	backlight: backlight {
> > +		compatible = "pwm-backlight";
> > +		pwms = <&pwm 0 50000 PWM_POLARITY_INVERTED>;
> > +		brightness-levels = <0 10 20 30 40 50 60 70 80 90 100>;
> 
> The increase in perceived brightness should be linear. Usually, for
> PWMs backed backlight, an exponential list is a much better
> approximation.

Thanks for the hint, it never occurred to me that pwm duty cycle was not
linear with brightness, but that makes sense. I'll give that a try (on
255 values instead of 10 to keep some level of precision in low
brightness). The way to go here is probably use a base-255 logarithm
such as: duty cycle = range * log(i+1)/log(255) with i the linear
brightness value (0 to 255) and range the amplitude of our values (that
gets divided by the maximum brightness, so it's really up to hardware
precision at this point). I'll go with a range of 255 as well.

> > +		default-brightness-level = <5>;
> > +		enable-gpios = <&pio 7 7 GPIO_ACTIVE_HIGH>; /* PH7 */
> > +	};
> > +
> > +	panel: panel {
> > +		compatible = "innolux,at070tn90";
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		power-supply = <&panel_power>;
> > +		backlight = <&backlight>;
> > +
> > +		port@0 {
> > +			reg = <0>;
> > +			#address-cells = <1>;
> > +			#size-cells = <0>;
> > +
> > +			panel_input: endpoint@0 {
> > +				reg = <0>;
> > +				remote-endpoint = <&tcon0_out_panel>;
> > +			};
> > +		};
> > +	};
> > +
> > +	panel_power: panel_power {
> > +		compatible = "regulator-fixed";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&panel_power_pin>;
> > +		regulator-name = "panel-power";
> > +		regulator-min-microvolt = <10400000>;
> > +		regulator-max-microvolt = <10400000>;
> > +		gpio = <&pio 7 8 GPIO_ACTIVE_HIGH>; /* PH8 */
> > +		enable-active-high;
> > +		regulator-boot-on;
> > +	};
> > +};
> > +
> > +&codec {
> > +	allwinner,pa-gpios = <&pio 7 15 GPIO_ACTIVE_HIGH>; /* PH15 */
> > +	status = "okay";
> > +};
> > +
> > +&cpu0 {
> > +	cpu-supply = <&reg_dcdc2>;
> > +};
> > +
> > +&de {
> > +	status = "okay";
> > +};
> > +
> > +&ehci0 {
> > +	status = "okay";
> > +};
> > +
> > +&ehci1 {
> > +	status = "okay";
> > +};
> > +
> > +&i2c0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c0_pins_a>;
> > +	status = "okay";
> > +
> > +	axp209: pmic@34 {
> > +		reg = <0x34>;
> > +		interrupt-parent = <&nmi_intc>;
> > +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > +	};
> > +};
> > +
> > +&i2c1 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c1_pins_a>;
> > +	status = "okay";
> > +
> > +	lis3dh: accelerometer@18 {
> > +		compatible = "st,lis3dh-accel";
> > +		reg = <0x18>;
> > +		vdd-supply = <&reg_vcc3v3>;
> > +		vddio-supply = <&reg_vcc3v3>;
> > +		st,drdy-int-pin = <1>;
> > +	};
> > +};
> > +
> > +&i2c2 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&i2c2_pins_a>;
> > +	status = "okay";
> > +	clock-frequency = <400000>;
> > +
> > +	gsl1680: touchscreen@40 {
> > +		compatible = "silead,gsl1680";
> > +		reg = <0x40>;
> > +		interrupt-parent = <&pio>;
> > +		interrupts = <7 21 IRQ_TYPE_EDGE_FALLING>; /* EINT21 (PH21) */
> > +		power-gpios = <&pio 7 20 GPIO_ACTIVE_HIGH>; /* PH20 */
> > +		firmware-name = "gsl1680-ainol-aw1.fw";
> > +		touchscreen-size-x = <480>;
> > +		touchscreen-size-y = <800>;
> > +		touchscreen-swapped-x-y;
> > +		touchscreen-inverted-y;
> > +		silead,max-fingers = <5>;
> > +	};
> > +};
> > +
> > +&lradc {
> > +	vref-supply = <&reg_ldo2>;
> > +	status = "okay";
> > +
> > +	button@571 {
> > +		label = "Volume Up";
> > +		linux,code = <KEY_VOLUMEUP>;
> > +		channel = <0>;
> > +		voltage = <571428>;
> > +	};
> > +
> > +	button@761 {
> > +		label = "Volume Down";
> > +		linux,code = <KEY_VOLUMEDOWN>;
> > +		channel = <0>;
> > +		voltage = <761904>;
> > +	};
> > +
> > +	button@952 {
> > +		label = "Home";
> > +		linux,code = <KEY_HOME>;
> > +		channel = <0>;
> > +		voltage = <952380>;
> > +	};
> > +};
> > +
> > +&mmc0 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&mmc0_pins_a>;
> > +	vmmc-supply = <&reg_vcc3v3>;
> 
> You have the regulators described in your DT, you'd better use them
> instead of the one coming from sunxi-common-regulators.dtsi.

Well, according to the reference A20 design, the mmc pins and the card
are powered by the 3.3V power rail, that comes from a regular step-down
regulator sourcing from IPSOUT, so I don't see what regulator I should
better use. Do you have a suggestion?

> > +	bus-width = <4>;
> > +	cd-gpios = <&pio 7 1 GPIO_ACTIVE_HIGH>; /* PH1 */
> > +	cd-inverted;
> > +	status = "okay";
> > +};
> > +
> > +&ohci0 {
> > +	status = "okay";
> > +};
> > +
> > +&ohci1 {
> > +	status = "okay";
> > +};
> > +
> > +&otg_sram {
> > +	status = "okay";
> > +};
> > +
> > +&pio {
> > +	panel_power_pin: panel_power_pin@0 {
> > +		pins = "PH8";
> > +		function = "gpio_out";
> > +	};
> > +};
> 
> You don't need that pinctrl node.

I'll get rid of it then. You mentioned that regulator-simple uses the
old GPIO API, so I assumed it meant that a pinctrl node is still needed.
For reference, it uses of_get_named_gpio (not the devm-managed fashion).

Cheers,

Paul

-- 
Developer of free digital technology and hardware support.

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists