Re: [PATCH v4 3/3] arm64: dts: mt8173: Add nor flash node

[bayi.cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>]

On Wed, 2015-10-14 at 19:26 +0800, Daniel Kurtz wrote:
> Sergei,
> 
> On Wed, Oct 14, 2015 at 7:05 PM, Sergei Shtylyov
> <sergei.shtylyov-M4DtvfQ/ZS1MRgGoP+s0PdBPR1lH4CV8@public.gmane.org> wrote:
> > Hello.
> >
> > On 10/14/2015 4:07 AM, Daniel Kurtz wrote:
> >
> >>>> Add Mediatek nor flash node
> >>>>
> >>>> Signed-off-by: Bayi Cheng <bayi.cheng-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> >>>> ---
> >>>>    arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++
> >>>>    1 file changed, 15 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>> b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>> index d18ee42..385c2e4 100644
> >>>> --- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>> +++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
> >>>> @@ -365,6 +365,21 @@
> >>>>                          status = "disabled";
> >>>>                  };
> >>>>
> >>>> +               nor_flash: spi@1100d000 {
> >>>
> >>>
> >>>     I don't think this is a good label name for a SPI controller.
> >>
> >>
> >> You think "spi" is not a good name for SPI controller?
> >> Or you mean the label "nor_flash"?
> >
> >
> >    I think I said clearly: "label". :-)
> >
> >> Would you prefer:
> >>
> >>    spi_nor_flash: spi@1100d000
> >
> >
> >    I'd prefer the "_nor_flash" part to be dropped, to be used for the real
> > flash device (if needed) which is a sub-node of this node.
> 
> The dedicated nor_flash spi bus cannot just be labeled 'spi', as that
> would conflict with the general purpose 'spi' bus.  Instead, it is
> given a unique label that can be used as a phandle elsewhere (e.g., in
> a board file, to enable the bus).  The name "spi_nor_flash" makes it
> clear that this spi bus is only for use for accessing the nor_flash.
> 
> Bayi: Actually, this reminds me.  I'd prefer if the bus node was
> disabled by default, and only enabled by boards as required.  So, the
> node should include:
>    status = "disabled";
> 
> -Dan

Hi, Dan & Sergei, Thanks for your advice, and I will add "status =
"disable"" to the node in the next patch.Thanks~

Bayi