| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <56249491.4020009@pengutronix.de>
Date: Mon, 19 Oct 2015 08:58:25 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Heiko Schocher <hs@...x.de>, linux-kernel@...r.kernel.org
Cc: Benoît Cousson <bcousson@...libre.com>,
Anant Gole <anantgole@...com>, devicetree@...r.kernel.org,
netdev@...r.kernel.org, linux-can@...r.kernel.org,
Tony Lindgren <tony@...mide.com>,
Wolfgang Grandegger <wg@...ndegger.com>,
linux-omap@...r.kernel.org, anton.a.glukhov@...il.com
Subject: Re: [PATCH] net, can, ti_hecc: add DT support for the ti,hecc
controller
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.
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@...x.de>
> ---
>
> .../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@...50000 {
> + 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@...50000 {
> + 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@...c000000 {
> 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 |
Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)
Powered by blists - more mailing lists