On Thu, Jun 13, 2013 at 11:47:52, Nori, Sekhar wrote: > On 6/12/2013 5:40 PM, Philip, Avinash wrote: > > 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. > > You can always build it by enabling it in menuconfig. Its an ARMv6 > platform so you will have to disable other ARMv5 based platforms from > while enabling it. ARMv5 and ARMv6 cannot co-exist in the same image.
I will try and update. > > > > >> > >> 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. > > Okay, please note these experiments in your cover letter. Its an 18usec > difference. I have no reference to say if that will affect any > application, but it will at least serve as information for someone who > may get affected by it. Ok I will give above details in commit message. > > > > >> 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 > > Where is this patch? This patch is not in mainline and got it from patchwork https://patchwork.kernel.org/patch/97853/ > What is the commit-id if it is in mainline? Where > is the failure log? With tnetv107x_defconfig build is failing arch/arm/mach-davinci/board-tnetv107x-evm.c:282:15: error: 'davinci_timer_init' undeclared here (not in a function) arch/arm/mach-davinci/board-tnetv107x-evm.c:284:15: error: 'davinci_init_late' undeclared here (not in a function) make[1]: *** [arch/arm/mach-davinci/board-tnetv107x-evm.o] Error 1 Following patch fixes the build above breakage diff --git a/arch/arm/mach-davinci/board-tnetv107x-evm.c b/arch/arm/mach-davinci/board-tnetv107x-evm.c index ba79837..4a9c320 100644 --- a/arch/arm/mach-davinci/board-tnetv107x-evm.c +++ b/arch/arm/mach-davinci/board-tnetv107x-evm.c @@ -30,6 +30,7 @@ #include <asm/mach/arch.h> #include <asm/mach-types.h> +#include <mach/common.h> #include <mach/irqs.h> #include <mach/edma.h> #include <mach/mux.h> @@ -147,7 +148,7 @@ static struct davinci_nand_pdata nand_config = { .ecc_bits = 1, }; -static struct davinci_uart_config serial_config __initconst = { +static struct davinci_uart_config serial_config = { .enabled_uarts = BIT(1), }; @@ -245,7 +246,7 @@ static struct ti_ssp_data ssp_config = { }, }; -static struct tnetv107x_device_info evm_device_info __initconst = { +static struct tnetv107x_device_info evm_device_info = { .serial_config = &serial_config, .mmc_config[1] = &mmc_config, /* controller 1 */ .nand_config[0] = &nand_config, /* chip select 0 */ > > > > > So I prefer to leave tnetv107x platform for now. > > I don't think that's acceptable. At least by me. I think 2 options are available 1. Convert gpio-tnetv107x.c to platform driver. This will help in removing gpio references in davinci_soc_info structure. 2. Remove inline gpio api support and start use gpiolib support. I prefer first option. It will help in removing <arch/arm/mach-davinci/include/mach/gpio-davinci.h>. Thanks Avinash > > Thanks, > Sekhar >