On Mon, Oct 19, 2015 at 1:58 AM, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote: > On 10/19/2015 08:39 AM, Heiko Schocher wrote: >> add DT support for the ti hecc controller, used on >> am3517 SoCs. > > A similar patch was posted a few days ago, see > http://comments.gmane.org/gmane.linux.can/8616 and my comments. I don't seem to have that in my inbox. Please send DT bindings to the DT list and maintainers. Rob > > Please coordinate with Anton Glukhov (Cc'ed) and/or pick up his patches > as they are in better shape. > > Marc >> >> Signed-off-by: Heiko Schocher <hs@xxxxxxx> >> --- >> >> .../devicetree/bindings/net/can/ti_hecc-can.txt | 20 ++++++++++ >> arch/arm/boot/dts/am3517.dtsi | 13 +++++++ >> drivers/net/can/ti_hecc.c | 45 +++++++++++++++++++++- >> 3 files changed, 76 insertions(+), 2 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/net/can/ti_hecc-can.txt >> >> diff --git a/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt >> new file mode 100644 >> index 0000000..09fab59 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/can/ti_hecc-can.txt >> @@ -0,0 +1,20 @@ >> +* TI HECC CAN * >> + >> +Required properties: >> + - compatible: Should be "ti,hecc" > > We usually put the name of the first SoC this IP core appears in to the > compatible. > >> + - reg: Should contain CAN controller registers location and length >> + - interrupts: Should contain IRQ line for the CAN controller > > I'm missing the description of the ti,* properties. I think they are > required, too. Although the code doesn't enforce it. > >> + >> +Example: >> + >> + can0: hecc@5c050000 { >> + compatible = "ti,hecc"; >> + reg = <0x5c050000 0x4000>; >> + interrupts = <24>; >> + ti,hecc_scc_offset = <0>; >> + ti,hecc_scc_ram_offset = <0x3000>; >> + ti,hecc_ram_offset = <0x3000>; >> + ti,hecc_mbx_offset = <0x2000>; >> + ti,hecc_int_line = <0>; >> + ti,hecc_version = <1>; > > Versioning in the OF world is done via the compatible. Are the offsets a > per SoC parameter? I'm not sure if it's better to put > the offsets into the driver. > >> + }; >> diff --git a/arch/arm/boot/dts/am3517.dtsi b/arch/arm/boot/dts/am3517.dtsi >> index 5e3f5e8..47bc429 100644 >> --- a/arch/arm/boot/dts/am3517.dtsi >> +++ b/arch/arm/boot/dts/am3517.dtsi >> @@ -25,6 +25,19 @@ >> interrupt-names = "mc"; >> }; >> >> + can0: hecc@5c050000 { >> + compatible = "ti,hecc"; >> + reg = <0x5c050000 0x4000>; >> + interrupts = <24>; >> + ti,hecc_scc_offset = <0>; >> + ti,hecc_scc_ram_offset = <0x3000>; >> + ti,hecc_ram_offset = <0x3000>; >> + ti,hecc_mbx_offset = <0x2000>; >> + ti,hecc_int_line = <0>; >> + ti,hecc_version = <1>; >> + status = "disabled"; >> + }; >> + >> davinci_emac: ethernet@0x5c000000 { >> compatible = "ti,am3517-emac"; >> ti,hwmods = "davinci_emac"; >> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c >> index c08e8ea..f1705d5 100644 >> --- a/drivers/net/can/ti_hecc.c >> +++ b/drivers/net/can/ti_hecc.c >> @@ -875,16 +875,56 @@ static const struct net_device_ops ti_hecc_netdev_ops = { >> .ndo_change_mtu = can_change_mtu, >> }; >> >> +#if defined(CONFIG_OF) >> +static const struct of_device_id ti_hecc_can_dt_ids[] = { >> + { >> + .compatible = "ti,hecc", >> + }, { >> + /* sentinel */ >> + } >> +}; >> +MODULE_DEVICE_TABLE(of, ti_hecc_can_dt_ids); >> +#endif > > Please remove the ifdef, use __maybe_unused instead. > >> + >> +static const struct ti_hecc_platform_data >> +*ti_hecc_can_get_driver_data(struct platform_device *pdev) >> +{ >> + if (pdev->dev.of_node) { >> + struct ti_hecc_platform_data *data; >> + struct device_node *np = pdev->dev.of_node; >> + >> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); >> + if (!data) >> + return NULL; >> + >> + of_property_read_u32(np, "ti,hecc_scc_offset", >> + &data->scc_hecc_offset); >> + of_property_read_u32(np, "ti,hecc_scc_ram_offset", >> + &data->scc_ram_offset); >> + of_property_read_u32(np, "ti,hecc_ram_offset", >> + &data->hecc_ram_offset); >> + of_property_read_u32(np, "ti,hecc_mbx_offset", >> + &data->mbx_offset); >> + of_property_read_u32(np, "ti,hecc_int_line", >> + &data->int_line); >> + of_property_read_u32(np, "ti,hecc_version", >> + &data->version); > > I'm missing error handling here. > >> + return data; >> + } >> + return (const struct ti_hecc_platform_data *) >> + dev_get_platdata(&pdev->dev); > > Is this cast needed? > >> +} >> + >> static int ti_hecc_probe(struct platform_device *pdev) >> { >> struct net_device *ndev = (struct net_device *)0; >> struct ti_hecc_priv *priv; >> - struct ti_hecc_platform_data *pdata; >> + const struct ti_hecc_platform_data *pdata; >> struct resource *mem, *irq; >> void __iomem *addr; >> int err = -ENODEV; >> >> - pdata = dev_get_platdata(&pdev->dev); >> + pdata = ti_hecc_can_get_driver_data(pdev); >> if (!pdata) { >> dev_err(&pdev->dev, "No platform data\n"); >> goto probe_exit; >> @@ -1040,6 +1080,7 @@ static int ti_hecc_resume(struct platform_device *pdev) >> static struct platform_driver ti_hecc_driver = { >> .driver = { >> .name = DRV_NAME, >> + .of_match_table = of_match_ptr(ti_hecc_can_dt_ids), >> }, >> .probe = ti_hecc_probe, >> .remove = ti_hecc_remove, >> > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
