Re: [PATCH 01/02] sh-pfc: Add r8a73a4 pinmux support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Magnus,

Thanks for the patch.

On Monday 18 March 2013 23:50:39 Magnus Damm wrote:
> From: Magnus Damm <damm@opensource.se>
> 
> 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 <kuninori.morimoto.gx@renesas.com>
> Signed-off-by: Takashi Yoshii <takashi.yoshii.zj@renesas.com>
> Signed-off-by: Magnus Damm <damm@opensource.se>

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