On Mon, Nov 11, 2013 at 01:26:11PM +0000, Laurent Pinchart wrote: > Hi Mark, > > Thank you for the quick and detailed review. > > On Monday 11 November 2013 09:40:39 Mark Rutland wrote: > > Hi Laurent, > > > > I have a few comments. > > > > On Sun, Nov 10, 2013 at 03:33:40AM +0000, Laurent Pinchart wrote: > > > Document the device tree bindings for the sci serial port devices. > > > > > > Cc: devicetree@xxxxxxxxxxxxxxx > > > Signed-off-by: Laurent Pinchart > > > <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx> > > > Acked-by: Simon Horman <horms+renesas@xxxxxxxxxxxx> > > > --- > > > > > > .../bindings/serial/renesas,sci-serial.txt | 42 +++++++++++++++++ > > > 1 file changed, 42 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/serial/renesas,sci-serial.txt> > > > diff --git > > > a/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > > b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt new > > > file mode 100644 > > > index 0000000..66d3bca > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/serial/renesas,sci-serial.txt > > > @@ -0,0 +1,42 @@ > > > +* Renesas SH-Mobile Serial Communication Interface > > > + > > > +Required properties: > > > + > > > + - compatible: one of the following types (scif, scifa, scifb, hscif). > > > > Minor nit, but would be nice to have "Should contain one of the > > following:". We might have future variants whereby the compatible string > > will actually be a string list. > > Sure, I can do that. > > > > + > > > + - "renesas,scif-r8a7790" for R8A7790 (R-Car H2) SCIF compatible UART. > > > + - "renesas,scifa-r8a7790" for R8A7790 (R-Car H2) SCIFA compatible > > > UART. + - "renesas,scifb-r8a7790" for R8A7790 (R-Car H2) SCIFB > > > compatible UART. + - "renesas,hscif-r8a7790" for R8A7790 (R-Car H2) > > > HSCIF compatible UART. + - "renesas,scif-generic" for generic SCIF > > > compatible UART. > > > + - "renesas,scifa-generic" for generic SCIFA compatible UART. > > > + - "renesas,scifb-generic" for generic SCIFB compatible UART. > > > + - "renesas,hscif-generic" for generic HSCIF compatible UART. > > > > Is the "-generic" suffix necessary? Why not just "renesas,scif" etc? > > You're right, I'll remove the suffix. > > > > + > > > + When compatible with the generic version, nodes must also list the > > > + SoC-specific version corresponding to the platform. > > > > Presumably the SoC-specific version should come first; it would be nice > > to note that. > > Indeed, I'll do so. > > > > + > > > + - reg: Base address and length of the memory resource used by the UART. > > > > I assume by memory resource you mean the memory-mapped registers? > > Resource sounds like Linux-internal nomenclature leaking. > > I'll fix it as well, although we could decide on a definition of "resource" > that isn't Linux-specific :-) We could, I guess. For the moment I'd prefer to point out that these are MMIO registers :) > > > > + > > > + - interrupt-parent: Reference to the parent interrupt controller. > > > > I don't think this is strictly necessary. It's implicit by default and > > can be added optionally when a system is wired in a complex way. As it's > > a completely standard optional property I'm not even sure it needs to be > > documented. > > > > > + - interrupts: Interrupt number. > > > > Minor nit: "Should contain an interrupt-specifier for ..." > > > > I saw the cover mentioned multiple interrupts. Which logical interrupt > > output of the device are you expecting here? > > SCI devices come in several flavours, with individual interrupt sources wired > up to different interrupt lines, or multiplexed all together. As the DT > bindings support the later only at the moment, there's only a single interrupt > to handle. Ok. If the binding just covers the muxed interrupt for now that's fine. We can add interrupt-names later when we support multiple interrupts (and fall back to assuming muxed when no interrupt-names property is present). > > Should we come up with a standard way of saying in the bindings that a device > uses the interrupt bindings defined in interrupt-controller/interrupts.txt ? > This will become even more important with the interrupts-extended property, we > don't want every DT bindings document to detail all possible way to specify > interrupts. It would certainly be nice to have a standard way of doing just that. I'd hope that with the schema work going on that would fall into place automatically, so perhaps it's not worth doing now. > > > > + > > > + - clocks: Reference to the SCIx UART interface clock. > > > > Minor nit: s/Reference to/A phandle + clock-specifier pair for/ > > > > > + - clock-names: Should be "sci_ick". > > > > As we're using clock-names, it would be nicer still to define clocks in > > terms of clock-names so as to make it easier to update the document in > > future and make the expected use of clock-names clearer: > > > > - clocks: Should > > "Should", or "must" ? I don't expect the format of clocks and the relationship to clock-names to change, so "must" is likely more appropriate. I'm just too used to writing "should"... > > > contain a phandle + clock-specifier pair for each entry > > in clock-names. > > > > - clock-names: Should contain "sci_ick". > > Shouldn't that be > > "Should contain "sci_ick" for the SCIx UART interface clock." > > ? Otherwise the bindings wouldn't tell which clock the clocks property should > reference. Yes, that sounds much better. Cheers, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
