Re: [PATCH 1/2] documentation: add rockchip spi documentation

[Mark Rutland]

On Tue, Jun 24, 2014 at 11:32:21AM +0100, Heiko StÃbner wrote:
> Am Dienstag, 24. Juni 2014, 11:18:02 schrieb Mark Rutland:
> > On Tue, Jun 24, 2014 at 04:58:43AM +0100, addy ke wrote:
> > > Signed-off-by: addy ke <addy.ke@xxxxxxxxxxxxxx>
> > > ---
> > > 
> > >  .../devicetree/bindings/spi/spi-rockchip.txt       | 51
> > >  ++++++++++++++++++++++ 1 file changed, 51 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > > b/Documentation/devicetree/bindings/spi/spi-rockchip.txt new file mode
> > > 100644
> > > index 0000000..ce9c881
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/spi/spi-rockchip.txt
> > > @@ -0,0 +1,51 @@
> > > +* Rockchip SPI Controller
> > > +
> > > +The Rockchip SPI controller is used to interface with various devices
> > > such as flash +and display controllers using the SPI communication
> > > interface.
> > > +
> > > +Required SoC Specific Properties:
> > > +
> > > +- compatible: should be one of the following.
> > > +    - rockchip,rk3066-spi: for rk3066, rk3188 and rk3288 platforms.
> > 
> > Are you sure you don't want specifc strings for rk3188 and rk3288 (in
> > addtion to the common "rockchip,rk3066-spi")?
> 
> Wasn't the convention that "later" platforms that are compatible to an earlier 
> one, reuse this compatible string instead of introducing a new one?

That's why I said in addition to the common one. I'd only expect the
driver to look for "rockchip,rk3066-spi", but a DTB could have:

	compatible = "rockchip,rk3188-spi", "rockchip,rk3066-spi";

Seeding the DTBs with the extra strings early makes it more likely that
we can rely on them later. If we don't happen to need them they only
clutter some DTBs.

> From what I've heard so far, the specific spi controller got introduced with 
> the rk3066 [earlier SoCs used a different implementation] and didn't change for 
> rk3188 and rk3288. Addy may be able to verify this.
> 
> 
> > > +- reg: physical base address of the controller and length of memory
> > > mapped
> > > +       region.
> > > +- interrupts: The interrupt number to the cpu. The interrupt specifier
> > > format +              depends on the interrupt controller.
> > > +- clocks: Must contain an entry for each entry in clock-names.
> > > +- clock-names: Shall be "spiclk" for the transfer-clock, and "apb_pclk"
> > > for +			   the peripheral clock.
> > > +
> > > +Optional properties:
> > > +- dmas: DMA specifiers for tx and rx dma. See the DMA client binding,
> > > +		Documentation/devicetree/bindings/dma/dma.txt
> > > +- dma-names: DMA request names should include "tx" and "rx" if present.
> > > +
> > > +Example:
> > > +
> > > +- SoC Specific Portion:
> > > +
> > > +	spi0: spi@ff110000 {
> > > +		compatible = "rockchip,rockchip-spi";
> > 
> > This does not match the description of the compatible property.
> > 
> > > +		reg = <0xff110000 0x1000>;
> > > +		dmas = <&pdma1 11>, <&pdma1 12>;
> > > +		dma-names = "tx", "rx";
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > 
> > These weren't mentioned.
> > 
> > > +		interrupts = <GIC_SPI 44 IRQ_TYPE_LEVEL_HIGH>;
> > > +		pinctrl-names = "default";
> > > +		pinctrl-0 = <&spi0_clk &spi0_tx &spi0_rx &spi0_cs0 &spi0_cs1>;
> > 
> > pinctrl was not mentioned.
> > 
> > > +		clocks = <&cru SCLK_SPI0>, <&cru PCLK_SPI0>;
> > > +		clock-names = "spiclk", "apb_pclk";
> > > +		status = "disabled";
> > 
> > Any reason for the status?
> 
> I guess to have the spi controller only be enabled when a board is using it as 
> below. But it may be an implementation detail which could be omitted from the 
> binding doc.

Yes please :)

> 
> > 
> > > +	};
> > > +
> > > +- Board Specific Portion:
> > > +
> > > +	&spi0 {
> > > +		status = "okay";
> > > +		spi_test@00 {
> > > +			compatible = "rockchip,spi_test";
> > 
> > Huh?
> 
> SPI declares it's devices similar to i2c, so while the example might profit 
> from a more casual device, I'm not exactly sure what is the problem here.

Sure. Given that this is a known pattern for SPI controllers, do we need
this in each binding doc?

Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/