[PATCH v4 1/3] serial: sh-sci: Add OF support

[Bastian Hecht]

Hi Arnd,

thanks for having a look at it.

2013/3/4 Arnd Bergmann <arnd at arndb.de>:
> On Monday 04 March 2013 17:20:52 Bastian Hecht wrote:
>> diff --git a/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt
>> new file mode 100644
>> index 0000000..6ad1adf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/tty/serial/renesas,sci-serial.txt
>> @@ -0,0 +1,53 @@
>> +* Renesas SH-Mobile Serial Communication Interface
>> +
>> +Required properties:
>> +- compatible : Should be "renesas,sci-<port type>-uart", where <port type> may be
>> +  SCI, SCIF, IRDA, SCIFA or SCIFB.
>
> Why capital letters here? Maybe just list
>
>         "renesas,sci-uart", "renesas,scif-uart", ...

Yes - changed.

>> +- reg : Address and length of the register set for the device
>> +- interrupts : Should contain the following IRQs: ERI, RXI, TXI and BRI.
>
> You probably want to require the "interrupt-names" property as well
> then.

I added the interrupt-names property but still enforce the ordering in
the binding specs.
Don't know if we want the extra overhead to scan for strings or just
take a certain order for granted.
If we want to change it to parsing, it would be better to switch
completely to platform_get_irq_byname() and change all the current
platform code, else we will produce code overhead.

>> +- cell-index : The device id.
>> +- renesas,scscr : Should contain a bitfield used by the Serial Control Register.
>> +  b7 = SCSCR_TIE
>> +  b6 = SCSCR_RIE
>> +  b5 = SCSCR_TE
>> +  b4 = SCSCR_RE
>> +  b3 = SCSCR_REIE
>> +  b2 = SCSCR_TOIE
>> +  b1 = SCSCR_CKE1
>> +  b0 = SCSCR_CKE0
>> +- renesas,scbrr-algo-id : Algorithm ID for the Bit Rate Register
>> +  1 = SCBRR_ALGO_1 ((clk + 16 * bps) / (16 * bps) - 1)
>> +  2 = SCBRR_ALGO_2 ((clk + 16 * bps) / (32 * bps) - 1)
>> +  3 = SCBRR_ALGO_3 (((clk * 2) + 16 * bps) / (16 * bps) - 1)
>> +  4 = SCBRR_ALGO_4 (((clk * 2) + 16 * bps) / (32 * bps) - 1)
>> +  5 = SCBRR_ALGO_5 (((clk * 1000 / 32) / bps) - 1)
>
> Maybe replace this with a "clock-frequency" property? This may
> be what the registers contain, but it is not very readable.

Hmm... do you want a frequency in absolute terms? And then calculate
the best SCBRR value for it?
I'm unsure about this, either you must exactly hit it or accept
tolerances? As something in between I renamed the property to
"renesas,clock-algorithm", but I don't know if this is what you want.

>> +Optional properties:
>> +- renesas,autoconf : Set if device is capable of auto configuration
>> +- renesas,regtype : Overwrite the register layout. In most cases you can rely
>> +  on auto-probing (omit this property or set to 0) but some legacy devices
>> +  use a non-default register layout. Possible layouts are
>> +  0 = SCIx_PROBE_REGTYPE (default)
>> +  1 = SCIx_SCI_REGTYPE
>> +  2 = SCIx_IRDA_REGTYPE
>> +  3 = SCIx_SCIFA_REGTYPE
>> +  4 = SCIx_SCIFB_REGTYPE
>> +  5 = SCIx_SH2_SCIF_FIFODATA_REGTYPE
>> +  6 = SCIx_SH3_SCIF_REGTYPE
>> +  7 = SCIx_SH4_SCIF_REGTYPE
>> +  8 = SCIx_SH4_SCIF_NO_SCSPTR_REGTYPE
>> +  9 = SCIx_SH4_SCIF_FIFODATA_REGTYPE
>> + 10 = SCIx_SH7705_SCIF_REGTYPE
>
> Why do you keep these separate from the "compatible" property?
> By definition, the register layout is already determined by
> "compatible".

Ok, I've melted the register type and port type (defined in
include/uapi/linux/serial_core.h) definition into the compatible
property. Looks good!

>> +#ifdef CONFIG_OF
>> +static const struct of_device_id of_sci_match[] = {
>> +     { .compatible = "renesas,sci-SCI-uart",
>> +             .data = (void *)PORT_SCI },
>> +     { .compatible = "renesas,sci-SCIF-uart",
>> +             .data = (void *)PORT_SCIF },
>> +     { .compatible = "renesas,sci-IRDA-uart",
>> +             .data = (void *)PORT_IRDA },
>> +     { .compatible = "renesas,sci-SCIFA-uart",
>> +             .data = (void *)PORT_SCIFA },
>> +     { .compatible = "renesas,sci-SCIFB-uart",
>> +             .data = (void *)PORT_SCIFB },
>> +     {},
>> +};
>> +MODULE_DEVICE_TABLE(of, of_sci_match);
>> +
>> +static struct plat_sci_port *sci_parse_dt(struct platform_device *pdev,
>> +                                                             int *dev_id)
>> +{
>> +     struct plat_sci_port *p;
>> +     struct device_node *np = pdev->dev.of_node;
>> +     const struct of_device_id *match;
>> +     struct resource *res;
>> +     const __be32 *prop;
>> +     int i, irq, val;
>> +
>
> You can remove the #ifdef by replacing it with
>
>         if (!IS_ENABLED(CONFIG_OF) || !np)
>                 return NULL;
>
> here. This gives better compile time coverage and more readable code.

Alright, done!

> Otherwise looks very nice!

Cheers!

 Bastian


>         Arnd