Re: [PATCH RFC 2/2] ARM: dts: imx: add devicetree for Tolino Shine 2 HD

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 5377 bytes --]

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@kemnade.info>
> ---
> 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 = <&reg_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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]