Re: [PATCH v2 28/29] serial: sh-sci: Add device tree bindings documentation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux