Hi Geert,

Thanks for your time and review

> >  drivers/pinctrl/sh-pfc/pfc-r8a7795.c | 121
> > +++++++++++++++++++++++++++++++++++
> >  1 file changed, 121 insertions(+)
> >
> > diff --git a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> > b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> > index 33be5d56..6f246ec 100644
> > --- a/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> > +++ b/drivers/pinctrl/sh-pfc/pfc-r8a7795.c
> > @@ -1658,6 +1658,91 @@ static const unsigned int canfd1_data_mux[] = {
> >         CANFD1_TX_MARK,         CANFD1_RX_MARK,
> >  };
> >
> > +/* - DRIF
> > +--------------------------------------------------------------- */
> static const unsigned int drif0_data_a_pins[] = {
> > +       /* CLK, SYNC, D0, D1 */
> > +       RCAR_GP_PIN(6, 8), RCAR_GP_PIN(6, 9), RCAR_GP_PIN(6, 10),
> > +       RCAR_GP_PIN(6, 7),
> > +};
> > +static const unsigned int drif0_data_a_mux[] = {
> > +       RIF0_CLK_A_MARK, RIF0_SYNC_A_MARK, RIF0_D0_A_MARK,
> > +RIF0_D1_A_MARK, };
> 
> According to my information, each DRIF module consists of two sub-modules
> (that's why there are 8 module clocks for 4 DRIF modules), each handling a
> data pin, but sharing the CLK and SYNC signals.
> Hence it's possible to receive using only one data pin. Is that correct?

Yes, that is correct.

> 
> Shouldn't the pinctrl data reflect that? The second unused data pin could
> be used for something else.

Is that possible? For e.g. when MOD_SEL0(bit8 -> sel_drif2) is set to 0, all 
the 4 pins are owned by DRIF IP(RIF2_XXX_A set) even though one of the 
RIF2_D0_A or RIF2_D1_A may be unused depending on the master it interfaces 
with. Am I missing something?

> 
> One way to handle this is like SDHI and DU do: provide 2 sets of data, one
> for single-bit, and one for dual-bit configurations:
>   - "drif0_data1_a_pins" for CLK, SYNC, and D0,
>   - "drif0_data2_a_pins" for CLK. SYNC, D0, and D1.
> 
> Alternative, it could be split in 3 groups:
>   - "drif0_ctrl_a_pins" for CLK and SYNC,
>   - "drif0_data0_a_pins for D0,
>   - "drif0_data1_a_pins for D1.
> 
> I think the latter is preferable, as you can probably configure the DRIF
> to receive data through D1 only, and leave D0 unused?
> 
> > +static const unsigned int drif3_data_a_pins[] = {
> > +       /* CLK, SYNC, D0, D1 */
> > +       RCAR_GP_PIN(6, 17), RCAR_GP_PIN(6, 18), RCAR_GP_PIN(6, 19),
> > +       RCAR_GP_PIN(6, 10),
> 
> According to my datasheet, the above line should be
> 
>         RCAR_GP_PIN(6, 20),

You are right. Thanks again. I'll fix that typo :-(

Thanks,
Ramesh

Reply via email to