linux-kernel - Re: [PATCH 2/2] ARM: dts: pxa: add mioa701 board description

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]

Message-ID: <87lgada80e.fsf@belgarion.home>
Date:   Sat, 14 Jul 2018 22:15:45 +0200
From:   Robert Jarzmik <robert.jarzmik@...e.fr>
To:     Rob Herring <robh+dt@...nel.org>
Cc:     Mark Rutland <mark.rutland@....com>,
        Daniel Mack <daniel@...que.org>,
        Haojian Zhuang <haojian.zhuang@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>, devicetree@...r.kernel.org,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        "moderated list\:ARM\/FREESCALE IMX \/ MXC ARM ARCHITECTURE" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/2] ARM: dts: pxa: add mioa701 board description

Rob Herring <robh+dt@...nel.org> writes:

Hi Rob,

>> +       /* compatible = "mitac,mioa701"; */
>> +       compatible = "marvell,pxa270";
>
> Why the comment?
Leftover, I'll remove it.

>> +                       pstore_region:region@...2000000 {
>
> Drop the 0x
Done.

>> +                               compatible = "linux,contiguous-memory-region";
>> +                               reg = <0xa2000000 1048576>;
>
> Use hex for the size.
Ok.

>> +               ffuart: uart@...00000 {
>
> Should be "serial@...". You'll have to fix in the base dtsi.
Ok for all of these.
>> +               pxai2c1: i2c@...01680 {
>> +                       mrvl,i2c-fast-mode;
>> +                       pinctrl-names = "default";
>> +                       pinctrl-0 = <&pinctrl_i2c_default>;
>> +                       status = "okay";
>> +
>> +                       mt9m111: camera@5d {
>> +                               compatible = "micron,mt9m111";
>> +                               reg = <0x5d>;
>> +                               gpios = <&gpio 56 GPIO_ACTIVE_HIGH>;
>> +
>> +                               remote = <&pxa_camera>;
>
> Not needed with the graph:
I don't understand that, do you mean the port node is not needed ?

>
>> +                               port {
>> +                                       mt9m111_1: endpoint {
>> +                                               bus-width = <8>;
>> +                                               remote-endpoint = <&pxa_camera>;
>> +                                       };
>> +                               };
>> +                       };
>> +               };
>> +               gpio-keys {
>> +                       compatible = "gpio-keys";
>> +                       #address-cells = <1>;
>> +                       #size-cells = <0>;
>> +                       autorepeat;
>> +                       pinctrl-names = "default";
>> +                       pinctrl-0 = <&pinctrl_gpiokeys_default>;
>> +                       status = "okay";
>
> Don't need status here? It shouldn't be defined in the base dtsi.
Heuh what ? The status is just above, and gpio-keys is not defined in any dtsi.

>> +
>> +                       button@0 {
>
> If you have unit-address there should also be a reg property. Just
> drop and name the node 'power' or 'power-button'. And similarly for
> the rest.
Ok.

>> +
>> +       regulators {
>> +               compatible = "simple-bus";
>
> Drop this and move children to the top level. This is not an mmio bus.
Ok.

>> +       docg3: flash@0 {
>
> This probably should be a child of the bus controller.
Most certainly yes.

>
>> +               compatible = "m-systems,diskonchip-g3";
>> +               reg = <0x0 0x2000>;
>> +       };
>> +
>> +       panel {
>> +               compatible = "toshiba,ltm0305a776";
>> +               lcd-type = "color-tft";
>
> This should be implied by the compatible.
Mmm no. Actually, this is used by the framebuffer controller, ie. pxafb to set
it up. And I don't think adding a list of panel compatibles in pxafb is a good
idea.
The corresponding code is in : drivers/video/fbdev/pxafb.c, see
"static const char * const lcd_types[]".

>> +               display-timings {
>> +                       native-mode = <&timing0>;
>
> We generally imply this from the compatible (i.e. use simple-panel).
But you suppose there is an actual driver for the panel, right ? But there is
none.

> Could use another level of indentation.
Sure.

>> +       ac_charger: charger@1 {
>
> Drop the unit-address.
Ok.

Thanks for the review.

-- 
Robert