On Sat, Aug 15, 2020 at 09:33:36PM +0200, Andreas Kemnade wrote: > This adds a devicetree for the Tolino Shine 2 HD Ebook reader. It is based > on boards marked with "37NB-E60QF0+4A2". It is equipped with an i.MX6SL > SoC. > > Expected to work: > - Buttons > - Wifi > - Touchscreen > - LED > - uSD > - USB > - RTC > > Not working due to missing drivers: > - Backlight (requires NTXEC driver) > - EPD > > Not working due to unknown reasons: > - deep sleep (echo standby >/sys/power/state works), > wakeup fails when imx_gpc_pre_suspend(true) was called. > > Signed-off-by: Andreas Kemnade <andreas@xxxxxxxxxxxx> > --- > Reason for RFC: The suspend trouble might be caused by bad devicetree. > But as the devicetree is already useful I decided to submit it. [...] > +++ b/arch/arm/boot/dts/imx6sl-tolino-shine2hd.dts > @@ -0,0 +1,582 @@ > +// SPDX-License-Identifier: (GPL-2.0) I don't think the parentheses are required when you don't have a logical operator (OR) in the SPDX expression. > +&i2c1 { > + pinctrl-names = "default","sleep"; > + pinctrl-0 = <&pinctrl_i2c1>; > + pinctrl-1 = <&pinctrl_i2c1_sleep>; > + status = "okay"; > + > + /* TODO: embedded controller at 0x43 (driver missing) */ Sorry for the delay, BTW. I'm still (slowly) working on v2. > + ricoh619: pmic@32 { > + compatible = "ricoh,rc5t619"; > + pinctrl-names = "default"; > + pinctrl-0 = <&pinctrl_ricoh_gpio>; > + reg = <0x32>; > + interrupt-parent = <&gpio5>; > + interrupts = <11 IRQ_TYPE_EDGE_FALLING>; > + system-power-controller; > + > + regulators { How did you derive the regulator voltages? > + pinctrl_hog: hoggrp { > + fsl,pins = < > + MX6SL_PAD_LCD_DAT0__GPIO2_IO20 0x79 > + MX6SL_PAD_LCD_DAT1__GPIO2_IO21 0x79 > + MX6SL_PAD_LCD_DAT2__GPIO2_IO22 0x79 > + MX6SL_PAD_LCD_DAT3__GPIO2_IO23 0x79 > + MX6SL_PAD_LCD_DAT4__GPIO2_IO24 0x79 > + MX6SL_PAD_LCD_DAT5__GPIO2_IO25 0x79 > + MX6SL_PAD_LCD_DAT6__GPIO2_IO26 0x79 > + MX6SL_PAD_LCD_DAT7__GPIO2_IO27 0x79 > + MX6SL_PAD_LCD_DAT8__GPIO2_IO28 0x79 > + MX6SL_PAD_LCD_DAT9__GPIO2_IO29 0x79 > + MX6SL_PAD_LCD_DAT10__GPIO2_IO30 0x79 > + MX6SL_PAD_LCD_DAT11__GPIO2_IO31 0x79 > + MX6SL_PAD_LCD_DAT12__GPIO3_IO00 0x79 > + MX6SL_PAD_LCD_DAT13__GPIO3_IO01 0x79 > + MX6SL_PAD_LCD_DAT14__GPIO3_IO02 0x79 > + MX6SL_PAD_LCD_DAT15__GPIO3_IO03 0x79 > + MX6SL_PAD_LCD_DAT16__GPIO3_IO04 0x79 > + MX6SL_PAD_LCD_DAT17__GPIO3_IO05 0x79 > + MX6SL_PAD_LCD_DAT18__GPIO3_IO06 0x79 > + MX6SL_PAD_LCD_DAT19__GPIO3_IO07 0x79 > + MX6SL_PAD_LCD_DAT20__GPIO3_IO08 0x79 > + MX6SL_PAD_LCD_DAT21__GPIO3_IO09 0x79 > + MX6SL_PAD_LCD_DAT22__GPIO3_IO10 0x79 > + MX6SL_PAD_LCD_DAT23__GPIO3_IO11 0x79 > + MX6SL_PAD_LCD_CLK__GPIO2_IO15 0x79 > + MX6SL_PAD_LCD_ENABLE__GPIO2_IO16 0x79 > + MX6SL_PAD_LCD_HSYNC__GPIO2_IO17 0x79 > + MX6SL_PAD_LCD_VSYNC__GPIO2_IO18 0x79 > + MX6SL_PAD_LCD_RESET__GPIO2_IO19 0x79 > + MX6SL_PAD_KEY_COL3__GPIO3_IO30 0x79 > + MX6SL_PAD_KEY_ROW7__GPIO4_IO07 0x79 > + MX6SL_PAD_ECSPI2_MOSI__GPIO4_IO13 0x79 > + MX6SL_PAD_KEY_COL5__GPIO4_IO02 0x79 > + MX6SL_PAD_KEY_ROW6__GPIO4_IO05 0x79 > + >; > + }; Why are there so many hogged pins? Will some of them receive a proper configuration once the EPDC driver is implemented? > +&snvs_rtc { > + /* we are using the RTC in the PMIC, not disabled in imx6sl.dtsi */ > + status = "disabled"; This comment sounds a bit ambiguous (and this potentially confusing). Perhaps: + /* we are using the RTC in the PMIC, but this one is not disabled in imx6sl.dtsi */ Or even just: + /* we are using the RTC in the PMIC */ > +&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"; > +}; IMHO, please add a comment saying what this MMC controller is connected to (internal storage?). > + > +&usdhc3 { > + pinctrl-names = "default", "state_100mhz", "state_200mhz", "sleep"; > + pinctrl-0 = <&pinctrl_usdhc3>; > + pinctrl-1 = <&pinctrl_usdhc3_100mhz>; > + pinctrl-2 = <&pinctrl_usdhc3_200mhz>; > + pinctrl-3 = <&pinctrl_usdhc3_sleep>; > + vmmc-supply = <®_wifi>; > + mmc-pwrseq = <&wifi_pwrseq>; > + cap-power-off-card; > + non-removable; > + status = "okay"; > + > + /* CyberTan WC121 SDIO WiFi */ > +}; The HWCONFIG block from my Shine2HD reports RTL8189 as the Wifi chip (value 8 at offset 4), and kernel logs from the vendor kernel appear to agree that it's a realtek chip, at least (lines prefixed RTL871X). From my experience with the CyberTan WC121, it has a Broadcom fullmac chip inside. Now I wonder where this discrepancy or variability comes from. I guess the SDIO setup can deal with different chips (like Broadcom vs. Realtek) as long as the board has been designed to always use the same reset/power/etc. lines. I don't see any branching based on the 'Wifi' HWCONFIG entry in the vendor kernel, so I guess that's the case. In any case, it might be nice to also note the chip used inside the WLAN package (e.g. BCM43362), to make it easier for interested users to choose the right drivers. Kind regards, Jonathan Neuschäfer
Attachment:
signature.asc
Description: PGP signature