Re: [PATCH v8 1/3] arm64: dts: fsl: librem5: Add a device tree for the Librem5 devkit

From: Angus Ainslie
Date: Sat May 11 2019 - 13:50:12 EST


Hi Fabio,

On 2019-05-11 08:34, Fabio Estevam wrote:
Hi Angus,

This looks good. Only minor issues:

On Wed, May 1, 2019 at 7:57 PM Angus Ainslie (Purism) <angus@xxxxxxxx> wrote:
+++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5-devkit.dts
@@ -0,0 +1,823 @@
+/* SPDX-License-Identifier: GPL-2.0+

This should be
// SPDX-License-Identifier: GPL-2.0+

as pointed out by checkpatch.

+ reg_1v8_p: regulator-1V8-P {

Maybe lowercase instead?


Ok, I'll fix those 2.

+ wifi_pwr_en: wifi-en {
+ compatible = "regulator-fixed";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_wifi_pwr_en>;
+ regulator-name = "WIFI_EN";
+ regulator-min-microvolt = <3300000>;
+ regulator-max-microvolt = <3300000>;
+ gpio = <&gpio3 5 GPIO_ACTIVE_HIGH>;
+ enable-active-high;
+ regulator-always-on;

Do you really needs all these regulators to be 'regulator-always-on'?


Unfortunately yes.

+&i2c1 {
+ clock-frequency = <400000>;

Maybe you could use 100kHz instead in order to avoid the problem
described by the following i.MX8M erratum:

e7805: I2C: When the I2C clock speed is configured for 400 kHz, the
SCL low period violates the I2C spec of 1.3 uS min
https://www.nxp.com/docs/en/errata/IMX8MDQLQ_1N14W.pdf


I discovered this a few days ago.

https://source.puri.sm/angus.ainslie/linux-next/commit/10c9d9e55aba0e179166001b9292e37a37bfe0ea

You're right 100kHz is a better choice.

+&usdhc2 {
+ pinctrl-names = "default", "state_100mhz", "state_200mhz";
+ pinctrl-0 = <&pinctrl_usdhc2>;
+ pinctrl-1 = <&pinctrl_usdhc2_100mhz>;
+ pinctrl-2 = <&pinctrl_usdhc2_200mhz>;
+ bus-width = <4>;
+ vmmc-supply = <&reg_usdhc2_vmmc>;
+ power-supply = <&wifi_pwr_en>;
+

Unneeded blank line.

I'll fix it for the next rev.

Thanks
Angus