From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Date: Thu, 21 Mar 2013 12:19:55 +0000 Subject: Re: [PATCH 01/02] sh-pfc: Add r8a73a4 pinmux support Message-Id: <2774524.M2sxvGcfqv@avalon> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: linux-sh@vger.kernel.org Hi Magnus, Thanks for the patch. On Monday 18 March 2013 23:50:39 Magnus Damm wrote: > From: Magnus Damm > > Add initial PFC support for the r8a73a4 SoC. > > At this point only GPIO interface is supported, move to > newer interfaces planned as incremental changes. I'm fine with splitting PFC support for the r8a73a4 into a first patch that implements the old API and incremental changes that move to the new API, to making partial backporting easier. However, to avoid increasing my already high work load related to the PFC driver, I don't want to push this patch to mainline without the incremental changes that convert the driver to the pinctrl API. Could you thus please submit a v2 that will include pinctrl support ? (I believe this summarizes the discussion we had yesterday, please let me know if there had been any misunderstanding). > Original authors are Morimoto-san with help from Yoshii-san, > thanks to them for the heavy lifting. Adjusted by Magnus > to work together with updated code in drivers/pinctrl. > > Signed-off-by: Kuninori Morimoto > Signed-off-by: Takashi Yoshii > Signed-off-by: Magnus Damm The code looks OK to me. I don't have a copy of the r8a73a4 datasheet so I can't verify all the details. > --- > > Written against "next" renesas.git 811689afc214564c4a5f238ecf4d8bdc0e52b615 > Requires "r8a73a4.h" provided by > [PATCH 00/04] ARM: shmobile: r8a73a4 SoC support V2 > > arch/arm/mach-shmobile/include/mach/r8a73a4.h | 918 ++++++++ > drivers/pinctrl/sh-pfc/Kconfig | 5 > drivers/pinctrl/sh-pfc/Makefile | 1 > drivers/pinctrl/sh-pfc/core.c | 3 > drivers/pinctrl/sh-pfc/core.h | 1 > drivers/pinctrl/sh-pfc/pfc-r8a73a4.c | 2826 ++++++++++++++++++++++ > 6 files changed, 3754 insertions(+) Could you please split this in two patches, one for arch/arm and one for drivers/pinctrl ? The following comments apply to the pinctrl API only, they're hints for the v2 mentioned above. > --- 0006/arch/arm/mach-shmobile/include/mach/r8a73a4.h > +++ work/arch/arm/mach-shmobile/include/mach/r8a73a4.h 2013-03-18 > 22:38:11.000000000 +0900 @@ -1,6 +1,924 @@ > #ifndef __ASM_R8A73A4_H__ > #define __ASM_R8A73A4_H__ > > +/* > + * Pin Function Controller: > + * GPIO_FN_xx - GPIO used to select pin function > + * GPIO_PORTxx - GPIO mapped to real I/O pin on CPU > + */ > +enum { > + > + /* PORT */ > + GPIO_PORT0, GPIO_PORT1, GPIO_PORT2, GPIO_PORT3, GPIO_PORT4, > + GPIO_PORT5, GPIO_PORT6, GPIO_PORT7, GPIO_PORT8, GPIO_PORT9, [snip] The whole enum should be removed when moving to the pinctrl API. Port numbers will be used directly instead of the GPIO_PORTx values. > --- /dev/null > +++ work/drivers/pinctrl/sh-pfc/pfc-r8a73a4.c 2013-03-18 22:44:53.000000000 [snip] > +const struct sh_pfc_soc_info r8a73a4_pinmux_info = { > + .name = "r8a73a4_pfc", > + > + .input = { PINMUX_INPUT_BEGIN, PINMUX_INPUT_END }, > + .input_pu = { PINMUX_INPUT_PULLUP_BEGIN, PINMUX_INPUT_PULLUP_END }, > + .input_pd = { PINMUX_INPUT_PULLDOWN_BEGIN, PINMUX_INPUT_PULLDOWN_END }, PU/PD should be handled as in sh-pfc: sh73a0: Add bias (pull-up/down) pinconf support sh-pfc: sh73a0: Remove pull-up function GPIOS > + .output = { PINMUX_OUTPUT_BEGIN, PINMUX_OUTPUT_END }, > + .function = { PINMUX_FUNCTION_BEGIN, PINMUX_FUNCTION_END }, > + > + .pins = pinmux_pins, > + .nr_pins = ARRAY_SIZE(pinmux_pins), GPIO numbers are sparse, don't forget to add ranges as in ARM: shmobile: sh73a0: Support sparse GPIO numbers > + .func_gpios = pinmux_func_gpios, > + .nr_func_gpios = ARRAY_SIZE(pinmux_func_gpios), You should add functions for all the functions used by board code. See for instance sh-pfc: sh7372: Add SDHCI and MMCIF pin groups and functions Finally you should remove support for function GPIOs as in the following patches (not in Simon's tree yet, they're available from my git tree at git://linuxtv.org/pinchartl/fbdev.git pinmux/3.9/gpio) sh-pfc: r8a7779: Remove function GPIO sh-pfc: r8a7779: Don't use GPIO enum entries ARM: shmobile: r8a7779: Remove all GPIOs > + .cfg_regs = pinmux_config_regs, > + .data_regs = pinmux_data_regs, > + > + .gpio_data = pinmux_data, > + .gpio_data_size = ARRAY_SIZE(pinmux_data), > +}; -- Regards, Laurent Pinchart