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", ...
> +- 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.
> +- 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.
> +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".
> +#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.
Otherwise looks very nice!
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
