Re: [PATCH 2/2] pinctrl: sh-pfc: add R8A77980 PFC support

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

Hello!

On 02/26/2018 03:42 PM, Geert Uytterhoeven wrote:

>> Add the PFC support for the R8A77980 SoC including pin groups for some
>> on-chip devices such as AVB, CAN-FD, GETHER, [H]SCIF, I2C, INTC-EX, MMC,
>> MSIOF, PWM, and VIN...
>>
>> Based on the original (and large) patch by Vladimir Barinov.
>>
>> Signed-off-by: Vladimir Barinov <vladimir.barinov@xxxxxxxxxxxxxxxxxx>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>
> 
> Thanks for your patch!
> 
> To avoid scaring off potential reviewers, it may be better to not include that
> many pin groups and functions in future initial submissions.
> This also helps if issues are detected during review in some of them (like
> below), delaying queuing of basic functionality and other correct parts.
> 
> I only looked at CPU_ALL_PORT(), pins, groups, and functions.
> Comments below.
> 
>> --- /dev/null
>> +++ renesas-drivers/drivers/pinctrl/sh-pfc/pfc-r8a77980.c
> 
>> +/* - AVB -------------------------------------------------------------------- */
>> +static const unsigned int avb_link_pins[] = {
>> +       /* AVB_LINK */
>> +       RCAR_GP_PIN(1, 18),
>> +};
>> +static const unsigned int avb_link_mux[] = {
>> +       AVB_LINK_MARK,
>> +};
>> +static const unsigned int avb_magic_pins[] = {
>> +       /* AVB_MAGIC */
>> +       RCAR_GP_PIN(1, 16),
>> +};
>> +static const unsigned int avb_magic_mux[] = {
>> +       AVB_MAGIC_MARK,
>> +};
>> +static const unsigned int avb_phy_int_pins[] = {
>> +       /* AVB_PHY_INT */
>> +       RCAR_GP_PIN(1, 17),
>> +};
>> +static const unsigned int avb_phy_int_mux[] = {
>> +       AVB_PHY_INT_MARK,
>> +};
>> +static const unsigned int avb_mdio_pins[] = {
>> +       /* AVB_MDC, AVB_MDIO */
>> +       RCAR_GP_PIN(1, 15), RCAR_GP_PIN(1, 14),
>> +};
>> +static const unsigned int avb_mdio_mux[] = {
>> +       AVB_MDC_MARK, AVB_MDIO_MARK,
>> +};
> 
> The grouping is different from other R-Car Gen3 SoCs.

   Not true AFAICS -- only the group naming is different.

>> +/* - CANFD0 ----------------------------------------------------------------- */
>> +static const unsigned int canfd0_data_a_pins[] = {
>> +       /* CANFD0_TX, CANFD0_RX */
>> +       RCAR_GP_PIN(1, 21), RCAR_GP_PIN(1, 22),
>> +};
>> +static const unsigned int canfd0_data_a_mux[] = {
>> +       CANFD0_TX_A_MARK, CANFD0_RX_A_MARK,
>> +};
>> +static const unsigned int canfd_clk_a_pins[] = {
>> +       /* CANFD_CLK */
>> +       RCAR_GP_PIN(1, 25),
>> +};
>> +static const unsigned int canfd_clk_a_mux[] = {
>> +       CANFD_CLK_A_MARK,
>> +};
>> +static const unsigned int canfd0_data_b_pins[] = {
>> +       /* CANFD0_TX, CANFD0_RX */
>> +       RCAR_GP_PIN(3, 6), RCAR_GP_PIN(3, 7),
>> +};
>> +static const unsigned int canfd0_data_b_mux[] = {
>> +       CANFD0_TX_B_MARK, CANFD0_RX_B_MARK,
>> +};
>> +static const unsigned int canfd_clk_b_pins[] = {
>> +       /* CANFD_CLK */
>> +       RCAR_GP_PIN(3, 8),
>> +};
>> +static const unsigned int canfd_clk_b_mux[] = {
>> +       CANFD_CLK_B_MARK,
>> +};
> 
> Please move the canfd_clk pins to their own section.

   Are you aware the CANFD_CLK is controlled by MOD_SEL0.SEL_CANFD0? If it's allowed
to have the pins controlled by a single MOD_SEL field in the diff groups, I'll place
CANFD_CLK into a separate group...

> Note that they are called can_clk in the pin function sheet, but the
> PFC section uses both can_clk and canfd_clk.
> Given V3H (like V3M) has no plain CAN, only CANFD, using canfd_clk
> sounds right to me, though.
> 
>> +/* - DU --------------------------------------------------------------------- */
>> +static const unsigned int du_rgb666_pins[] = {
>> +       /* DU_R[7:2], DU_G[7:2], DU_B[7:2] */
>> +       RCAR_GP_PIN(0, 5), RCAR_GP_PIN(0, 4), RCAR_GP_PIN(0, 3),
>> +       RCAR_GP_PIN(0, 2), RCAR_GP_PIN(0, 1), RCAR_GP_PIN(0, 0),
>> +       RCAR_GP_PIN(0, 11), RCAR_GP_PIN(0, 10), RCAR_GP_PIN(0, 9),
>> +       RCAR_GP_PIN(0, 8), RCAR_GP_PIN(0, 7), RCAR_GP_PIN(0, 6),
>> +       RCAR_GP_PIN(0, 17), RCAR_GP_PIN(0, 16), RCAR_GP_PIN(0, 15),
>> +       RCAR_GP_PIN(0, 14), RCAR_GP_PIN(0, 13), RCAR_GP_PIN(0, 12),
>> +};
>> +static const unsigned int du_rgb666_mux[] = {
>> +       DU_DR7_MARK, DU_DR6_MARK, DU_DR5_MARK,
>> +       DU_DR4_MARK, DU_DR3_MARK, DU_DR2_MARK,
>> +       DU_DG7_MARK, DU_DG6_MARK, DU_DG5_MARK,
>> +       DU_DG4_MARK, DU_DG3_MARK, DU_DG2_MARK,
>> +       DU_DB7_MARK, DU_DB6_MARK, DU_DB5_MARK,
>> +       DU_DB4_MARK, DU_DB3_MARK, DU_DB2_MARK,
>> +};
> 
> du_rgb888 is missing (see GP2_19..GP2_24).

   OK, seeing...

>> +/* - HSCIF0 ----------------------------------------------------------------- */
> 
>> +static const unsigned int scif_clk_a_pins[] = {
>> +       /* SCIF_CLK */
>> +       RCAR_GP_PIN(0, 10),
>> +};
>> +static const unsigned int scif_clk_a_mux[] = {
>> +       SCIF_CLK_A_MARK,
>> +};
> 
> 
> [...]
> 
>> +static const unsigned int scif_clk_b_pins[] = {
>> +       /* SCIF_CLK */
>> +       RCAR_GP_PIN(1, 25),
>> +};
>> +static const unsigned int scif_clk_b_mux[] = {
>> +       SCIF_CLK_B_MARK,
>> +};
> 
> Please move the scif_clk pins to their own section, as they are shared by
> HSCIF and SCIF.

   Again, the same bit in MOD_SEL0...

>> +/* - VIN0 ------------------------------------------------------------------- */
> 
>> +static const unsigned int vin0_sync_pins[] = {
>> +       /* VI0_VSYNC#, VI0_HSYNC# */
>> +       RCAR_GP_PIN(2, 3), RCAR_GP_PIN(2, 2),
>> +};
>> +static const unsigned int vin0_sync_mux[] = {
>> +       VI0_HSYNC_N_MARK, VI0_VSYNC_N_MARK,
> 
> Reversed order compared to the pins above.

   Right.

>> +/* - VIN1 ------------------------------------------------------------------- */
> 
>> +static const unsigned int vin1_sync_pins[] = {
>> +       /* VI1_VSYNC#, VI1_HSYNC# */
>> +        RCAR_GP_PIN(3, 3), RCAR_GP_PIN(3, 2),
>> +};
>> +static const unsigned int vin1_sync_mux[] = {
>> +       VI1_HSYNC_N_MARK, VI1_VSYNC_N_MARK,
> 
> Reversed order compared to the pins above.

   Right.

[...]
>> +       VIN_DATA_PIN_GROUP(vin0_data, 8),
>> +       VIN_DATA_PIN_GROUP(vin0_data, 10),
>> +       VIN_DATA_PIN_GROUP(vin0_data, 12),
>> +       VIN_DATA_PIN_GROUP(vin0_data, 16),
>> +       VIN_DATA_PIN_GROUP(vin0_data, 20),
>> +       VIN_DATA_PIN_GROUP(vin0_data, 24),
> 
> Missing case for vin0_data_18 (RGB-666)

   We probably need the vin pin union extended...

[...]

> Gr{oetje,eeting}s,
> 
>                         Geert

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html