On 2/18/19 9:43 AM, Geert Uytterhoeven wrote:
> Hi Marek,

Hi,

> On Sat, Feb 16, 2019 at 2:49 PM <marek.va...@gmail.com> 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+rene...@gmail.com>
> 
> 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/.

Added

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

For one, It's unrelated register to POCCTRL, so I don't want to mix them
together. The other reason is this r8a779*_pin_to_pocctrl(), which uses
the ioctrl_regs[] array ; if I added the TDSEL there, that'd just make
things extra confusing.

-- 
Best regards,
Marek Vasut

Reply via email to