[PATCH 0/3] dts: ARM: add Kobo Clara HD eBook reader

[PATCH 0/3] dts: ARM: add Kobo Clara HD eBook reader

[Andreas Kemnade]

This adds a device tree for the Kobo Clara HD eBook reader.
Name on mainboard is: 37NB-E60K00+4A4
Serials start with: E60K02 (a number also seen in
vendor kernel sources)
These boards are also found in the Tolino Shine 3 reader
but equipped with a i.MX6SL processor. Support for that
device is planned to be added in a later patch series.
To prepare for that the device tree is split up into
a board file containing SoC-independent stuff and a
file containing the SoC-dependent stuff.

Work is based on code from the vendor kernel at
https://github.com/kobolabs/Kobo-Reader/blob/master/hw/imx6sll-clara/kernel.tar.bz2
but things need to be heavily reworked due to
incompatible bindings and to prepare for Tolino Shine 3

Andreas Kemnade (3):
  ARM: dts: add Netronix E60K02 board common file
  dt-bindings: arm: fsl: add compatible string for Kobo Clara HD
  ARM: dts: imx: add devicetree for Kobo Clara HD

 .../devicetree/bindings/arm/fsl.yaml          |   1 +
 arch/arm/boot/dts/Makefile                    |   3 +-
 arch/arm/boot/dts/e60k02.dtsi                 | 339 ++++++++++++++++++
 arch/arm/boot/dts/imx6sll-kobo-clarahd.dts    | 275 ++++++++++++++
 4 files changed, 617 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/boot/dts/e60k02.dtsi
 create mode 100644 arch/arm/boot/dts/imx6sll-kobo-clarahd.dts

-- 
2.20.1

[PATCH 1/3] ARM: dts: add Netronix E60K02 board common file

[Andreas Kemnade]

The Netronix board E60K02 can be found some several Ebook-Readers,
at least the Kobo Clara HD and the Tolino Shine 3. The board
is equipped with different SoCs.

For now the following peripherals are included:
- LED
- Power Key
- Cover (gpio via hall sensor)
- RC5T619 PMIC (the kernel misses support for rtc and charger
  subdevices).
- Backlight via lm3630a
- Wifi sdio chip detection (mmc-powerseq and stuff)

It is based on vendor kernel but heavily reworked due to many
changed bindings.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
backligt dependencies:
module autoloading:
https://patchwork.kernel.org/patch/11139987/ 
enable-gpios property:
https://patchwork.kernel.org/patch/11143795/

arch/arm/boot/dts/e60k02.dtsi | 339 ++++++++++++++++++++++++++++++++++
 1 file changed, 339 insertions(+)
 create mode 100644 arch/arm/boot/dts/e60k02.dtsi

diff --git a/arch/arm/boot/dts/e60k02.dtsi b/arch/arm/boot/dts/e60k02.dtsi
new file mode 100644
index 000000000000..c4fa8e314e2e
--- /dev/null
+++ b/arch/arm/boot/dts/e60k02.dtsi
@@ -0,0 +1,339 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright 2019 Andreas Kemnade
+ * based on works
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ * and
+ * Copyright (C) 2014 Ricoh Electronic Devices Co., Ltd
+ *
+ * Netronix E60K02 board common.
+ * This board is equipped with different SoCs and
+ * found in ebook-readers like the Kobo Clara HD (with i.MX6SLL) and
+ * the Tolino Shine 3 (with i.MX6SL)
+ */
+
+/ {
+
+	memory {
+		reg = <0x80000000 0x80000000>;
+	};
+
+	chosen {
+		stdout-path = &uart1;
+	};
+
+	wifi_pwrseq: wifi_pwrseq {
+		compatible = "mmc-pwrseq-simple";
+		post-power-on-delay-ms = <20>;
+		reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;
+	};
+
+	regulators {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		reg_sd3_vmmc: wifi_regulator {
+			compatible = "regulator-fixed";
+			regulator-name = "SD3_SPWR";
+			regulator-min-microvolt = <3000000>;
+			regulator-max-microvolt = <3000000>;
+
+			gpio = <&gpio4 29 GPIO_ACTIVE_HIGH>;
+			enable-active-high;
+
+		};
+	};
+
+	leds {
+		compatible = "gpio-leds";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_led>;
+
+		GLED {
+			gpios = <&gpio5 7 GPIO_ACTIVE_LOW>;
+			linux,default-trigger = "timer";
+		};
+	};
+
+	gpio-keys {
+		compatible = "gpio-keys";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_gpio_keys>;
+		power {
+			label = "Power";
+			gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
+			linux,code = <KEY_POWER>;
+			gpio-key,wakeup;
+		};
+		cover {
+			label = "Cover";
+			gpios = <&gpio5 12 GPIO_ACTIVE_LOW>;
+			linux,code = <SW_LID>;
+			linux,input-type = <0x05>;    /* EV_SW */
+			gpio-key,wakeup;
+		};
+	};
+
+};
+
+
+
+&audmux {
+	pinctrl-names = "default";
+	status = "disabled";
+};
+
+&snvs_rtc {
+	status = "disabled";
+};
+
+&i2c1 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default","sleep";
+	pinctrl-0 = <&pinctrl_i2c1 &pinctrl_lm3630a_bl_gpio>;
+	pinctrl-1 = <&pinctrl_i2c1_sleep>;
+	status = "okay";
+
+	lm3630a: lm3630a-i2c@36 {
+		reg = <0x36>;
+		status = "ok";
+
+		compatible = "ti,lm3630a";
+		enable-gpios = <&gpio2 10 0>;
+
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		led@0 {
+			reg = <0>;
+			led-sources = <0>;
+			label = "backlight_warm";
+			default-brightness = <0>;
+			max-brightness = <255>;
+		};
+
+		led@1 {
+			reg = <1>;
+			led-sources = <1>;
+			label = "backlight_cold";
+			default-brightness = <0>;
+			max-brightness = <255>;
+		};
+
+	};
+};
+
+&i2c3 {
+	clock-frequency = <100000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_i2c3 &pinctrl_ricoh_gpio>;
+	status = "okay";
+
+	ricoh619: ricoh619-i2c@32 {
+		compatible = "ricoh,rc5t619";
+		reg = <0x32>;
+
+		system-power-controller;
+		gpios = <&gpio5 11 GPIO_ACTIVE_LOW>;
+		gpio_chg = <&gpio5 15 GPIO_ACTIVE_LOW>;
+		gpio_bat_low = <&gpio3 28 GPIO_ACTIVE_LOW>;
+	};
+
+};
+
+&ricoh619
+{
+	compatible = "ricoh,rc5t619";
+
+	regulators {
+		ricoh619_dcdc1_reg: DCDC1 {
+			regulator-name = "DCDC1";
+			regulator-min-microvolt = <300000>;
+			regulator-max-microvolt = <1875000>;
+			regulator-always-on;
+			regulator-boot-on;
+			regulator-state-mem {
+				regulator-on-in-suspend;
+				regulator-suspend-microvolt = <900000>;
+			};
+		};
+
+		/* Core3_3V3 */
+		ricoh619_dcdc2_reg: DCDC2 {
+			regulator-name = "DCDC2";
+			regulator-always-on;
+			regulator-boot-on;
+			regulator-state-mem {
+				regulator-on-in-suspend;
+				regulator-suspend-microvolt = <3300000>;
+			};
+		};
+
+		ricoh619_dcdc3_reg: DCDC3 {
+			regulator-name = "DCDC3";
+			regulator-min-microvolt = <300000>;
+			regulator-max-microvolt = <1875000>;
+			regulator-always-on;//
+			regulator-boot-on;
+			regulator-state-mem {
+				regulator-on-in-suspend;
+				regulator-suspend-microvolt = <1140000>;
+			};
+		};
+
+		/* Core4_1V2 */
+		ricoh619_dcdc4_reg: DCDC4 {
+			regulator-name = "DCDC4";
+			regulator-min-microvolt = <1200000>;
+			regulator-max-microvolt = <1200000>;
+			regulator-always-on;
+			regulator-boot-on;
+			regulator-state-mem {
+				regulator-on-in-suspend;
+				regulator-suspend-microvolt = <1140000>;
+			};
+		};
+
+		/* Core4_1V8 */
+		ricoh619_dcdc5_reg: DCDC5 {
+			regulator-name = "DCDC5";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-always-on;
+			regulator-boot-on;
+			regulator-state-mem {
+				regulator-on-in-suspend;
+				regulator-suspend-microvolt = <1700000>;
+			};
+		};
+
+		/* IR_3V3 */
+		ricoh619_ldo1_reg: LDO1  {
+			regulator-name = "LDO1";
+			//regulator-always-on;
+			regulator-boot-on;
+		};
+
+		/* Core1_3V3 */
+		ricoh619_ldo2_reg: LDO2  {
+			regulator-name = "LDO2";
+			regulator-always-on;
+			regulator-boot-on;
+			regulator-state-mem {
+				regulator-on-in-suspend;
+				regulator-suspend-microvolt = <3000000>;
+			};
+		};
+
+		/* Core5_1V2 */
+		ricoh619_ldo3_reg: LDO3  {
+			regulator-name = "LDO3";
+			regulator-always-on;
+			regulator-boot-on;
+		};
+
+		ricoh619_ldo4_reg: LDO4 {
+			regulator-name = "LDO4";
+			regulator-boot-on;
+		};
+
+		/* SPD_3V3 */
+		ricoh619_ldo5_reg: LDO5 {
+			regulator-name = "LDO5";
+			regulator-always-on;
+			regulator-boot-on;
+		};
+
+		/* DDR_0V6 */
+		ricoh619_ldo6_reg: LDO6 {
+			regulator-name = "LDO6";
+			regulator-always-on;
+			regulator-boot-on;
+		};
+
+		/* VDD_PWM */
+		ricoh619_ldo7_reg: LDO7 {
+			regulator-name = "LDO7";
+			regulator-always-on;
+			regulator-boot-on;
+		};
+
+		/* ldo_1v8 */
+		ricoh619_ldo8_reg: LDO8 {
+			regulator-name = "LDO8";
+			regulator-min-microvolt = <1800000>;
+			regulator-max-microvolt = <1800000>;
+			regulator-always-on;
+			regulator-boot-on;
+		};
+
+		ricoh619_ldo9_reg: LDO9 {
+			regulator-name = "LDO9";
+			regulator-boot-on;
+		};
+
+		ricoh619_ldo10_reg: LDO10 {
+			regulator-name = "LDO10";
+			regulator-boot-on;
+		};
+
+		ricoh619_ldortc1_reg: LDORTC1  {
+			regulator-name = "LDORTC1";
+			regulator-boot-on;
+		};
+
+		ricoh619_ldortc2_reg: LDORTC2 {
+			regulator-name = "LDORTC2";
+			regulator-boot-on;
+		};
+	};
+};
+
+&uart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_uart1>;
+	status = "okay";
+};
+
+&usdhc1 {
+	status = "disabled";
+};
+
+&usdhc2 {
+	pinctrl-names = "default", "state_100mhz", "state_200mhz","sleep";
+	pinctrl-0 = <&pinctrl_usdhc2>;
+	pinctrl-1 = <&pinctrl_usdhc2_100mhz>;
+	pinctrl-2 = <&pinctrl_usdhc2_200mhz>;
+	pinctrl-3 = <&pinctrl_usdhc2_sleep>;
+	non-removable;
+	status = "okay";
+};
+
+&usdhc3 {
+	pinctrl-names = "default", "state_100mhz", "state_200mhz","sleep";
+	pinctrl-0 = <&pinctrl_usdhc3>, <&pinctrl_usdhc3_pwr>;
+	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
+	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
+	pinctrl-3 = <&pinctrl_usdhc3_sleep>, <&pinctrl_usdhc3_pwr>;
+	vmmc-supply = <&reg_sd3_vmmc>;
+	mmc-pwrseq = <&wifi_pwrseq>;
+	cap-power-off-card;
+	non-removable;
+	status = "okay";
+};
+
+&usbotg1 {
+	pinctrl-names = "default";
+	disable-over-current;
+	srp-disable;
+	hnp-disable;
+	adp-disable;
+	status = "okay";
+};
+
+
+&ssi2 {
+	status = "disabled";
+};
+
-- 
2.20.1

[PATCH 2/3] dt-bindings: arm: fsl: add compatible string for Kobo Clara HD

[Andreas Kemnade]

This adds a compatible string fro the Kobo Clara HD eBook reader.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 Documentation/devicetree/bindings/arm/fsl.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
index 7294ac36f4c0..afa3bfeca0c0 100644
--- a/Documentation/devicetree/bindings/arm/fsl.yaml
+++ b/Documentation/devicetree/bindings/arm/fsl.yaml
@@ -148,6 +148,7 @@ properties:
         items:
           - enum:
               - fsl,imx6sll-evk
+              - kobo,clarahd
           - const: fsl,imx6sll
 
       - description: i.MX6SX based Boards
-- 
2.20.1

[PATCH 3/3] ARM: dts: imx: add devicetree for Kobo Clara HD

[Andreas Kemnade]

This adds a devicetree for the Kobo Clara HD Ebook reader. It
is on based on boards called "e60k02". It is equipped with an
imx6sll SoC.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 arch/arm/boot/dts/Makefile                 |   3 +-
 arch/arm/boot/dts/imx6sll-kobo-clarahd.dts | 275 +++++++++++++++++++++
 2 files changed, 277 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/boot/dts/imx6sll-kobo-clarahd.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 9159fa2cea90..a8a235c74c37 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -551,7 +551,8 @@ dtb-$(CONFIG_SOC_IMX6SL) += \
 	imx6sl-evk.dtb \
 	imx6sl-warp.dtb
 dtb-$(CONFIG_SOC_IMX6SLL) += \
-	imx6sll-evk.dtb
+	imx6sll-evk.dtb \
+	imx6sll-kobo-clarahd.dtb
 dtb-$(CONFIG_SOC_IMX6SX) += \
 	imx6sx-nitrogen6sx.dtb \
 	imx6sx-sabreauto.dtb \
diff --git a/arch/arm/boot/dts/imx6sll-kobo-clarahd.dts b/arch/arm/boot/dts/imx6sll-kobo-clarahd.dts
new file mode 100644
index 000000000000..f5ca44171d7c
--- /dev/null
+++ b/arch/arm/boot/dts/imx6sll-kobo-clarahd.dts
@@ -0,0 +1,275 @@
+// SPDX-License-Identifier: (GPL-2.0)
+/*
+ * Device tree for the Kobo Clara HD ebook reader
+ *
+ * Name on mainboard is: 37NB-E60K00+4A4
+ * Serials start with: E60K02 (a number also seen in
+ * vendor kernel sources)
+ *
+ * This mainboard seems to be equipped with different SoCs.
+ * In the Kobo Clara HD ebook reader it is an i.MX6SLL
+ *
+ * Copyright 2019 Andreas Kemnade
+ * based on works
+ * Copyright 2016 Freescale Semiconductor, Inc.
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/input/input.h>
+#include <dt-bindings/gpio/gpio.h>
+#include "imx6sll.dtsi"
+#include "e60k02.dtsi"
+
+/ {
+	model = "Kobo Clara HD";
+	compatible = "kobo,clarahd", "fsl,imx6sll";
+};
+
+&clks {
+	assigned-clocks = <&clks IMX6SLL_CLK_PLL4_AUDIO_DIV>;
+	assigned-clock-rates = <393216000>;
+};
+
+&cpu0 {
+	arm-supply = <&ricoh619_dcdc3_reg>;
+	soc-supply = <&ricoh619_dcdc1_reg>;
+};
+
+&iomuxc {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_hog>;
+
+	imx6sll-lpddr3-arm2 {
+		pinctrl_hog: hoggrp {
+			fsl,pins = <
+				MX6SLL_PAD_LCD_DATA00__GPIO2_IO20	0x79
+				MX6SLL_PAD_LCD_DATA01__GPIO2_IO21	0x79
+				MX6SLL_PAD_LCD_DATA02__GPIO2_IO22	0x79
+				MX6SLL_PAD_LCD_DATA03__GPIO2_IO23	0x79
+				MX6SLL_PAD_LCD_DATA04__GPIO2_IO24	0x79
+				MX6SLL_PAD_LCD_DATA05__GPIO2_IO25	0x79
+				MX6SLL_PAD_LCD_DATA06__GPIO2_IO26	0x79
+				MX6SLL_PAD_LCD_DATA07__GPIO2_IO27	0x79
+				MX6SLL_PAD_LCD_DATA08__GPIO2_IO28	0x79
+				MX6SLL_PAD_LCD_DATA09__GPIO2_IO29	0x79
+				MX6SLL_PAD_LCD_DATA10__GPIO2_IO30	0x79
+				MX6SLL_PAD_LCD_DATA11__GPIO2_IO31	0x79
+				MX6SLL_PAD_LCD_DATA12__GPIO3_IO00	0x79
+				MX6SLL_PAD_LCD_DATA13__GPIO3_IO01	0x79
+				MX6SLL_PAD_LCD_DATA14__GPIO3_IO02	0x79
+				MX6SLL_PAD_LCD_DATA15__GPIO3_IO03	0x79
+				MX6SLL_PAD_LCD_DATA16__GPIO3_IO04	0x79
+				MX6SLL_PAD_LCD_DATA17__GPIO3_IO05	0x79
+				MX6SLL_PAD_LCD_DATA18__GPIO3_IO06	0x79
+				MX6SLL_PAD_LCD_DATA19__GPIO3_IO07	0x79
+				MX6SLL_PAD_LCD_DATA20__GPIO3_IO08	0x79
+				MX6SLL_PAD_LCD_DATA21__GPIO3_IO09	0x79
+				MX6SLL_PAD_LCD_DATA22__GPIO3_IO10	0x79
+				MX6SLL_PAD_LCD_DATA23__GPIO3_IO11	0x79
+				MX6SLL_PAD_LCD_CLK__GPIO2_IO15		0x79
+				MX6SLL_PAD_LCD_ENABLE__GPIO2_IO16	0x79
+				MX6SLL_PAD_LCD_HSYNC__GPIO2_IO17	0x79
+				MX6SLL_PAD_LCD_VSYNC__GPIO2_IO18	0x79
+				MX6SLL_PAD_LCD_RESET__GPIO2_IO19	0x79
+				MX6SLL_PAD_KEY_COL3__GPIO3_IO30		0x79
+				MX6SLL_PAD_KEY_ROW7__GPIO4_IO07		0x79
+				MX6SLL_PAD_ECSPI2_MOSI__GPIO4_IO13	0x79
+				MX6SLL_PAD_KEY_COL5__GPIO4_IO02		0x79
+				MX6SLL_PAD_KEY_ROW6__GPIO4_IO05		0x79
+			>;
+		};
+
+		pinctrl_usdhc3_pwr: usdhc3_pwr_grp {
+			fsl,pins = <
+				MX6SLL_PAD_SD2_DATA7__GPIO5_IO00	0x10059		/* WIFI_RST */
+				MX6SLL_PAD_SD2_DATA5__GPIO4_IO31	0x10059		/* WIFI_INT */
+				MX6SLL_PAD_SD2_DATA6__GPIO4_IO29	0x10059		/* WIFI_3V3_ON */
+			>;
+		};
+
+		pinctrl_audmux3: audmux3grp {
+			fsl,pins = <
+				MX6SLL_PAD_AUD_TXC__AUD3_TXC		0x4130b0
+				MX6SLL_PAD_AUD_TXFS__AUD3_TXFS		0x4130b0
+				MX6SLL_PAD_AUD_TXD__AUD3_TXD		0x4110b0
+				MX6SLL_PAD_AUD_RXD__AUD3_RXD		0x4130b0
+				MX6SLL_PAD_AUD_MCLK__AUDIO_CLK_OUT	0x4130b0
+			>;
+		};
+
+		pinctrl_led: ledgrp {
+			fsl,pins = <
+				MX6SLL_PAD_SD1_DATA6__GPIO5_IO07 0x17059
+			>;
+		};
+
+		pinctrl_uart1: uart1grp {
+			fsl,pins = <
+				MX6SLL_PAD_UART1_TXD__UART1_DCE_TX 0x1b0b1
+				MX6SLL_PAD_UART1_RXD__UART1_DCE_RX 0x1b0b1
+			>;
+		};
+
+		pinctrl_usdhc2: usdhc2grp {
+			fsl,pins = <
+				MX6SLL_PAD_SD2_CMD__SD2_CMD		0x17059
+				MX6SLL_PAD_SD2_CLK__SD2_CLK		0x13059
+				MX6SLL_PAD_SD2_DATA0__SD2_DATA0		0x17059
+				MX6SLL_PAD_SD2_DATA1__SD2_DATA1		0x17059
+				MX6SLL_PAD_SD2_DATA2__SD2_DATA2		0x17059
+				MX6SLL_PAD_SD2_DATA3__SD2_DATA3		0x17059
+			>;
+		};
+
+		pinctrl_usdhc2_100mhz: usdhc2grp_100mhz {
+			fsl,pins = <
+				MX6SLL_PAD_SD2_CMD__SD2_CMD		0x170b9
+				MX6SLL_PAD_SD2_CLK__SD2_CLK		0x130b9
+				MX6SLL_PAD_SD2_DATA0__SD2_DATA0		0x170b9
+				MX6SLL_PAD_SD2_DATA1__SD2_DATA1		0x170b9
+				MX6SLL_PAD_SD2_DATA2__SD2_DATA2		0x170b9
+				MX6SLL_PAD_SD2_DATA3__SD2_DATA3		0x170b9
+			>;
+		};
+
+		pinctrl_usdhc2_200mhz: usdhc2grp_200mhz {
+			fsl,pins = <
+				MX6SLL_PAD_SD2_CMD__SD2_CMD		0x170f9
+				MX6SLL_PAD_SD2_CLK__SD2_CLK		0x130f9
+				MX6SLL_PAD_SD2_DATA0__SD2_DATA0		0x170f9
+				MX6SLL_PAD_SD2_DATA1__SD2_DATA1		0x170f9
+				MX6SLL_PAD_SD2_DATA2__SD2_DATA2		0x170f9
+				MX6SLL_PAD_SD2_DATA3__SD2_DATA3		0x170f9
+			>;
+		};
+
+		pinctrl_usdhc2_sleep: usdhc2grp_sleep {
+			fsl,pins = <
+				MX6SLL_PAD_SD2_CMD__GPIO5_IO04		0x100f9
+				MX6SLL_PAD_SD2_CLK__GPIO5_IO05		0x100f9
+				MX6SLL_PAD_SD2_DATA0__GPIO5_IO01	0x100f9
+				MX6SLL_PAD_SD2_DATA1__GPIO4_IO30	0x100f9
+				MX6SLL_PAD_SD2_DATA2__GPIO5_IO03	0x100f9
+				MX6SLL_PAD_SD2_DATA3__GPIO4_IO28	0x100f9
+			>;
+		};
+
+		pinctrl_usdhc3: usdhc3grp {
+			fsl,pins = <
+				MX6SLL_PAD_SD3_CMD__SD3_CMD	0x11059
+				MX6SLL_PAD_SD3_CLK__SD3_CLK	0x11059
+				MX6SLL_PAD_SD3_DATA0__SD3_DATA0	0x11059
+				MX6SLL_PAD_SD3_DATA1__SD3_DATA1	0x11059
+				MX6SLL_PAD_SD3_DATA2__SD3_DATA2	0x11059
+				MX6SLL_PAD_SD3_DATA3__SD3_DATA3	0x11059
+			>;
+		};
+
+		pinctrl_usdhc3_100mhz: usdhc3grp_100mhz {
+			fsl,pins = <
+				MX6SLL_PAD_SD3_CMD__SD3_CMD	0x170b9
+				MX6SLL_PAD_SD3_CLK__SD3_CLK	0x170b9
+				MX6SLL_PAD_SD3_DATA0__SD3_DATA0	0x170b9
+				MX6SLL_PAD_SD3_DATA1__SD3_DATA1	0x170b9
+				MX6SLL_PAD_SD3_DATA2__SD3_DATA2	0x170b9
+				MX6SLL_PAD_SD3_DATA3__SD3_DATA3	0x170b9
+			>;
+		};
+
+		pinctrl_usdhc3_200mhz: usdhc3grp_200mhz {
+			fsl,pins = <
+				MX6SLL_PAD_SD3_CMD__SD3_CMD	0x170f9
+				MX6SLL_PAD_SD3_CLK__SD3_CLK	0x170f9
+				MX6SLL_PAD_SD3_DATA0__SD3_DATA0	0x170f9
+				MX6SLL_PAD_SD3_DATA1__SD3_DATA1	0x170f9
+				MX6SLL_PAD_SD3_DATA2__SD3_DATA2	0x170f9
+				MX6SLL_PAD_SD3_DATA3__SD3_DATA3	0x170f9
+			>;
+		};
+
+		pinctrl_usdhc3_sleep: usdhc3grp_sleep {
+			fsl,pins = <
+				MX6SLL_PAD_SD3_CMD__GPIO5_IO21	0x100c1
+				MX6SLL_PAD_SD3_CLK__GPIO5_IO18	0x100c1
+				MX6SLL_PAD_SD3_DATA0__GPIO5_IO19	0x100c1
+				MX6SLL_PAD_SD3_DATA1__GPIO5_IO20	0x100c1
+				MX6SLL_PAD_SD3_DATA2__GPIO5_IO16	0x100c1
+				MX6SLL_PAD_SD3_DATA3__GPIO5_IO17	0x100c1
+			>;
+		};
+
+		pinctrl_usbotg1: usbotg1grp {
+			fsl,pins = <
+				MX6SLL_PAD_EPDC_PWR_COM__USB_OTG1_ID 0x17059
+			>;
+		};
+
+		pinctrl_i2c1: i2c1grp {
+			fsl,pins = <
+				MX6SLL_PAD_I2C1_SCL__I2C1_SCL	 0x4001f8b1
+				MX6SLL_PAD_I2C1_SDA__I2C1_SDA	 0x4001f8b1
+			>;
+		};
+
+		pinctrl_i2c1_sleep: i2c1grp_sleep {
+			fsl,pins = <
+				MX6SLL_PAD_I2C1_SCL__I2C1_SCL	 0x400108b1
+				MX6SLL_PAD_I2C1_SDA__I2C1_SDA	 0x400108b1
+			>;
+		};
+
+		pinctrl_i2c2: i2c2grp {
+			fsl,pins = <
+				MX6SLL_PAD_I2C2_SCL__I2C2_SCL	 0x4001f8b1
+				MX6SLL_PAD_I2C2_SDA__I2C2_SDA	 0x4001f8b1
+			>;
+		};
+
+		pinctrl_i2c2_sleep: i2c2grp_sleep {
+			fsl,pins = <
+				MX6SLL_PAD_I2C2_SCL__I2C2_SCL	 0x400108b1
+				MX6SLL_PAD_I2C2_SDA__I2C2_SDA	 0x400108b1
+			>;
+		};
+
+		pinctrl_i2c3: i2c3grp {
+			fsl,pins = <
+				MX6SLL_PAD_REF_CLK_24M__I2C3_SCL  0x4001f8b1
+				MX6SLL_PAD_REF_CLK_32K__I2C3_SDA  0x4001f8b1
+			>;
+		};
+
+		pinctrl_ecspi1: ecspi1grp {
+			fsl,pins = <
+				MX6SLL_PAD_ECSPI1_MISO__ECSPI1_MISO	 0x100b1
+				MX6SLL_PAD_ECSPI1_MOSI__ECSPI1_MOSI	 0x100b1
+				MX6SLL_PAD_ECSPI1_SCLK__ECSPI1_SCLK	 0x100b1
+				MX6SLL_PAD_ECSPI1_SS0__GPIO4_IO11	 0x100b1
+			>;
+		};
+
+		pinctrl_lm3630a_bl_gpio: lm3630a_bl_gpio_grp {
+			fsl,pins = <
+				MX6SLL_PAD_EPDC_PWR_CTRL3__GPIO2_IO10	0x10059 /* HWEN */
+			>;
+		};
+
+		pinctrl_ricoh_gpio: ricoh_gpio_grp {
+			fsl,pins = <
+				MX6SLL_PAD_SD1_CLK__GPIO5_IO15	0x1b8b1 /* ricoh619 chg */
+				MX6SLL_PAD_SD1_DATA0__GPIO5_IO11 0x1b8b1 /* ricoh619 irq */
+				MX6SLL_PAD_KEY_COL2__GPIO3_IO28	0x1b8b1 /* ricoh619 bat_low_int */
+			>;
+		};
+
+		pinctrl_gpio_keys: gpio_keys_grp {
+			fsl,pins = <
+				MX6SLL_PAD_SD1_DATA1__GPIO5_IO08	0x17059	/* PWR_SW */
+				MX6SLL_PAD_SD1_DATA4__GPIO5_IO12	0x17059	/* HALL_EN */
+			>;
+		};
+
+	};
+};
+
-- 
2.20.1

Re: [PATCH 2/3] dt-bindings: arm: fsl: add compatible string for Kobo Clara HD

[Marco Felsch]

Hi Andreas,

On 19-09-27 08:14, Andreas Kemnade wrote:
> This adds a compatible string fro the Kobo Clara HD eBook reader.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---

Just a nitpick, this should be patch number 1.

Regards,
  Marco

>  Documentation/devicetree/bindings/arm/fsl.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/fsl.yaml b/Documentation/devicetree/bindings/arm/fsl.yaml
> index 7294ac36f4c0..afa3bfeca0c0 100644
> --- a/Documentation/devicetree/bindings/arm/fsl.yaml
> +++ b/Documentation/devicetree/bindings/arm/fsl.yaml
> @@ -148,6 +148,7 @@ properties:
>          items:
>            - enum:
>                - fsl,imx6sll-evk
> +              - kobo,clarahd
>            - const: fsl,imx6sll
>  
>        - description: i.MX6SX based Boards
> -- 
> 2.20.1
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] ARM: dts: add Netronix E60K02 board common file

[Marco Felsch]

Hi Andreas,

thanks for the patch.

On 19-09-27 08:14, Andreas Kemnade wrote:
> The Netronix board E60K02 can be found some several Ebook-Readers,
> at least the Kobo Clara HD and the Tolino Shine 3. The board
> is equipped with different SoCs.
> 
> For now the following peripherals are included:
> - LED
> - Power Key
> - Cover (gpio via hall sensor)
> - RC5T619 PMIC (the kernel misses support for rtc and charger
>   subdevices).
> - Backlight via lm3630a
> - Wifi sdio chip detection (mmc-powerseq and stuff)
> 
> It is based on vendor kernel but heavily reworked due to many
> changed bindings.
> 
> Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
> ---
> backligt dependencies:
> module autoloading:
> https://patchwork.kernel.org/patch/11139987/ 
> enable-gpios property:
> https://patchwork.kernel.org/patch/11143795/
> 
> arch/arm/boot/dts/e60k02.dtsi | 339 ++++++++++++++++++++++++++++++++++
>  1 file changed, 339 insertions(+)
>  create mode 100644 arch/arm/boot/dts/e60k02.dtsi
> 
> diff --git a/arch/arm/boot/dts/e60k02.dtsi b/arch/arm/boot/dts/e60k02.dtsi
> new file mode 100644
> index 000000000000..c4fa8e314e2e
> --- /dev/null
> +++ b/arch/arm/boot/dts/e60k02.dtsi
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 Andreas Kemnade
> + * based on works
> + * Copyright 2016 Freescale Semiconductor, Inc.
> + * and
> + * Copyright (C) 2014 Ricoh Electronic Devices Co., Ltd
> + *
> + * Netronix E60K02 board common.
> + * This board is equipped with different SoCs and
> + * found in ebook-readers like the Kobo Clara HD (with i.MX6SLL) and
> + * the Tolino Shine 3 (with i.MX6SL)
> + */
> +
> +/ {
> +
> +	memory {
> +		reg = <0x80000000 0x80000000>;
> +	};
> +
> +	chosen {
> +		stdout-path = &uart1;
> +	};
> +
> +	wifi_pwrseq: wifi_pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		post-power-on-delay-ms = <20>;
> +		reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;

Can you add a pinctrl-entry here please? The general rule is to mux
things where you use it.

> +	};

Please reorder the whole dt alphabetical.

> +
> +	regulators {
> +		compatible = "simple-bus";
> +		#address-cells = <1>;
> +		#size-cells = <0>;

Drop the regultors { } container especially if we only have a single
regulator.

> +
> +		reg_sd3_vmmc: wifi_regulator {

Either use:
reg_sd3_vmmc: regulator-sd3-vmmc
or
reg_wifi: regulator-wifi

> +			compatible = "regulator-fixed";
> +			regulator-name = "SD3_SPWR";
> +			regulator-min-microvolt = <3000000>;
> +			regulator-max-microvolt = <3000000>;
> +
> +			gpio = <&gpio4 29 GPIO_ACTIVE_HIGH>;

Please add a pinctrl here to mux this gpio.

> +			enable-active-high;
> +
> +		};
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_led>;

Please move all muxing you made here into this file or add phandles so
the dts file need to add only the muxing stuff. This applies to all
pinctrl you made here.

> +
> +		GLED {
> +			gpios = <&gpio5 7 GPIO_ACTIVE_LOW>;
> +			linux,default-trigger = "timer";
> +		};
> +	};
> +
> +	gpio-keys {
> +		compatible = "gpio-keys";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&pinctrl_gpio_keys>;
> +		power {
> +			label = "Power";
> +			gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
> +			linux,code = <KEY_POWER>;

Add missing header: dt-bindings/input/input.h to use this.

> +			gpio-key,wakeup;
> +		};
> +		cover {
> +			label = "Cover";
> +			gpios = <&gpio5 12 GPIO_ACTIVE_LOW>;
> +			linux,code = <SW_LID>;
> +			linux,input-type = <0x05>;    /* EV_SW */

In the header above EV_SW is also specified so please use it here.

> +			gpio-key,wakeup;
> +		};
> +	};
> +
> +};
> +
> +
> +

Whitespaces

> +&audmux {
> +	pinctrl-names = "default";
> +	status = "disabled";

Why you mentioned a pinctrl-names here without the mux? Do we need the
status line here? The common case is that such devices are off by
default/the base dt.

> +};
> +
> +&snvs_rtc {
> +	status = "disabled";

Same applies here.

> +};
> +
> +&i2c1 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default","sleep";
> +	pinctrl-0 = <&pinctrl_i2c1 &pinctrl_lm3630a_bl_gpio>;

The &pinctrl_lm3630a_bl_gpio should be moved into the lm3630a node.

> +	pinctrl-1 = <&pinctrl_i2c1_sleep>;
> +	status = "okay";
> +
> +	lm3630a: lm3630a-i2c@36 {

please name it backlight@36

> +		reg = <0x36>;
> +		status = "ok";

status lines are always be the last and if it is okay you can drop it
because the default is okay.

> +
> +		compatible = "ti,lm3630a";
> +		enable-gpios = <&gpio2 10 0>;

Please use GPIO_ACTIVE_HIGH.

> +
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		led@0 {
> +			reg = <0>;
> +			led-sources = <0>;
> +			label = "backlight_warm";
> +			default-brightness = <0>;
> +			max-brightness = <255>;
> +		};
> +
> +		led@1 {
> +			reg = <1>;
> +			led-sources = <1>;
> +			label = "backlight_cold";
> +			default-brightness = <0>;
> +			max-brightness = <255>;
> +		};
> +
> +	};
> +};
> +
> +&i2c3 {
> +	clock-frequency = <100000>;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_i2c3 &pinctrl_ricoh_gpio>;

Device mux goes into the device.

> +	status = "okay";
> +
> +	ricoh619: ricoh619-i2c@32 {

Please name it pmic@32

> +		compatible = "ricoh,rc5t619";
> +		reg = <0x32>;
> +
> +		system-power-controller;
> +		gpios = <&gpio5 11 GPIO_ACTIVE_LOW>;
> +		gpio_chg = <&gpio5 15 GPIO_ACTIVE_LOW>;
> +		gpio_bat_low = <&gpio3 28 GPIO_ACTIVE_LOW>;
> +	};
> +
> +};
> +
> +&ricoh619
> +{

Nope. The whole bunch should be moved to the place above where add the
device.

> +	compatible = "ricoh,rc5t619";
> +
> +	regulators {
> +		ricoh619_dcdc1_reg: DCDC1 {

please drop the ricoh619_ prefix.

> +			regulator-name = "DCDC1";
> +			regulator-min-microvolt = <300000>;
> +			regulator-max-microvolt = <1875000>;
> +			regulator-always-on;
> +			regulator-boot-on;
> +			regulator-state-mem {
> +				regulator-on-in-suspend;
> +				regulator-suspend-microvolt = <900000>;

regulator-suspend-microvolt is deprecated. Please use
regulator-suspend-min-microvolt and regulator-suspend-max-microvolt.

> +			};
> +		};
> +
> +		/* Core3_3V3 */
> +		ricoh619_dcdc2_reg: DCDC2 {
> +			regulator-name = "DCDC2";
> +			regulator-always-on;
> +			regulator-boot-on;
> +			regulator-state-mem {
> +				regulator-on-in-suspend;
> +				regulator-suspend-microvolt = <3300000>;
> +			};
> +		};
> +
> +		ricoh619_dcdc3_reg: DCDC3 {
> +			regulator-name = "DCDC3";
> +			regulator-min-microvolt = <300000>;
> +			regulator-max-microvolt = <1875000>;
> +			regulator-always-on;//

Remove //

> +			regulator-boot-on;
> +			regulator-state-mem {
> +				regulator-on-in-suspend;
> +				regulator-suspend-microvolt = <1140000>;
> +			};
> +		};
> +
> +		/* Core4_1V2 */
> +		ricoh619_dcdc4_reg: DCDC4 {
> +			regulator-name = "DCDC4";
> +			regulator-min-microvolt = <1200000>;
> +			regulator-max-microvolt = <1200000>;
> +			regulator-always-on;
> +			regulator-boot-on;
> +			regulator-state-mem {
> +				regulator-on-in-suspend;
> +				regulator-suspend-microvolt = <1140000>;
> +			};
> +		};
> +
> +		/* Core4_1V8 */
> +		ricoh619_dcdc5_reg: DCDC5 {
> +			regulator-name = "DCDC5";
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			regulator-always-on;
> +			regulator-boot-on;
> +			regulator-state-mem {
> +				regulator-on-in-suspend;
> +				regulator-suspend-microvolt = <1700000>;
> +			};
> +		};
> +
> +		/* IR_3V3 */
> +		ricoh619_ldo1_reg: LDO1  {
> +			regulator-name = "LDO1";
> +			//regulator-always-on;
> +			regulator-boot-on;
> +		};
> +
> +		/* Core1_3V3 */
> +		ricoh619_ldo2_reg: LDO2  {
> +			regulator-name = "LDO2";
> +			regulator-always-on;
> +			regulator-boot-on;
> +			regulator-state-mem {
> +				regulator-on-in-suspend;
> +				regulator-suspend-microvolt = <3000000>;
> +			};
> +		};
> +
> +		/* Core5_1V2 */
> +		ricoh619_ldo3_reg: LDO3  {
> +			regulator-name = "LDO3";
> +			regulator-always-on;
> +			regulator-boot-on;
> +		};
> +
> +		ricoh619_ldo4_reg: LDO4 {
> +			regulator-name = "LDO4";
> +			regulator-boot-on;
> +		};
> +
> +		/* SPD_3V3 */
> +		ricoh619_ldo5_reg: LDO5 {
> +			regulator-name = "LDO5";
> +			regulator-always-on;
> +			regulator-boot-on;
> +		};
> +
> +		/* DDR_0V6 */
> +		ricoh619_ldo6_reg: LDO6 {
> +			regulator-name = "LDO6";
> +			regulator-always-on;
> +			regulator-boot-on;
> +		};
> +
> +		/* VDD_PWM */
> +		ricoh619_ldo7_reg: LDO7 {
> +			regulator-name = "LDO7";
> +			regulator-always-on;
> +			regulator-boot-on;
> +		};
> +
> +		/* ldo_1v8 */
> +		ricoh619_ldo8_reg: LDO8 {
> +			regulator-name = "LDO8";
> +			regulator-min-microvolt = <1800000>;
> +			regulator-max-microvolt = <1800000>;
> +			regulator-always-on;
> +			regulator-boot-on;
> +		};
> +
> +		ricoh619_ldo9_reg: LDO9 {
> +			regulator-name = "LDO9";
> +			regulator-boot-on;
> +		};
> +
> +		ricoh619_ldo10_reg: LDO10 {
> +			regulator-name = "LDO10";
> +			regulator-boot-on;
> +		};
> +
> +		ricoh619_ldortc1_reg: LDORTC1  {
> +			regulator-name = "LDORTC1";
> +			regulator-boot-on;
> +		};
> +
> +		ricoh619_ldortc2_reg: LDORTC2 {
> +			regulator-name = "LDORTC2";
> +			regulator-boot-on;
> +		};
> +	};
> +};
> +
> +&uart1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&pinctrl_uart1>;
> +	status = "okay";
> +};
> +
> +&usdhc1 {
> +	status = "disabled";
> +};

Can be dropped

> +
> +&usdhc2 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz","sleep";
> +	pinctrl-0 = <&pinctrl_usdhc2>;
> +	pinctrl-1 = <&pinctrl_usdhc2_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc2_200mhz>;
> +	pinctrl-3 = <&pinctrl_usdhc2_sleep>;
> +	non-removable;
> +	status = "okay";
> +};
> +
> +&usdhc3 {
> +	pinctrl-names = "default", "state_100mhz", "state_200mhz","sleep";
> +	pinctrl-0 = <&pinctrl_usdhc3>, <&pinctrl_usdhc3_pwr>;
> +	pinctrl-1 = <&pinctrl_usdhc3_100mhz>;
> +	pinctrl-2 = <&pinctrl_usdhc3_200mhz>;
> +	pinctrl-3 = <&pinctrl_usdhc3_sleep>, <&pinctrl_usdhc3_pwr>;
> +	vmmc-supply = <&reg_sd3_vmmc>;
> +	mmc-pwrseq = <&wifi_pwrseq>;
> +	cap-power-off-card;
> +	non-removable;
> +	status = "okay";
> +};
> +
> +&usbotg1 {
> +	pinctrl-names = "default";
> +	disable-over-current;
> +	srp-disable;
> +	hnp-disable;
> +	adp-disable;
> +	status = "okay";
> +};
> +
> +
> +&ssi2 {
> +	status = "disabled";
> +};

Can be dropped.


Thanks for your patch.

Regards,
  Marco

> +
> -- 
> 2.20.1
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] ARM: dts: add Netronix E60K02 board common file

[Andreas Kemnade]

Hi Marco,

On Fri, 27 Sep 2019 11:47:21 +0200
Marco Felsch <m.felsch@pengutronix.de> wrote:

> Hi Andreas,
> 
> thanks for the patch.
> 
thanks for the quick review. Most of your comments are clear.

[...]
> > +	wifi_pwrseq: wifi_pwrseq {
> > +		compatible = "mmc-pwrseq-simple";
> > +		post-power-on-delay-ms = <20>;
> > +		reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;  
> 
> Can you add a pinctrl-entry here please? The general rule is to mux
> things where you use it.
> 
yes, there are many places in my patch where they are added to some
parent devices.
I will fix that.
[...]
> > +	leds {
> > +		compatible = "gpio-leds";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_led>;  
> 
> Please move all muxing you made here into this file or add phandles so
> the dts file need to add only the muxing stuff. This applies to all
> pinctrl you made here.
> 
so you disagree with this pattern:
in .dtsi
 some_device {
   pinctrl-0 = <&pinctrl_some_device>;
 };

and in .dts (one I sent with this patch series and the tolino/mx6sl one
is not ready-cooked yet, will be part of a later series)
&iomuxc {
   pinctrl_some_device: some_devicegrp {
   	fsl,pins = <...>;
   };
};

?

> > +
> > +		GLED {
> > +			gpios = <&gpio5 7 GPIO_ACTIVE_LOW>;
> > +			linux,default-trigger = "timer";
> > +		};
> > +	};
> > +
> > +	gpio-keys {
> > +		compatible = "gpio-keys";
> > +		pinctrl-names = "default";
> > +		pinctrl-0 = <&pinctrl_gpio_keys>;
> > +		power {
> > +			label = "Power";
> > +			gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
> > +			linux,code = <KEY_POWER>;  
> 
> Add missing header: dt-bindings/input/input.h to use this.
> 
I am doing this in the .dts but it is probably better to do it here
because it is used here.

> > +			gpio-key,wakeup;
> > +		};
> > +		cover {
> > +			label = "Cover";
> > +			gpios = <&gpio5 12 GPIO_ACTIVE_LOW>;
> > +			linux,code = <SW_LID>;
> > +			linux,input-type = <0x05>;    /* EV_SW */  
> 
> In the header above EV_SW is also specified so please use it here.
> 
This pattern is in many files. I took one as an example. It seems that
50% of devicetree files have this pattern, the other 50% do have the
pattern you proposed (which I like more). So probably EV_SW was not
available in former times?

> > +			gpio-key,wakeup;
> > +		};
> > +	};
> > +
> > +};
> > +
> > +
> > +  
> 
> Whitespaces
> 
> > +&audmux {
> > +	pinctrl-names = "default";
> > +	status = "disabled";  
> 
> Why you mentioned a pinctrl-names here without the mux? Do we need the
> status line here? The common case is that such devices are off by
> default/the base dt.
> 
yes, that things can be removed.
> > +};
> > +
> > +&snvs_rtc {
> > +	status = "disabled";  
> 
> Same applies here.
> 

No, seems to be an exception, it does not have a status = "disabled" in
imx6sll.dtsi.

> > +};
> > +
> > +&i2c1 {
> > +	clock-frequency = <100000>;
> > +	pinctrl-names = "default","sleep";
> > +	pinctrl-0 = <&pinctrl_i2c1 &pinctrl_lm3630a_bl_gpio>;  
> 
> The &pinctrl_lm3630a_bl_gpio should be moved into the lm3630a node.
> 
> > +	pinctrl-1 = <&pinctrl_i2c1_sleep>;
> > +	status = "okay";
> > +
> > +	lm3630a: lm3630a-i2c@36 {  
> 
> please name it backlight@36
> 
> > +		reg = <0x36>;
> > +		status = "ok";  
> 
> status lines are always be the last and if it is okay you can drop it
> because the default is okay.
> 
well, I added it because the driver was not loaded but later I found out
that the real reason is that module aliases were broken and forgot to
remove that "ok".

Regards,
Andreas

Re: [PATCH 1/3] ARM: dts: add Netronix E60K02 board common file

[Marco Felsch]

Hi Andreas,

On 19-09-27 21:08, Andreas Kemnade wrote:
> Hi Marco,
> 
> On Fri, 27 Sep 2019 11:47:21 +0200
> Marco Felsch <m.felsch@pengutronix.de> wrote:
> 
> > Hi Andreas,
> > 
> > thanks for the patch.
> > 
> thanks for the quick review. Most of your comments are clear.
> 
> [...]
> > > +	wifi_pwrseq: wifi_pwrseq {
> > > +		compatible = "mmc-pwrseq-simple";
> > > +		post-power-on-delay-ms = <20>;
> > > +		reset-gpios = <&gpio5 0 GPIO_ACTIVE_LOW>;  
> > 
> > Can you add a pinctrl-entry here please? The general rule is to mux
> > things where you use it.
> > 
> yes, there are many places in my patch where they are added to some
> parent devices.
> I will fix that.
> [...]
> > > +	leds {
> > > +		compatible = "gpio-leds";
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pinctrl_led>;  
> > 
> > Please move all muxing you made here into this file or add phandles so
> > the dts file need to add only the muxing stuff. This applies to all
> > pinctrl you made here.
> > 
> so you disagree with this pattern:
> in .dtsi
>  some_device {
>    pinctrl-0 = <&pinctrl_some_device>;
>  };
> 
> and in .dts (one I sent with this patch series and the tolino/mx6sl one
> is not ready-cooked yet, will be part of a later series)
> &iomuxc {
>    pinctrl_some_device: some_devicegrp {
>    	fsl,pins = <...>;
>    };
> };
> 
> ?

Yes, because IMHO a dtsi is self contained as well as a dts. If it is
common for all boards you can move the muxing into the dtsi else it
should be done within the dts.

> > > +
> > > +		GLED {
> > > +			gpios = <&gpio5 7 GPIO_ACTIVE_LOW>;
> > > +			linux,default-trigger = "timer";
> > > +		};
> > > +	};
> > > +
> > > +	gpio-keys {
> > > +		compatible = "gpio-keys";
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&pinctrl_gpio_keys>;
> > > +		power {
> > > +			label = "Power";
> > > +			gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
> > > +			linux,code = <KEY_POWER>;  
> > 
> > Add missing header: dt-bindings/input/input.h to use this.
> > 
> I am doing this in the .dts but it is probably better to do it here
> because it is used here.
> 
> > > +			gpio-key,wakeup;
> > > +		};
> > > +		cover {
> > > +			label = "Cover";
> > > +			gpios = <&gpio5 12 GPIO_ACTIVE_LOW>;
> > > +			linux,code = <SW_LID>;
> > > +			linux,input-type = <0x05>;    /* EV_SW */  
> > 
> > In the header above EV_SW is also specified so please use it here.
> > 
> This pattern is in many files. I took one as an example. It seems that
> 50% of devicetree files have this pattern, the other 50% do have the
> pattern you proposed (which I like more). So probably EV_SW was not
> available in former times?

I don't know, checking the git history should bring the answer ;)
Anyway, if it is available we should use it.

> > > +			gpio-key,wakeup;
> > > +		};
> > > +	};
> > > +
> > > +};
> > > +
> > > +
> > > +  
> > 
> > Whitespaces
> > 
> > > +&audmux {
> > > +	pinctrl-names = "default";
> > > +	status = "disabled";  
> > 
> > Why you mentioned a pinctrl-names here without the mux? Do we need the
> > status line here? The common case is that such devices are off by
> > default/the base dt.
> > 
> yes, that things can be removed.
> > > +};
> > > +
> > > +&snvs_rtc {
> > > +	status = "disabled";  
> > 
> > Same applies here.
> > 
> 
> No, seems to be an exception, it does not have a status = "disabled" in
> imx6sll.dtsi.

Did you mean 6sll or 6ull?

Okay, is this baseboard only used with a 6ull?

Regards,
  Marco

> > > +};
> > > +
> > > +&i2c1 {
> > > +	clock-frequency = <100000>;
> > > +	pinctrl-names = "default","sleep";
> > > +	pinctrl-0 = <&pinctrl_i2c1 &pinctrl_lm3630a_bl_gpio>;  
> > 
> > The &pinctrl_lm3630a_bl_gpio should be moved into the lm3630a node.
> > 
> > > +	pinctrl-1 = <&pinctrl_i2c1_sleep>;
> > > +	status = "okay";
> > > +
> > > +	lm3630a: lm3630a-i2c@36 {  
> > 
> > please name it backlight@36
> > 
> > > +		reg = <0x36>;
> > > +		status = "ok";  
> > 
> > status lines are always be the last and if it is okay you can drop it
> > because the default is okay.
> > 
> well, I added it because the driver was not loaded but later I found out
> that the real reason is that module aliases were broken and forgot to
> remove that "ok".
> 
> Regards,
> Andreas
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/3] ARM: dts: add Netronix E60K02 board common file

[Andreas Kemnade]

On Mon, 30 Sep 2019 10:27:15 +0200
Marco Felsch <m.felsch@pengutronix.de> wrote:

[..]
> > so you disagree with this pattern:
> > in .dtsi
> >  some_device {
> >    pinctrl-0 = <&pinctrl_some_device>;
> >  };
> > 
> > and in .dts (one I sent with this patch series and the tolino/mx6sl one
> > is not ready-cooked yet, will be part of a later series)
> > &iomuxc {
> >    pinctrl_some_device: some_devicegrp {
> >    	fsl,pins = <...>;
> >    };
> > };
> > 
> > ?  
> 
> Yes, because IMHO a dtsi is self contained as well as a dts. If it is
> common for all boards you can move the muxing into the dtsi else it
> should be done within the dts.
> 
well, since imx6sll-pinfunc.h is different than imx6sl-pinfunc.h,
we agree that this belongs to the dts.

> > > > +&snvs_rtc {
> > > > +	status = "disabled";    
> > > 
> > > Same applies here.
> > >   
> > 
> > No, seems to be an exception, it does not have a status = "disabled" in
> > imx6sll.dtsi.  
> 
> Did you mean 6sll or 6ull?
> 
> Okay, is this baseboard only used with a 6ull?
> 

MCIMX6V7DVN10AB and MCIMX6L8DVN10AB
So it is 6sll and 6sl (6sl support will be added in a follow-up patch
series). 

I will send a v2 this evening, so we can all look at better-sorted
things.

Regards,
Andreas