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

From: Philip, Avinash
Date: Wed Jun 12 2013 - 08:12:10 EST

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.


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



èº{.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&—