Hi Arnd,
thanks for having a look at it.
2013/3/4 Arnd Bergmann <arnd@xxxxxxxx>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
