Messages in this thread | | | Subject | Re: [PATCH] arm64: dts: Add device trees for MSC SM2S-IMX8PLUS SoM and carrier board | From | Martyn Welch <> | Date | Tue, 19 Jul 2022 17:37:48 +0100 |
| |
On Tue, 2022-07-19 at 12:01 +0200, Krzysztof Kozlowski wrote: > On 18/07/2022 17:23, Martyn Welch wrote: > > Add device trees for one of a number of MSCs variants of the SM2S- > > IMX8PLUS > > system on module along with the compatible SM2S-SK-AL-EP1 carrier > > board. > > As the name suggests, this family of SoMs use the NXP i.MX8MP SoC > > and > > provide the SMARC module interface. > > > > Signed-off-by: Martyn Welch <martyn.welch@collabora.com> > > Use subject prefix matching subsystem. I expect other folks in > Collabora > help you in that, so you do not need our advices for such trivial > stuff. :) >
Hi Krzysztof,
Thanks for the review.
I picked that based on the last 20-30 commits under arch/arm64/boot/dts/. Would you prefer something starting "arm64: dts: freescale: "? I see that "arm64: dts: imx8mp: " is typically being used for changes to the more generic imx8mp device trees...
> > + extcon_usb0: extcon_usb0 { > > No underscores, extcon is Linux term, so use node name describing > device. >
I note that the device binding file lists an example using "extcon_usb1". I also note that existing users seem to broadly use a variation of "extcon-XXXX", would "extcon-usb0" be acceptable in this case?
> > > + dsi_lvds_bridge: sn65dsi84@2d { > > Node names should be generic. > https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation >
My apologies - I thought I'd got all of these...
> > > + qspi_flash: qspi_flash@0 { > > You didn't test the bindings (dtbs_check), did you? There is no way > this > passess... >
No, despite having written device tree bindings on and off for something approaching 18 years (though admittedly more off than on), I was unaware of this tool. I'll run this before resubmitting.
> > + reg = <0>; > > + #address-cells = <1>; > > + #size-cells = <1>; > > + compatible = "jedec,spi-nor"; > > Eh? Now compatible in the middle? Sorry, this are trivial code > quality > comments. Please use internal review or base your work on some > upstreamed existing board. >
Sorry - I missed that one. I'd moved most of the compatible strings to the top of nodes.
Martyn
> Best regards, > Krzysztof
| |