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@xxxxxxxxxxxxxx
>
> 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:
signature.asc
Description: PGP signature
