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

["Raphaël Poggi" <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?
Ok no problem.

> 
> >  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 {
Ok
> 
> >+	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.

Yes, you're right.

> 
> Btw, do you use "tab" for alignment?
Yes, but maybe my vim config is wrong ? (set tabstop=4, set shiftwidth=4, set softtabstop=4)

> 
> >  	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
> 

Thanks for you review,

Raphaël Poggi

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