Re: [PATCH v3 18/21] pinctrl: renesas: r8a779g0: add missing PWM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Morimoto-san,

On Tue, Jun 14, 2022 at 8:00 AM Kuninori Morimoto
<kuninori.morimoto.gx@xxxxxxxxxxx> wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>
> V4H has PWM/PWM_A/PWM_B, but current PFC setting is mixed.
> This patch add missing PWM settings, and tidyup these.
>
> According to Document, GP3_14 Function4 is PWM2_A,
> but we can't select it at P1SR3[27:24].
> This patch just ignore it for now.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pfc-r8a779g0.c
> +++ b/drivers/pinctrl/renesas/pfc-r8a779g0.c
> @@ -304,9 +304,9 @@
>  #define IP1SR1_11_8    FM(MSIOF0_SCK)          FM(HSCK1_X)             FM(SCK1_X)      F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)
>  #define IP1SR1_15_12   FM(MSIOF0_RXD)          F_(0, 0)                F_(0, 0)        F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)
>  #define IP1SR1_19_16   FM(HTX0)                FM(TX0)                 F_(0, 0)        F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)
> -#define IP1SR1_23_20   FM(HCTS0_N)             FM(CTS0_N)              FM(PWM8)        F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)
> -#define IP1SR1_27_24   FM(HRTS0_N)             FM(RTS0_N)              FM(PWM9)        F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)
> -#define IP1SR1_31_28   FM(HSCK0)               FM(SCK0)                FM(PWM0)        F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)
> +#define IP1SR1_23_20   FM(HCTS0_N)             FM(CTS0_N)              FM(PWM8_A)      F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)
> +#define IP1SR1_27_24   FM(HRTS0_N)             FM(RTS0_N)              FM(PWM9_A)      F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)
> +#define IP1SR1_31_28   FM(HSCK0)               FM(SCK0)                FM(PWM0_A)      F_(0, 0)        F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0) F_(0, 0)

I think all renames belong in "[PATCH v3 03/21] pinctrl: renesas:
Initial R8A779G0 (V4H) PFC support", as it is a bug in that patch.

> @@ -2847,20 +2869,28 @@ static const char * const pcie_groups[] = {
>         "pcie1_clkreq_n",
>  };
>
> -static const char * const pwm0_groups[] = {
> -       "pwm0",
> +static const char * const pwm0_a_groups[] = {
> +       "pwm0_a",
> +};
> +
> +static const char * const pwm1_a_groups[] = {
> +       "pwm1_a",
> +};
> +
> +static const char * const pwm1_b_groups[] = {
> +       "pwm1_b",
>  };

Please have a single pwm1_groups[], containing both "pwm1_a" and
"pwm1_b".  Same for the other groups.

> @@ -3005,16 +3035,18 @@ static const struct sh_pfc_function pinmux_functions[] = {
>
>         SH_PFC_FUNCTION(pcie),
>
> -       SH_PFC_FUNCTION(pwm0),
> -       SH_PFC_FUNCTION(pwm1),
> -       SH_PFC_FUNCTION(pwm2),
> -       SH_PFC_FUNCTION(pwm3),
> +       SH_PFC_FUNCTION(pwm0_a),
> +       SH_PFC_FUNCTION(pwm1_a),
> +       SH_PFC_FUNCTION(pwm1_b),
> +       SH_PFC_FUNCTION(pwm2_b),
> +       SH_PFC_FUNCTION(pwm3_a),
> +       SH_PFC_FUNCTION(pwm3_b),
>         SH_PFC_FUNCTION(pwm4),
>         SH_PFC_FUNCTION(pwm5),
>         SH_PFC_FUNCTION(pwm6),
>         SH_PFC_FUNCTION(pwm7),
> -       SH_PFC_FUNCTION(pwm8),
> -       SH_PFC_FUNCTION(pwm9),
> +       SH_PFC_FUNCTION(pwm8_a),
> +       SH_PFC_FUNCTION(pwm9_a),

Please drop these changes, as they are not needed.

>
>         SH_PFC_FUNCTION(qspi0),
>         SH_PFC_FUNCTION(qspi1),

However, given the inconsistent naming of pins for PWMs that are
available on a single pin (PWM[08]_A vs. PWM[4-7]), I expect several
of these to be renamed in future revisions of the documentation.
As pin group names are part of the DT ABI, that should happen sooner
rather than later...

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



[Index of Archives]     [Linux Samsung SOC]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux