Re: [PATCH v2 5/8] pinctrl: sh-pfc: r8a77990: Add VIN pins, groups and functions

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 3699 bytes --]

Hi Geert, Simon,
   sorry to resurect this one, but while upporting VIN pin definition
for R8A77965 I have noticed something in this patch.

Please see below.

On Tue, Oct 02, 2018 at 11:25:31AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,

[snip]

> > @@ -1889,6 +2077,32 @@ static const struct sh_pfc_pin_group pinmux_groups[] = {
> >         SH_PFC_PIN_GROUP(usb0_id),
> >         SH_PFC_PIN_GROUP(usb30),
> >         SH_PFC_PIN_GROUP(usb30_id),
> > +       VIN_DATA_PIN_GROUP(vin4_data_a, 8),
> > +       VIN_DATA_PIN_GROUP(vin4_data_a, 10),
> > +       VIN_DATA_PIN_GROUP(vin4_data_a, 12),
> > +       VIN_DATA_PIN_GROUP(vin4_data_a, 16),
> > +       VIN_DATA_PIN_GROUP(vin4_data_a, 20),
> > +       VIN_DATA_PIN_GROUP(vin4_data_a, 24),
> > +       VIN_DATA_PIN_GROUP(vin4_data_b, 8),
> > +       VIN_DATA_PIN_GROUP(vin4_data_b, 10),
> > +       VIN_DATA_PIN_GROUP(vin4_data_b, 12),
> > +       VIN_DATA_PIN_GROUP(vin4_data_b, 16),
> > +       VIN_DATA_PIN_GROUP(vin4_data_b, 20),
> > +       VIN_DATA_PIN_GROUP(vin4_data_b, 24),

look here...

[snip]

> >
> > +static const char * const vin4_groups[] = {
> > +       "vin4_data8_a",
> > +       "vin4_data10_a",
> > +       "vin4_data12_a",
> > +       "vin4_data16_a",
> > +       "vin4_data20_a",
> > +       "vin4_data24_a",
> > +       "vin4_data8_b",
> > +       "vin4_data10_b",
> > +       "vin4_data12_b",
> > +       "vin4_data16_b",
> > +       "vin4_data20_b",
> > +       "vin4_data24_b",

Then here.

VIN_DATA_PIN_GROUP() expands as:

#define VIN_DATA_PIN_GROUP(n, s)				\
	{							\
		.name = #n#s,					\
		.pins = n##_pins.data##s,			\
		.mux = n##_mux.data##s,				\
		.nr_pins = ARRAY_SIZE(n##_pins.data##s),	\
	}

So these groups should not be named
        "vin4_dataX_a" and
        "vin4_dataX_b"

But instead
        "vin4_data_aX" and
        "vin4_data_bX"

Am I wrong?

The only Gen3 SoC in mainline which uses the VIN data pins defined
through this macro is D3, which fortunately does not have any 'a' or
'b' group.

$ git grep vin\.*_data* arch/arm64/boot/dts/
arch/arm64/boot/dts/renesas/r8a77995-draak.dts:        groups =  "vin4_data8", "vin4_sync", "vin4_clk";

$ cat drivers/pinctrl/sh-pfc/pfc-r8a77995.c | grep "vin4_data, 8"
        VIN_DATA_PIN_GROUP(vin4_data, 8),

Going forward we might see some user of vin data groups having to
refer to "vin4_data_a8" and so on, which is not nice compared to
"vin4_data8_a".

What do you think?
In any case, this patch is indeed wrong. Or we align the group names
to what the macro produces, or change the macro, but I cannot tell how
to do that in a sane way? (introduce a new one that wants a 'group'
argument too?)

Thanks
  j

> > +       "vin4_data8_sft8",
>
> You dropped the sft8 pins, but forgot to remove the sft8 group name.
>
> > +       "vin4_sync",
> > +       "vin4_field",
> > +       "vin4_clkenb",
> > +       "vin4_clk",
> > +};
> > +
> > +static const char * const vin5_groups[] = {
> > +       "vin5_data8_a",
> > +       "vin5_data8_sft8_a",
>
> Likewise.
>
> > +       "vin5_data10_a",
> > +       "vin5_data12_a",
> > +       "vin5_data16_a",
> > +       "vin5_data8_b",
> > +       "vin5_sync_a",
> > +       "vin5_field_a",
> > +       "vin5_clkenb_a",
> > +       "vin5_clk_a",
> > +       "vin5_clk_b",
> > +};
>
> The rest looks OK to me, and matches the datasheet clarification.
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]