Hi Marek,
On Sat, Feb 16, 2019 at 2:49 PM <marek.vasut@xxxxxxxxx> wrote:
> The TDSELCTRL register is responsible for configuring the SDHI clock
> return path delay and may be adjusted by the bootloader. Retain the
> value across suspend/resume to prevent hardware instability after
> resume.
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@xxxxxxxxx>
Thanks for your patch!
Looks good to me. Two comments though.
R-Car D3 (r8a77995) also has a TDSEL register, but it is not handled by
your patch. Adding that would mean s/SDHI/SDHI and MMC/.
> --- a/drivers/pinctrl/sh-pfc/core.c
> +++ b/drivers/pinctrl/sh-pfc/core.c
> @@ -657,6 +657,10 @@ static unsigned int sh_pfc_walk_regs(struct sh_pfc *pfc,
> for (i = 0; pfc->info->ioctrl_regs[i].reg; i++)
> do_reg(pfc, pfc->info->ioctrl_regs[i].reg, n++);
>
> + if (pfc->info->tdsel_regs)
> + for (i = 0; pfc->info->tdsel_regs[i].reg; i++)
> + do_reg(pfc, pfc->info->tdsel_regs[i].reg, n++);
> +
> return n;
> }
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -176,6 +176,10 @@ struct pinmux_ioctrl_reg {
> u32 reg;
> };
>
> +struct pinmux_tdsel_reg {
> + u32 reg;
> +};
> +
> struct pinmux_data_reg {
> u32 reg;
> u8 reg_width;
> @@ -270,6 +274,7 @@ struct sh_pfc_soc_info {
> const struct pinmux_drive_reg *drive_regs;
> const struct pinmux_bias_reg *bias_regs;
> const struct pinmux_ioctrl_reg *ioctrl_regs;
> + const struct pinmux_tdsel_reg *tdsel_regs;
> const struct pinmux_data_reg *data_regs;
>
> const u16 *pinmux_data;
Is there any special reason why you added a new block of registers with
separate handling, instead of adding TDSEL to the existing
pinmux_ioctrl_reg[] arrays, which list other IOCTRL registers like
POCCTRL?
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
