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 <[email protected]>
---
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 = <®_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
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 <[email protected]>
> ---
> 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 = <®_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 |
Hi Marco,
On Fri, 27 Sep 2019 11:47:21 +0200
Marco Felsch <[email protected]> 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
Hi Andreas,
On 19-09-27 21:08, Andreas Kemnade wrote:
> Hi Marco,
>
> On Fri, 27 Sep 2019 11:47:21 +0200
> Marco Felsch <[email protected]> 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 |
On Mon, 30 Sep 2019 10:27:15 +0200
Marco Felsch <[email protected]> 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