Date:	Fri, 23 Jan 2015 00:37:33 +0200
From:	Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:	Niklas Söderlund <niso@....se>
Cc:	linux-kernel@...r.kernel.org, linus.walleij@...aro.org,
	devicetree@...r.kernel.org, linux-sh@...r.kernel.org,
	magnus.damm@...il.com
Subject: Re: [PATCH v2 2/4] sh-pfc: Add emev2 pinmux support

Hi Niklas,

Thank you for the patch.

I see that you've quickly taken my remarks into account, great work. Please 
see below for a couple more comments.

On Sunday 18 January 2015 13:20:02 Niklas Söderlund wrote:
> Add PFC support for the EMMA Mobile EV2 SoC including pin groups for
> on-chip devices.
> 
> Signed-off-by: Niklas Söderlund <niso@....se>
> ---
>  .../bindings/pinctrl/renesas,pfc-pinctrl.txt       |    1 +
>  drivers/pinctrl/sh-pfc/Kconfig                     |    5 +
>  drivers/pinctrl/sh-pfc/Makefile                    |    1 +
>  drivers/pinctrl/sh-pfc/core.c                      |    9 +
>  drivers/pinctrl/sh-pfc/core.h                      |    1 +
>  drivers/pinctrl/sh-pfc/pfc-emev2.c                 | 1774 +++++++++++++++++
>  6 files changed, 1791 insertions(+)
>  create mode 100644 drivers/pinctrl/sh-pfc/pfc-emev2.c

[snip]

> diff --git a/drivers/pinctrl/sh-pfc/pfc-emev2.c
> b/drivers/pinctrl/sh-pfc/pfc-emev2.c new file mode 100644
> index 0000000..2d2ac21
> --- /dev/null
> +++ b/drivers/pinctrl/sh-pfc/pfc-emev2.c
> @@ -0,0 +1,1774 @@
> +/*
> + * Pin Function Controller Support
> + *
> + * Copyright (C) 2014 Niklas Söderlund

Happy new year ;-)

[snip]

> +#define EMEV_MUX_PIN(name, pin, mark) \
> +	static const unsigned int name##_pins[] = { pin }; \
> +	static const unsigned int name##_mux[] = { mark##_MARK }
> +
> +/* = [ System ] =========== */
> +EMEV_MUX_PIN(err_rst_reqb, 3, ERR_RST_REQB);
> +EMEV_MUX_PIN(ref_clko, 4, REF_CLKO);
> +EMEV_MUX_PIN(ext_clki, 5, EXT_CLKI);
> +EMEV_MUX_PIN(lowpwr, 154, LOWPWR);
> +
> +/* = [ External Memory] === */

I don't have much information on the external memory bus, would it make sense 
to group some of the pins ? I expect that at AB_AD[15:0] will always be 
needed. Maybe AB_RDB and AB_WRB too, unless someone want to interface with 
read-only or write-only memory maybe ?

> +EMEV_MUX_PIN(ab_clk, 68, AB_CLK);
> +EMEV_MUX_PIN(ab_csb0, 69, AB_CSB0);
> +EMEV_MUX_PIN(ab_csb1, 70, AB_CSB1);
> +EMEV_MUX_PIN(ab_csb2, 71, AB_CSB2);
> +EMEV_MUX_PIN(ab_csb3, 72, AB_CSB3);
> +EMEV_MUX_PIN(ab_rdb, 73, AB_RDB);
> +EMEV_MUX_PIN(ab_wrb, 74, AB_WRB);
> +EMEV_MUX_PIN(ab_wait, 75, AB_WAIT);
> +EMEV_MUX_PIN(ab_adv, 76, AB_ADV);
> +EMEV_MUX_PIN(ab_ad0, 77, AB_AD0);
> +EMEV_MUX_PIN(ab_ad1, 78, AB_AD1);
> +EMEV_MUX_PIN(ab_ad2, 79, AB_AD2);
> +EMEV_MUX_PIN(ab_ad3, 80, AB_AD3);
> +EMEV_MUX_PIN(ab_ad4, 81, AB_AD4);
> +EMEV_MUX_PIN(ab_ad5, 82, AB_AD5);
> +EMEV_MUX_PIN(ab_ad6, 83, AB_AD6);
> +EMEV_MUX_PIN(ab_ad7, 84, AB_AD7);
> +EMEV_MUX_PIN(ab_ad8, 85, AB_AD8);
> +EMEV_MUX_PIN(ab_ad9, 86, AB_AD9);
> +EMEV_MUX_PIN(ab_ad10, 87, AB_AD10);
> +EMEV_MUX_PIN(ab_ad11, 88, AB_AD11);
> +EMEV_MUX_PIN(ab_ad12, 89, AB_AD12);
> +EMEV_MUX_PIN(ab_ad13, 90, AB_AD13);
> +EMEV_MUX_PIN(ab_ad14, 91, AB_AD14);
> +EMEV_MUX_PIN(ab_ad15, 92, AB_AD15);
> +EMEV_MUX_PIN(ab_a17, 93, AB_A17);
> +EMEV_MUX_PIN(ab_a18, 94, AB_A18);
> +EMEV_MUX_PIN(ab_a19, 95, AB_A19);
> +EMEV_MUX_PIN(ab_a20, 96, AB_A20);
> +EMEV_MUX_PIN(ab_a21, 97, AB_A21);
> +EMEV_MUX_PIN(ab_a22, 98, AB_A22);
> +EMEV_MUX_PIN(ab_a23, 99, AB_A23);
> +EMEV_MUX_PIN(ab_a24, 100, AB_A24);
> +EMEV_MUX_PIN(ab_a25, 101, AB_A25);
> +EMEV_MUX_PIN(ab_a26, 102, AB_A26);
> +EMEV_MUX_PIN(ab_a27, 103, AB_A27);
> +EMEV_MUX_PIN(ab_a28, 104, AB_A28);
> +EMEV_MUX_PIN(ab_ben0, 103, AB_BEN0);
> +EMEV_MUX_PIN(ab_ben1, 104, AB_BEN1);
> +
> +/* = [ CAM ] ============== */
> +static const unsigned int cam_ctrl_pins[] = {
> +	/* CLKO, CLKI, VS, HS */

The datasheet mentions the CLKO signal but doesn't document it further. I 
believe it's optional, in which case it should be moved to a separate group. 
All the other signals (control and data) can be grouped together, as they 
can't be used separately.

> +	131, 132, 133, 134,
> +};
> +static const unsigned int cam_ctrl_mux[] = {
> +	CAM_CLKO_MARK, CAM_CLKI_MARK, CAM_VS_MARK, CAM_HS_MARK,
> +};
> +
> +static const unsigned int cam_data_pins[] = {
> +	/* CAM_YUV[0:7] */
> +	135, 136, 137, 138,
> +	139, 140, 141, 142,
> +};
> +static const unsigned int cam_data_mux[] = {
> +	CAM_YUV0_MARK, CAM_YUV1_MARK, CAM_YUV2_MARK, CAM_YUV3_MARK,
> +	CAM_YUV4_MARK, CAM_YUV5_MARK, CAM_YUV6_MARK, CAM_YUV7_MARK,
> +};

[snip]

> +/* = [ JTAG ] ============= */
> +EMEV_MUX_PIN(jt_sel, 2, JT_SEL);
> +EMEV_MUX_PIN(jt_tdo, 151, JT_TDO);
> +EMEV_MUX_PIN(jt_tdoen, 152, JT_TDOEN);

Can the JTAG port signals be used independently ? If they always need to be 
used together you can combine the pins into a single group.

[snip]

> +/* = [ TP33 ] ============= */

Can the trace port control and data signals be used independently ? If they 
always need to be used together you can combine the pins into a single group.

> +static const unsigned int tp33_ctrl_pins[] = {
> +	/* CLK, CTRL */
> +	38, 39,
> +};
> +static const unsigned int tp33_ctrl_mux[] = {
> +	TP33_CLK_MARK, TP33_CTRL_MARK,
> +};
> +
> +static const unsigned int tp33_data_pins[] = {
> +	/* TP33_DATA[0:15] */
> +	40, 41, PIN_NUMBER(2, 17), PIN_NUMBER(3, 17),
> +	PIN_NUMBER(4, 17), PIN_NUMBER(2, 16), PIN_NUMBER(3, 16),
> +	PIN_NUMBER(4, 16),
> +	42, 43, PIN_NUMBER(2, 15), PIN_NUMBER(3, 15),
> +	PIN_NUMBER(4, 15), PIN_NUMBER(2, 14), PIN_NUMBER(3, 14),
> +	PIN_NUMBER(4, 14),
> +};
> +static const unsigned int tp33_data_mux[] = {
> +	TP33_DATA0_MARK, TP33_DATA1_MARK, TP33_DATA2_MARK, TP33_DATA3_MARK,
> +	TP33_DATA4_MARK, TP33_DATA5_MARK, TP33_DATA6_MARK, TP33_DATA7_MARK,
> +	TP33_DATA8_MARK, TP33_DATA9_MARK, TP33_DATA10_MARK, TP33_DATA11_MARK,
> +	TP33_DATA12_MARK, TP33_DATA13_MARK, TP33_DATA14_MARK, TP33_DATA15_MARK,
> +};

[snip]

> +/* = [ USI0 ] ============== */
> +EMEV_MUX_PIN(usi0_cs1, 105, USI0_CS1);
> +EMEV_MUX_PIN(usi0_cs2, 106, USI0_CS2);
> +EMEV_MUX_PIN(usi0_cs3, 115, USI0_CS3);
> +EMEV_MUX_PIN(usi0_cs4, 116, USI0_CS4);
> +EMEV_MUX_PIN(usi0_cs5, 117, USI0_CS5);
> +EMEV_MUX_PIN(usi0_cs6, 118, USI0_CS6);
> +
> +/* = [ USI1 ] ============== */
> +static const unsigned int usi1_ctrl_pins[] = {

Those are data pins, shouldn't the group be named usi1_data ?

> +	/* DI, DO*/
> +	107, 108,
> +};
> +static const unsigned int usi1_ctrl_mux[] = {
> +	USI1_DI_MARK, USI1_DO_MARK,
> +};
> +
> +/* = [ USI2 ] ============== */
> +static const unsigned int usi2_ctrl_pins[] = {

Same here, although there's also a clock pin.

Same comment for usi[345]_ctrl* below.

> +	/* CLK, DI, DO*/
> +	109, 110, 111,
> +};
> +static const unsigned int usi2_ctrl_mux[] = {
> +	USI2_CLK_MARK, USI2_DI_MARK, USI2_DO_MARK,
> +};

[snip]

> +/* = [ YUV ] ============== */

I believe the clock input is optional, but I think the clock output, data and 
synchronization signals should be part of the same group as they can't be used 
separately. You could also declare those groups as part of the LCD function, 
as the pins are used for LCD output.

> +EMEV_MUX_PIN(yuv3_clk_o, 18, YUV3_CLK_O);
> +EMEV_MUX_PIN(yuv3_clk_i, 20, YUV3_CLK_I);
> +
> +static const unsigned int yuv3_sync_pins[] = {
> +	/* HS, VS, DE */
> +	21, 22, 23,
> +};
> +static const unsigned int yuv3_sync_mux[] = {
> +	YUV3_HS_MARK, YUV3_VS_MARK, YUV3_DE_MARK,
> +};
> +
> +static const unsigned int yuv3_data_pins[] = {
> +	/* YUV3_D[0:15] */
> +	40, 41, PIN_NUMBER(2, 17), PIN_NUMBER(3, 17),
> +	PIN_NUMBER(4, 17), PIN_NUMBER(2, 16), PIN_NUMBER(3, 16),
> +	PIN_NUMBER(4, 16),
> +	42, 43, PIN_NUMBER(2, 15), PIN_NUMBER(3, 15),
> +	PIN_NUMBER(4, 15), PIN_NUMBER(2, 14), PIN_NUMBER(3, 14),
> +	PIN_NUMBER(4, 14),
> +};
> +static const unsigned int yuv3_data_mux[] = {
> +	YUV3_D0_MARK, YUV3_D1_MARK, YUV3_D2_MARK, YUV3_D3_MARK,
> +	YUV3_D4_MARK, YUV3_D5_MARK, YUV3_D6_MARK, YUV3_D7_MARK,
> +	YUV3_D8_MARK, YUV3_D9_MARK, YUV3_D10_MARK, YUV3_D11_MARK,
> +	YUV3_D12_MARK, YUV3_D13_MARK, YUV3_D14_MARK, YUV3_D15_MARK,
> +};

-- 
Regards,

Laurent Pinchart

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists