RE: [PATCH 03/11] gpio: davinci: Modify to platform driver

[Philip, Avinash]

On Wed, Jun 12, 2013 at 13:13:59, Nori, Sekhar wrote:
> On 6/11/2013 6:25 PM, Philip, Avinash wrote:
> 
> > On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote:
> 
> >> On 5/22/2013 12:40 PM, Philip Avinash wrote:
> 
> >>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
> >>>  		gpiochip_add(&ctlrs[i].chip);
> >>>  	}
> >>>  
> >>> -	soc_info->gpio_ctlrs = ctlrs;
> >>
> >>> -	soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
> >>
> >> You drop setting gpio_ctlrs_num here and don't introduce it anywhere
> >> else in the patchset so in effect you render the inline gpio get/set API
> >> useless. Looks like this initialization should be moved to platform code?
> > 
> > With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API
> > Has no more dependency on soc_info->gpio_ctlrs_num.
> 
> With this series, you have removed support for inline gpio get/set API.
> I see that the inline functions are still available for use on
> tnetv107x. I wonder why it is important to keep these for tnetv107x when
> not necessary for other DaVinci devices?

To support DT boot in da850, gpio davinci has to be converted to a driver and
remove references to davinci_soc_info from driver. But tnetv107x has 
separate GPIO driver and reference to davinci_soc_info can also be removed.
But I didn't found defconfig support for tnetv107x platforms and left untouched.
As I will not be able to build and test on tnetv107x, I prefer to not touch it.

> 
> When you are removing this feature, please note it prominently in your
> cover letter and patch description.

Ok

> Also, please provide some data on 
> how the latency now compares to that of inline access earlier. This is
> important especially for the read.

I am not sure whether I understood correctly or not? Meanwhile I had done
an experiment by reading printk timing before and after gpio_get_value from
a test module. I think this will help in software latency for inline access over
gpiolib specific access.

gpio_get_value latency testing code

+
+       local_irq_disable();
+       pr_emerg("%d %x\n", __LINE__, jiffies);
+       gpio_get_value(gpio_num);
+       pr_emerg("%d %x\n", __LINE__, jiffies);
+       local_irq_enable();

inline gpio functions with interrupt disabled
[   29.734337] 81 ffff966c
[   29.736847] 83 ffff966c

Time diff = 	0.00251

gpio library with interrupt disabled

[  272.876763] 81 fffff567
[  272.879291] 83 fffff567

Time diff =	0.002528

Inline function takes less time as expected.

> For the writes, gpio clock will
> mostly determine how soon the value changes on the pin.
> 
> I am okay with removing the inline access feature. It helps simplify
> code and most arm machines don't use them. I would just like to see some
> data for justification as this can be seen as feature regression. Also,
> if we are removing it, its better to also remove it completely and get
> the LOC savings instead of just stopping its usage.

I see build failure with below patch for tnetv107x
[v6,6/6] Davinci: tnetv107x default configuration 

So I prefer to leave tnetv107x platform for now.

Thanks
Avinash

> 

èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—