Re: [PATCH v2] pinctrl: sh-pfc: r8a7790: Add VIN1-B and VIN2-G pins, groups and functions

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Laurent,

On Tue, Sep 15, 2020 at 1:40 AM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
> On Tue, Sep 15, 2020 at 01:27:56AM +0200, Niklas Söderlund wrote:
> > On 2020-09-14 16:47:27 +0200, Geert Uytterhoeven wrote:
> > > On Sun, Sep 13, 2020 at 8:29 PM Lad Prabhakar wrote:
> > > > Add pins, groups and functions for the VIN1-B [data/clk/sync] and VIN2-G [data].
> > > >
> > > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > > > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> > > > ---
> > > > Changes for v2:
> > > > * Added complete list of VIN1-B pins
> > > > * Renamed vin2_data8_g to vin2_data8g
> > > > * Sorted vin1_sync_b pins
> > > >
> > > > v1 - https://patchwork.kernel.org/patch/11761191/

> > > > @@ -3959,6 +4039,18 @@ static const unsigned int vin2_data18_mux[] = {
> > > >         VI2_R4_MARK, VI2_R5_MARK,
> > > >         VI2_R6_MARK, VI2_R7_MARK,
> > > >  };
> > > > +static const unsigned int vin2_data8g_pins[] = {
> > > > +       RCAR_GP_PIN(0, 27), RCAR_GP_PIN(0, 28),
> > > > +       RCAR_GP_PIN(0, 29), RCAR_GP_PIN(1, 10),
> > > > +       RCAR_GP_PIN(1, 4), RCAR_GP_PIN(1, 5),
> > > > +       RCAR_GP_PIN(1, 6), RCAR_GP_PIN(1, 7),
> > > > +};
> > > > +static const unsigned int vin2_data8g_mux[] = {
> > > > +       VI2_G0_MARK, VI2_G1_MARK,
> > > > +       VI2_G2_MARK, VI2_G3_MARK,
> > > > +       VI2_G4_MARK, VI2_G5_MARK,
> > > > +       VI2_G6_MARK, VI2_G7_MARK,
> > > > +};
> > >
> > > Laurent, Niklas: are you happy with the name "vin2_data8g", or do
> > > you have a better suggestion?
> >
> > I learnt recently that traditionally for single 8-bit RAW inputs are
> > named R8 (fist in RGB). But as this really is the G pins and they are
> > labeled as such I'm OK with the name, but I would like to hear Laurent's
> > view as well.
>
> I think we should match the pin names from the datasheet, so a R suffix
> isn't a good option. I have a feeling we will suffer with this though,
> as here 'g' refers to the VIN green data pins (g[7:0], a.k.a.
> data[15:8]), while below 'b' refers to the second set of VIN data pins,
> not the blue data pins. One option would be to use "vin2_data8_shift8",
> but I'm not sure I'm very fond of that either. I also wonder whether we
> shouldn't call this "vin2_g8" instead of "vin2_data8g" as the pins are
> named VIN_G[7:0], not VIN_DATAG[7:0].

On R-Car H2 and RZ/G1H they're indeed named VIx_G[7:0].

However, looking at other R-Car Gen2 and Gen3 variants, there are
many possibilities, from all-RGB:
  1. R[7:0], G[7:0], B[7:0],
over:
  2. R[7:0], G[7:0], DATA[7:0]_B[7:0],
  3. D[23:16]_R[7:0], D[15:8]_G[7:0]_Y[7:0], D[7:0]_B[7:0]_C[7:0],
to all-DATA:
  4. DATA[11:0],
  5. DATA[23:0].

Following 1, 24-bit should be called "rgb24", and 18-bit "rgb18"
(I believe this is the only format using discontiguous pins?).
The in-betweens make sense, as YCbCr[7:0] data goes over the 8-bit DATA
or 16-bit D pins, but that sense is lost when considering other formats
that accept 10/12/16/20-bit input.
I guess that's why R-Car Gen3 settled at option 5, which is actually
what we've been doing in the pin control drivers from the beginning.

Nevertheless, I agree "vinX_g8" seems like the best name for this group,
as it's quite obvious from the name what it means, and isn't easily
confused with an alternate set of pins.
Note that the BSP (for R-Car Gen3, no idea about Gen2) uses
"vinX_data8_sft8" (which I never really liked), which Niklas is now
trying to upstream in "[PATCH 0/4] pinctrl: sh-pfc: Add VIN stf8 pins"
(https://lore.kernel.org/linux-renesas-soc/20200914233744.468175-1-niklas.soderlund+renesas@xxxxxxxxxxxx).
Expect more bikeshedding soon ;-)

> > > >  static const unsigned int vin2_sync_pins[] = {
> > > >         RCAR_GP_PIN(1, 16), /* HSYNC */
> > > >         RCAR_GP_PIN(1, 21), /* VSYNC */
> > >
> > > > @@ -4310,15 +4402,25 @@ static const struct {
> > > >                 VIN_DATA_PIN_GROUP(vin1_data, 10),
> > > >                 VIN_DATA_PIN_GROUP(vin1_data, 8),
> > > >                 VIN_DATA_PIN_GROUP(vin1_data, 4),
> > > > +               VIN_DATA_PIN_GROUP(vin1_data, 24, _b),
> > > > +               VIN_DATA_PIN_GROUP(vin1_data, 20, _b),
> > > > +               SH_PFC_PIN_GROUP(vin1_data18_b),
> > > > +               VIN_DATA_PIN_GROUP(vin1_data, 16, _b),
> > > > +               VIN_DATA_PIN_GROUP(vin1_data, 12, _b),
> > > > +               VIN_DATA_PIN_GROUP(vin1_data, 10, _b),
> > > > +               VIN_DATA_PIN_GROUP(vin1_data, 8, _b),
> > >
> > > Missing vin1_data4_b.
> > >
> > > >                 SH_PFC_PIN_GROUP(vin1_sync),
> > > > +               SH_PFC_PIN_GROUP(vin1_sync_b),
> > > >                 SH_PFC_PIN_GROUP(vin1_field),
> > > >                 SH_PFC_PIN_GROUP(vin1_clkenb),
> > > >                 SH_PFC_PIN_GROUP(vin1_clk),
> > > > +               SH_PFC_PIN_GROUP(vin1_clk_b),
> > > >                 VIN_DATA_PIN_GROUP(vin2_data, 24),
> > > >                 SH_PFC_PIN_GROUP(vin2_data18),
> > > >                 VIN_DATA_PIN_GROUP(vin2_data, 16),
> > > >                 VIN_DATA_PIN_GROUP(vin2_data, 8),
> > > >                 VIN_DATA_PIN_GROUP(vin2_data, 4),
> > > > +               SH_PFC_PIN_GROUP(vin2_data8g),
> > > >                 SH_PFC_PIN_GROUP(vin2_sync),
> > > >                 SH_PFC_PIN_GROUP(vin2_field),
> > > >                 SH_PFC_PIN_GROUP(vin2_clkenb),

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