| Message ID | 1444729160-26433-4-git-send-email-bayi.cheng@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello. On 10/13/2015 12:39 PM, Bayi Cheng wrote: > Add Mediatek nor flash node > > Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com> > --- > 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. > + compatible = "mediatek,mt8173-nor"; > + reg = <0 0x1100d000 0 0xe0>; > + clocks = <&pericfg CLK_PERI_SPI>, > + <&topckgen CLK_TOP_SPINFI_IFR_SEL>; > + clock-names = "spi", "sf"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + flash@0 { > + compatible = "jedec,spi-nor"; > + reg = <0>; > + }; > + }; > + > i2c3: i2c3@11010000 { Grr, the name should be "i2c@11010000". [...] MBR, Sergei
Sergei, On Tue, Oct 13, 2015 at 10:33 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Hello. > > On 10/13/2015 12:39 PM, Bayi Cheng wrote: > >> Add Mediatek nor flash node >> >> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com> >> --- >> 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"? Would you prefer: spi_nor_flash: spi@1100d000 > >> + compatible = "mediatek,mt8173-nor"; >> + reg = <0 0x1100d000 0 0xe0>; >> + clocks = <&pericfg CLK_PERI_SPI>, >> + <&topckgen CLK_TOP_SPINFI_IFR_SEL>; >> + clock-names = "spi", "sf"; >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + flash@0 { >> + compatible = "jedec,spi-nor"; >> + reg = <0>; >> + }; >> + }; >> + >> i2c3: i2c3@11010000 { > > > Grr, the name should be "i2c@11010000". Don't worry. This is already fixed in Matthias' tree: https://github.com/mbgg/linux-mediatek/commit/1ee35c05d9c67771d649f687e6f76e61e76eb779 -Dan > > [...] > > MBR, Sergei >
Hello. On 10/14/2015 4:07 AM, Daniel Kurtz wrote: >>> Add Mediatek nor flash node >>> >>> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com> >>> --- >>> 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. [...] > -Dan MBR, Sergei
Sergei, On Wed, Oct 14, 2015 at 7:05 PM, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote: > Hello. > > On 10/14/2015 4:07 AM, Daniel Kurtz wrote: > >>>> Add Mediatek nor flash node >>>> >>>> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com> >>>> --- >>>> 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
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@cogentembedded.com> wrote: > > Hello. > > > > On 10/14/2015 4:07 AM, Daniel Kurtz wrote: > > > >>>> Add Mediatek nor flash node > >>>> > >>>> Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com> > >>>> --- > >>>> 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
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 { + compatible = "mediatek,mt8173-nor"; + reg = <0 0x1100d000 0 0xe0>; + clocks = <&pericfg CLK_PERI_SPI>, + <&topckgen CLK_TOP_SPINFI_IFR_SEL>; + clock-names = "spi", "sf"; + #address-cells = <1>; + #size-cells = <0>; + + flash@0 { + compatible = "jedec,spi-nor"; + reg = <0>; + }; + }; + i2c3: i2c3@11010000 { compatible = "mediatek,mt8173-i2c"; reg = <0 0x11010000 0 0x70>,
Add Mediatek nor flash node Signed-off-by: Bayi Cheng <bayi.cheng@mediatek.com> --- arch/arm64/boot/dts/mediatek/mt8173.dtsi | 15 +++++++++++++++ 1 file changed, 15 insertions(+)