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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Sat, 11 May 2019 10:48:42 -0700
From:   Angus Ainslie <angus@...ea.ca>
To:     Fabio Estevam <festevam@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        NXP Linux Team <linux-imx@....com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v8 1/3] arm64: dts: fsl: librem5: Add a device tree for  the Librem5 devkit

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@...ea.ca> 
> 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