On Thu, 2015-05-07 at 12:31 -0400, Oleksandr G Zhadan wrote:
> Hi Scott,
>
> Thanks for fast response, please see inline.
>
> On 05/06/2015 11:22 PM, Scott Wood wrote:
> > On Tue, 2015-05-05 at 11:52 -0400, Oleksandr G Zhadan wrote:
> >> +-------------------------------------------------
> >> +
> >> +P1020 SPI controller
> >> +
> >> +Properties:
> >> +- compatible: "spansion,s25fl008k", "winbond,w25q80bl"
> >> +
> >> +Example:
> >> + spi@7000 {
> >> + flash@0 {
> >> + #address-cells = <1>;
> >> + #size-cells = <1>;
> >> + compatible = "spansion,s25fl008k", "winbond,w25q80bl";
> >> + reg = <0>;
> >> + spi-max-frequency = <40000000>; /* input clock */
> >> + ...
> >> + };
> >
> > This isn't describing the controller, but rather a SPI chip attached to
> > the controller. This also doesn't seem like the right place for random
> > SPI chips.
> >
> > If all you're specifying is the compatible, maybe create a
> > spi/trivial-devices.txt similar to i2c/trivial-devices.txt? Or
> > something specific to SPI flash chips to describe the partition
> > specification, though I generally recommend against describing
> > partitions in the device tree -- especially if this is a developer board
> > rather than something fixed-purpose where the partitioning is not going
> > to change based on user requirements.
> >
> >
>
> Mostly in all Documentation/devicetree/bindings/ I tried to satisfy
> checkpatch script as simple as possible. And for me as well it looks
> reasonable to create spi/trivial-devices.txt file and I will.
Checkpatch is a tool, not a dictator. Sometimes it gets things wrong.
Also, please CC devicet...@vger.kernel.org when adding bindings or
modifying dts files.
> >> +-------------------------------------------------
> >> +
> >> +Chipselect/Local Bus
> >> +
> >> +Properties:
> >> +- #address-cells: <2>.
> >> +- #size-cells: <1>.
> >> +- compatible: "fsl,p1020-elbc", "fsl,elbc",
> >> "simple-bus","fsl,p1020-immr"
> >> +- interrupts: interrupts to report localbus events.
> >> +
> >> +Example:
> >> +
> >> +&lbc {
> >> + #address-cells = <2>;
> >> + #size-cells = <1>;
> >> + compatible = "fsl,p1020-elbc", "fsl,elbc", "simple-bus";
> >> + interrupts = <19 2 0 0>;
> >> +};
> >
> > There's already a binding for elbc -- and the elbc node certainly should
> > not claim compatibility with "fsl,p1020-immr".
> >
> >
>
> to satisfy checkpatch script.
Even if that were necessary, why do it by copy-and-paste, and why put
the immr compatible in the binding for a different node?
> >> diff --git a/arch/powerpc/boot/dts/fsl/ucp1020som-post.dtsi
> >> b/arch/powerpc/boot/dts/fsl/ucp1020som-post.dtsi
> >> new file mode 100644
> >> index 0000000..930a6e3
> >> --- /dev/null
> >> +++ b/arch/powerpc/boot/dts/fsl/ucp1020som-post.dtsi
> >
> > Why can't you use p1020si-post.dtsi? The "si" means "silicon" -- it's
> > meant to be included by all p1020 boards.
> >
>
> Yes, silicon is the same, but p1020 boards using all 3 etsec ethernet
> controllers. Our board using only 2: etsec1 and etsec3.
So have your board write status = "disabled" into the etsec2 node after
including the post file.
> >> diff --git a/arch/powerpc/configs/ucp1020_defconfig
> >> b/arch/powerpc/configs/ucp1020_defconfig
> >> new file mode 100644
> >> index 0000000..62f99aa
> >> --- /dev/null
> >> +++ b/arch/powerpc/configs/ucp1020_defconfig
> >
> > Please explain why your board needs its own defconfig.
> >
>
> Because, it's our own board and it has some specific to board
> definitions like CONFIG_DEFAULT_HOSTNAME and some specific to product
> definitions.
>
> If I can do it in some other way could you please give me some example
> if it's possible.
I don't think stuff like CONFIG_DEFAULT_HOSTNAME belongs upstream.
Could you list what you need to be set that mpc85xx_smp_defconfig
doesn't set?
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
