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

[jacopo mondi <jacopo@jmondi.org>]

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

Hi Geert,

On Tue, Aug 28, 2018 at 09:46:57AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Mon, Aug 20, 2018 at 12:17 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> > From: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> >
> > This patch adds VIN{4,5} pins, groups and functions to the R8A77990 SoC.
> >
> > Signed-off-by: Takeshi Kihara <takeshi.kihara.df@renesas.com>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> Thanks for your patch!

Thanks for review.

>
> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a77990.c
> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a77990.c
> > @@ -1982,6 +1982,446 @@ static const unsigned int usb30_id_mux[] = {
> >         USB3HS0_ID_MARK,
> >  };
> >
> > +/* - VIN4 ------------------------------------------------------------------- */
> > +static const unsigned int vin4_data8_a_pins[] = {
> > +       RCAR_GP_PIN(2, 6),  RCAR_GP_PIN(2, 7),
> > +       RCAR_GP_PIN(2, 8),  RCAR_GP_PIN(2, 9),
> > +       RCAR_GP_PIN(2, 10), RCAR_GP_PIN(2, 11),
> > +       RCAR_GP_PIN(2, 12), RCAR_GP_PIN(2, 13),
> > +};
> > +
> > +static const unsigned int vin4_data8_a_mux[] = {
> > +       VI4_DATA0_A_MARK, VI4_DATA1_A_MARK,
> > +       VI4_DATA2_A_MARK, VI4_DATA3_A_MARK,
> > +       VI4_DATA4_A_MARK, VI4_DATA5_A_MARK,
> > +       VI4_DATA6_A_MARK, VI4_DATA7_A_MARK,
> > +};
> > +
> > +static const unsigned int vin4_data10_a_pins[] = {
> > +       RCAR_GP_PIN(2, 6),  RCAR_GP_PIN(2, 7),
> > +       RCAR_GP_PIN(2, 8),  RCAR_GP_PIN(2, 9),
> > +       RCAR_GP_PIN(2, 10), RCAR_GP_PIN(2, 11),
> > +       RCAR_GP_PIN(2, 12), RCAR_GP_PIN(2, 13),
> > +       RCAR_GP_PIN(1, 4),  RCAR_GP_PIN(1, 5),
> > +};
> > +
> > +static const unsigned int vin4_data10_a_mux[] = {
> > +       VI4_DATA0_A_MARK, VI4_DATA1_A_MARK,
> > +       VI4_DATA2_A_MARK, VI4_DATA3_A_MARK,
> > +       VI4_DATA4_A_MARK, VI4_DATA5_A_MARK,
> > +       VI4_DATA6_A_MARK, VI4_DATA7_A_MARK,
> > +       VI4_DATA8_MARK,   VI4_DATA9_MARK,
> > +};
>
> Can you please use union vin_data and VIN_DATA_PIN_GROUP(), to reduce
> redundancies, cfr. commit 9942a5b52990b8d5 ("pinctrl: sh-pfc: r8a7795:
> Deduplicate VIN4 pin definitions")?
> Or do you want to do that in a follow-up patch (which means more
> review work)?

I will have to resend anyhow, so I can make this change in v2.

>
> > +static const unsigned int vin4_data8_sft8_pins[] = {
> > +       RCAR_GP_PIN(1, 4),  RCAR_GP_PIN(1, 5),
> > +       RCAR_GP_PIN(1, 6),  RCAR_GP_PIN(1, 7),
> > +       RCAR_GP_PIN(1, 3),  RCAR_GP_PIN(1, 10),
> > +       RCAR_GP_PIN(1, 13), RCAR_GP_PIN(1, 14),
> > +};
>
> R-Car H3 and M3-W don't have the data8_sft8 groups yet (the BSP has).
> IIRC, that's due to rcar-vin not yet supporting that mode. Niklas?

Not sure what SFT mode is. The only registers naming this are related
to "Shift Down Volume" and RGB->YCrCb conversion. I'll wait for
Niklas, and eventually remove the pin group.
>
> > +static const unsigned int vin5_sync_a_pins[] = {
> > +       /* HSYNC_N, VSYNC_N */
> > +       RCAR_GP_PIN(1, 8), RCAR_GP_PIN(1, 9),
> > +};
> > +
> > +static const unsigned int vin5_sync_a_mux[] = {
> > +       VI5_HSYNC_N_A_MARK, VI5_VSYNC_N_A_MARK,
> > +};
> > +
> > +static const unsigned int vin5_field_a_pins[] = {
> > +       RCAR_GP_PIN(1, 10),
> > +};
> > +
> > +static const unsigned int vin5_field_a_mux[] = {
> > +       VI5_FIELD_A_MARK,
> > +};
> > +
> > +static const unsigned int vin5_clkenb_a_pins[] = {
> > +       RCAR_GP_PIN(0, 1),
> > +};
> > +
> > +static const unsigned int vin5_clkenb_a_mux[] = {
> > +       VI5_CLKENB_A_MARK,
> > +};
>
> "A" groups without "B" groups? Usually we drop the "_A" suffix in that case.
>
> They're actually named like that in hardware user manual rev. 1.00 (and no
> update in the errata)?

It's true, the sync signal pins of VI4 do not have any "_a" or "_b"
mark, while the VI5 ones do. Eg. we have "VI5_VSYNC#_A" and
"VI4_VSYNC#". This might be an error in the chip manual or a
suggestion that VIN5_DATA.*_B pins do not support synchronism signals
and can only work with embedded synchronization. This seems unlikely
to me, and checking Table 26.20.3 that lists the supported input format
for each channel, I don't see anything like that mentioned.

Morimoto-san, Shimoda-san, could you please verify why those pins,
have a different naming scheme?

Thanks
   j

>
> 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 --]