Re: [PATCH 1/3] can: ti_hecc: Add DT support for TI HECC module

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 3978 bytes --]

On 10/04/2015 09:59 PM, Anton Glukhov wrote:
> These patch set adds device tree support for TI HECC module.
> 
> Signed-off-by: Anton Glukhov <anton.a.glukhov@gmail.com>
> ---
>  drivers/net/can/ti_hecc.c | 56 +++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c
> index cf345cb..c1a89fd 100644
> --- a/drivers/net/can/ti_hecc.c
> +++ b/drivers/net/can/ti_hecc.c
> @@ -46,6 +46,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/clk.h>
>  #include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
>  
>  #include <linux/can/dev.h>
>  #include <linux/can/error.h>
> @@ -875,19 +877,64 @@ static const struct net_device_ops ti_hecc_netdev_ops = {
>  	.ndo_change_mtu		= can_change_mtu,
>  };
>  
> +#if defined(CONFIG_OF)

Have you actually tested to compile without CONFIG_OF? I think the
assignment of "of_match_table = ti_hecc_dt_ids" will fail. Please remove
the ifdef here. If it's still possible to compile without CONFIG_OF,
please use of_match_ptr and mark the struct as __maybe_unused.

> +static const struct of_device_id ti_hecc_dt_ids[] = {
> +	{
> +		.compatible = "ti,am35x-hecc",

AFAIK it's considered best practise not to use a generic name "am35x"
but to add the oldest SoC that support this IP core.

> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ti_hecc_dt_ids);
> +#endif
> +
> +static struct ti_hecc_platform_data *hecc_parse_dt(struct device *dev)
> +{
> +	struct ti_hecc_platform_data *pdata;
> +	struct device_node *np = dev->of_node;
> +
> +	pdata = devm_kzalloc(dev, sizeof(struct ti_hecc_platform_data), GFP_KERNEL);
> +	if (!pdata)
> +		return ERR_PTR(-ENOMEM);
> +
> +	if (of_property_read_u32(np, "ti,scc-ram-offset", &pdata->scc_ram_offset)) {
> +		dev_err(dev, "Missing scc-ram-offset property in the DT.\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	if (of_property_read_u32(np, "ti,hecc-ram-offset", &pdata->hecc_ram_offset)) {
> +		dev_err(dev, "Missing hecc-ram-offset property in the DT.\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +	if (of_property_read_u32(np, "ti,mbx-offset", &pdata->mbx_offset)) {
> +		dev_err(dev, "Missing mbx-offset property in the DT.\n");
> +		return ERR_PTR(-EINVAL);
> +	}
> +	if (of_property_read_u32(dev->of_node, "ti,int-line", &pdata->int_line)) {

bool?

> +		pdata->int_line = 0;

This makes no sense, as pdata is initialized as "0" anyways.

> +	}
> +
> +	return pdata;
> +}
> +
>  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;
> +	struct ti_hecc_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct device_node *np = pdev->dev.of_node;
>  	struct resource *mem, *irq;
>  	void __iomem *addr;
>  	int err = -ENODEV;
>  
> -	pdata = dev_get_platdata(&pdev->dev);
> +	if (!pdata && np) {
> +		pdata = hecc_parse_dt(&pdev->dev);
> +		if (IS_ERR(pdata))
> +			return PTR_ERR(pdata);
> +	}
> +
>  	if (!pdata) {
> -		dev_err(&pdev->dev, "No platform data\n");
> -		goto probe_exit;
> +		dev_err(&pdev->dev, "Platform data missing\n");
> +		return -EINVAL;
>  	}
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1040,6 +1087,7 @@ static int ti_hecc_resume(struct platform_device *pdev)
>  static struct platform_driver ti_hecc_driver = {
>  	.driver = {
>  		.name    = DRV_NAME,
> +		.of_match_table = ti_hecc_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   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 455 bytes --]