Re: [PATCH V2 1/2] i2c: at91: add support of device tree

From: Bo Shen <voice.shen@atmel.com>
To: "Raphaël Poggi" <poggi.raph@gmail.com>, barebox@lists.infradead.org
Subject: Re: [PATCH V2 1/2] i2c: at91: add support of device tree
Date: Mon, 4 Aug 2014 17:14:30 +0800	[thread overview]
Message-ID: <53DF4EF6.8050408@atmel.com> (raw)
In-Reply-To: <1407141112-3868-2-git-send-email-poggi.raph@gmail.com>

Hi Raphaël Poggi,

On 08/04/2014 04:31 PM, Raphaël Poggi wrote:
> Signed-off-by: Raphaël Poggi <poggi.raph@gmail.com>
> ---
>   drivers/i2c/busses/i2c-at91.c |   66 ++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 58 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 399f6a9..944a8b3 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -346,6 +346,12 @@ static struct at91_twi_pdata at91sam9g10_config = {
>   	.has_unre_flag = false,
>   };
>
> +static struct at91_twi_pdata at91sam9x5_config = {
> +	.clk_max_div = 7,
> +	.clk_offset = 4,
> +	.has_unre_flag = false,
> +};
> +

Can you add this also for non device tree?

>   static struct platform_device_id at91_twi_devtypes[] = {
>   	{
>   		.name = "i2c-at91rm9200",
> @@ -367,20 +373,63 @@ static struct platform_device_id at91_twi_devtypes[] = {
>   	}
>   };
>
> +static struct of_device_id at91_twi_dt_ids[] = {
> +	{
> +		.compatible = "atmel,at91rm9200-i2c",
> +		.data = (unsigned long) &at91rm9200_config,
> +	} , {
> +		.compatible = "atmel,at91sam9260-i2c",
> +		.data = (unsigned long) &at91sam9260_config,
> +	} , {
> +		.compatible = "atmel,at91sam9261-i2c",
> +		.data = (unsigned long) &at91sam9261_config,
> +	} , {
> +		.compatible = "atmel,at91sam9g20-i2c",
> +		.data = (unsigned long) &at91sam9g20_config,
> +	} , {
> +		.compatible = "atmel,at91sam9g10-i2c",
> +		.data = (unsigned long) &at91sam9g10_config,
> +	}, {
> +		.compatible = "atmel,at91sam9x5-i2c",
> +		.data = (unsigned long) &at91sam9x5_config,
> +	}, {
> +		/* sentinel */
> +	}
> +};
> +
> +static struct at91_twi_pdata *at91_twi_get_driver_data(struct device_d *dev)
> +{
> +    struct at91_twi_pdata *i2c_data = NULL;
> +    int rc;
> +
> +    if (dev->device_node) {
> +	const struct of_device_id *match;
> +	match = of_match_node(at91_twi_dt_ids, dev->device_node);
> +	if (!match)
> +	    i2c_data = NULL;
> +	else
> +	    i2c_data = (struct at91_twi_pdata *)match->data;
> +    }
> +    else {

Maybe better if: } else {

> +	rc = dev_get_drvdata(dev, (unsigned long *)&i2c_data);
> +	if (rc)
> +	    i2c_data = NULL;
> +    }
> +
> +    return i2c_data;
> +}
> +
>   static int at91_twi_probe(struct device_d *dev)
>   {
>   	struct at91_twi_dev *i2c_at91;
> -	struct at91_twi_pdata *i2c_data;
> -	int rc;
> +	int rc = 0;
>   	u32 bus_clk_rate;
>
>   	i2c_at91 = xzalloc(sizeof(struct at91_twi_dev));
>
> -	rc = dev_get_drvdata(dev, (unsigned long *)&i2c_data);
> -	if (rc)
> -		goto out_free;
> -
> -	i2c_at91->pdata = i2c_data;
> +	i2c_at91->pdata = at91_twi_get_driver_data(dev);
> +	if (!i2c_at91->pdata)
> +	    goto out_free;

Maybe add error information will be better.

Btw, do you use "tab" for alignment?

>   	i2c_at91->base = dev_request_mem_region(dev, 0);
>   	if (!i2c_at91->base) {
> @@ -389,7 +438,7 @@ static int at91_twi_probe(struct device_d *dev)
>   		goto out_free;
>   	}
>
> -	i2c_at91->clk = clk_get(dev, "twi_clk");
> +	i2c_at91->clk = clk_get(dev, NULL);
>   	if (IS_ERR(i2c_at91->clk)) {
>   		dev_err(dev, "no clock defined\n");
>   		rc = -ENODEV;
> @@ -429,6 +478,7 @@ static struct driver_d at91_twi_driver = {
>   	.name		= "at91-twi",
>   	.probe		= at91_twi_probe,
>   	.id_table	= at91_twi_devtypes,
> +	.of_compatible	= DRV_OF_COMPAT(at91_twi_dt_ids),
>   };
>   device_platform_driver(at91_twi_driver);
>
>

Best Regards,
Bo Shen

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox