Re: [PATCH v5 5/7] ARM: davinci: i2c: add OF support

[Heiko Schocher <hs-ynQEQJNshbs@public.gmane.org>]

Hello Sekhar,

On 13.07.2012 15:57, Sekhar Nori wrote:
> Hi Heiko,
>
> On 5/30/2012 3:49 PM, Heiko Schocher wrote:
>> add of support for the davinci i2c driver.
>>
>> Signed-off-by: Heiko Schocher<hs-ynQEQJNshbs@public.gmane.org>
>> Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
>> Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
>> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Cc: Ben Dooks<ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>
>> Cc: Wolfram Sang<w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
>> Cc: Grant Likely<grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>> Cc: Sekhar Nori<nsekhar-l0cyMroinI0@public.gmane.org>
>> Cc: Wolfgang Denk<wd-ynQEQJNshbs@public.gmane.org>
>> Cc: Sylwester Nawrocki<s.nawrocki-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
[...]
>> diff --git a/Documentation/devicetree/bindings/arm/davinci/i2c.txt b/Documentation/devicetree/bindings/arm/davinci/i2c.txt
>> new file mode 100644
>> index 0000000..e98a025
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/davinci/i2c.txt
[...]
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index a76d85f..4e7a966 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -38,9 +38,12 @@
>>   #include<linux/slab.h>
>>   #include<linux/cpufreq.h>
>>   #include<linux/gpio.h>
>> +#include<linux/of_i2c.h>
>> +#include<linux/of_device.h>
>>
>>   #include<mach/hardware.h>
>>   #include<mach/i2c.h>
>> +#include<mach/mux.h>
>
> Seems like a stray change. I was able to build and use just fine
> without this include.

Hups. Good catch! Removed.

>>
>>   /* ----- global defines ----------------------------------------------- */
>>
>> @@ -114,6 +117,7 @@ struct davinci_i2c_dev {
>>   	struct completion	xfr_complete;
>>   	struct notifier_block	freq_transition;
>>   #endif
>> +	struct davinci_i2c_platform_data *pdata;
>>   };
>>
>>   /* default platform data to use if not supplied in the platform_device */
>> @@ -149,13 +153,22 @@ static void generic_i2c_clock_pulse(unsigned int scl_pin)
>>   	}
>>   }
>>
>> +static struct davinci_i2c_platform_data
>> +		*i2c_get_plattformdata(struct davinci_i2c_dev *dev)
>> +{
>> +	if (dev->dev->platform_data == NULL)
>> +		return dev->pdata;
>> +
>> +	return dev->dev->platform_data;
>> +}
>
> It seems to me that if we setup the newly introduced dev->pdata
> member correctly once in probe, there should not be a need for this
> function. We can also eliminate multiple checks for pdata in code.
> See below for more.

Ok.

>> +
>>   /* This routine does i2c bus recovery as specified in the
>>    * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>>    */
>>   static void i2c_recover_bus(struct davinci_i2c_dev *dev)
>>   {
>>   	u32 flag = 0;
>> -	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
>> +	struct davinci_i2c_platform_data *pdata = i2c_get_plattformdata(dev);
>>
>>   	dev_err(dev->dev, "initiating i2c bus recovery\n");
>>   	/* Send NACK to the slave */
>> @@ -187,7 +200,7 @@ static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
>>
>>   static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>>   {
>> -	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
>> +	struct davinci_i2c_platform_data *pdata = i2c_get_plattformdata(dev);
>>   	u16 psc;
>>   	u32 clk;
>>   	u32 d;
>> @@ -235,7 +248,7 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>>    */
>>   static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>>   {
>> -	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
>> +	struct davinci_i2c_platform_data *pdata = i2c_get_plattformdata(dev);
>>
>>   	if (!pdata)
>>   		pdata =&davinci_i2c_platform_data_default;
>> @@ -308,7 +321,7 @@ static int
>>   i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>   {
>>   	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
>> -	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
>> +	struct davinci_i2c_platform_data *pdata = i2c_get_plattformdata(dev);
>>   	u32 flag;
>>   	u16 w;
>>   	int r;
>> @@ -635,6 +648,12 @@ static struct i2c_algorithm i2c_davinci_algo = {
>>   	.functionality	= i2c_davinci_func,
>>   };
>>
>> +static const struct of_device_id davinci_i2c_of_match[] = {
>> +	{.compatible = "ti,davinci-i2c", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, davinci_i2c_of_match);
>> +
>>   static int davinci_i2c_probe(struct platform_device *pdev)
>>   {
>>   	struct davinci_i2c_dev *dev;
>> @@ -676,6 +695,25 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>   	dev->irq = irq->start;
>>   	platform_set_drvdata(pdev, dev);
>>
>> +	if ((dev->dev->platform_data == NULL)&&
>> +		(pdev->dev.of_node)) {
>> +		u32 prop;
>> +
>> +		dev->pdata = devm_kzalloc(&pdev->dev,
>> +			sizeof(struct davinci_i2c_platform_data), GFP_KERNEL);
>> +		if (!dev->pdata) {
>> +			r = -ENOMEM;
>> +			goto err_free_mem;
>> +		}
>> +		memcpy(dev->pdata,&davinci_i2c_platform_data_default,
>> +			sizeof(struct davinci_i2c_platform_data));
>> +		if (!of_property_read_u32(pdev->dev.of_node, "clock-frequency",
>> +			&prop))
>> +			dev->pdata->bus_freq = prop / 1000;
>> +		if (!of_property_read_u32(pdev->dev.of_node, "bus-delay",
>> +			&prop))
>> +			dev->pdata->bus_delay = prop;
>
> You are leaving out two other platform data members (the gpio pins
> corresponding to data and clock) from DT data. We should be able to
> get that information from DT too, right?

Yes, but I had not ported the GPIO driver to OF ...

> So, I took this patch and tried to see if pdata maintenance can be
> simplified and came with the diff below. Can you have a look and see
> if this makes sense? I tested this using i2cdetect both with and
> without DT.

Tested your patch on the enbw_cmc board with a LM75 on the enbw_cmc
board, works fine. Can I post a "v6" of my patch, merged with your
patch below and your Signed-off?

> ---8<---
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 16d7645..d07c207 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -153,22 +153,13 @@ static void generic_i2c_clock_pulse(unsigned int scl_pin)
>   	}
>   }
>
> -static struct davinci_i2c_platform_data
> -		*i2c_get_plattformdata(struct davinci_i2c_dev *dev)
> -{
> -	if (dev->dev->platform_data == NULL)
> -		return dev->pdata;
> -
> -	return dev->dev->platform_data;
> -}
> -
>   /* This routine does i2c bus recovery as specified in the
>    * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
>    */
>   static void i2c_recover_bus(struct davinci_i2c_dev *dev)
>   {
>   	u32 flag = 0;
> -	struct davinci_i2c_platform_data *pdata = i2c_get_plattformdata(dev);
> +	struct davinci_i2c_platform_data *pdata = dev->pdata;
>
>   	dev_err(dev->dev, "initiating i2c bus recovery\n");
>   	/* Send NACK to the slave */
> @@ -176,8 +167,7 @@ static void i2c_recover_bus(struct davinci_i2c_dev *dev)
>   	flag |=  DAVINCI_I2C_MDR_NACK;
>   	/* write the data into mode register */
>   	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> -	if (pdata)
> -		generic_i2c_clock_pulse(pdata->scl_pin);
> +	generic_i2c_clock_pulse(pdata->scl_pin);
>   	/* Send STOP */
>   	flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>   	flag |= DAVINCI_I2C_MDR_STP;
> @@ -200,7 +190,7 @@ static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
>
>   static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>   {
> -	struct davinci_i2c_platform_data *pdata = i2c_get_plattformdata(dev);
> +	struct davinci_i2c_platform_data *pdata = dev->pdata;
>   	u16 psc;
>   	u32 clk;
>   	u32 d;
> @@ -248,10 +238,7 @@ static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
>    */
>   static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>   {
> -	struct davinci_i2c_platform_data *pdata = i2c_get_plattformdata(dev);
> -
> -	if (!pdata)
> -		pdata =&davinci_i2c_platform_data_default;
> +	struct davinci_i2c_platform_data *pdata = dev->pdata;
>
>   	/* put I2C into reset */
>   	davinci_i2c_reset_ctrl(dev, 0);
> @@ -321,13 +308,11 @@ static int
>   i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>   {
>   	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
> -	struct davinci_i2c_platform_data *pdata = i2c_get_plattformdata(dev);
> +	struct davinci_i2c_platform_data *pdata = dev->pdata;
>   	u32 flag;
>   	u16 w;
>   	int r;
>
> -	if (!pdata)
> -		pdata =&davinci_i2c_platform_data_default;
>   	/* Introduce a delay, required for some boards (e.g Davinci EVM) */
>   	if (pdata->bus_delay)
>   		udelay(pdata->bus_delay);
> @@ -693,10 +678,10 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>   #endif
>   	dev->dev = get_device(&pdev->dev);
>   	dev->irq = irq->start;
> +	dev->pdata = dev->dev->platform_data;
>   	platform_set_drvdata(pdev, dev);
>
> -	if ((dev->dev->platform_data == NULL)&&
> -		(pdev->dev.of_node)) {
> +	if (!dev->pdata&&  pdev->dev.of_node) {
>   		u32 prop;
>
>   		dev->pdata = devm_kzalloc(&pdev->dev,
> @@ -713,7 +698,10 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>   		if (!of_property_read_u32(pdev->dev.of_node, "bus-delay",
>   			&prop))
>   			dev->pdata->bus_delay = prop;
> +	} else if (!dev->pdata) {
> +		dev->pdata =&davinci_i2c_platform_data_default;
>   	}
> +
>   	dev->clk = clk_get(&pdev->dev, NULL);
>   	if (IS_ERR(dev->clk)) {
>   		r = -ENODEV;
>
>

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany