Re: [PATCH 3/5] pinctrl: pinctrl-aspeed-g6: add FWQSPI function-group

["Andrew Jeffery" <andrew@aj.id.au>]

On Tue, 8 Mar 2022, at 11:07, Jae Hyun Yoo wrote:
> From: Johnny Huang <johnny_huang@aspeedtech.com>
>
> Add FWSPIDQ2 (AE12) and FWSPIDQ3 (AF12) function-group to support
> AST2600 FW SPI quad mode. These pins can be used with dedicated FW
> SPI pins - FWSPICS0# (AB14), FWSPICK (AF13), FWSPIMOSI (AC14)
> and FWSPIMISO (AB13).
>
> Signed-off-by: Johnny Huang <johnny_huang@aspeedtech.com>
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
> ---
>  drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c 
> b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> index 54064714d73f..80838dc54b3a 100644
> --- a/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> +++ b/drivers/pinctrl/aspeed/pinctrl-aspeed-g6.c
> @@ -1236,12 +1236,17 @@ FUNC_GROUP_DECL(SALT8, AA12);
>  FUNC_GROUP_DECL(WDTRST4, AA12);
> 
>  #define AE12 196
> +SIG_EXPR_LIST_DECL_SESG(AE12, FWSPIQ2, FWQSPI, SIG_DESC_SET(SCU438, 4));
>  SIG_EXPR_LIST_DECL_SESG(AE12, GPIOY4, GPIOY4);
> -PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, GPIOY4));
> +PIN_DECL_(AE12, SIG_EXPR_LIST_PTR(AE12, FWSPIQ2),
> +	  SIG_EXPR_LIST_PTR(AE12, GPIOY4));
> 
>  #define AF12 197
> +SIG_EXPR_LIST_DECL_SESG(AF12, FWSPIQ3, FWQSPI, SIG_DESC_SET(SCU438, 5));
>  SIG_EXPR_LIST_DECL_SESG(AF12, GPIOY5, GPIOY5);
> -PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, GPIOY5));
> +PIN_DECL_(AF12, SIG_EXPR_LIST_PTR(AF12, FWSPIQ3),
> +	  SIG_EXPR_LIST_PTR(AF12, GPIOY5));
> +FUNC_GROUP_DECL(FWQSPI, AE12, AF12);

The idea behind the quad group was not to define a function for it 
specifically, but to re-use the FWSPID function and select the specific 
group associated with the specific style of SPI bus you desire. This 
way you'd have a pinctrl node like:

pinctrl_fwqspi_default = {
    function = "FWSPID";
    group = "FWQSPI";
};

(note the lack of 'Q' in the function name)

Maybe that's an abuse of groups, but I don't see a need for the 
function name to match the group name here, we're still doing SPI.

This can be seen in the DTS fix that Joel sent (disregarding the mixed 
voltage pins problem).

Thoughts?

Andrew

> 
>  #define AC12 198
>  SSSF_PIN_DECL(AC12, GPIOY6, FWSPIABR, SIG_DESC_SET(SCU438, 6));
> @@ -1911,6 +1916,7 @@ static const struct aspeed_pin_group 
> aspeed_g6_groups[] = {
>  	ASPEED_PINCTRL_GROUP(FSI2),
>  	ASPEED_PINCTRL_GROUP(FWSPIABR),
>  	ASPEED_PINCTRL_GROUP(FWSPID),
> +	ASPEED_PINCTRL_GROUP(FWQSPI),
>  	ASPEED_PINCTRL_GROUP(FWSPIWP),
>  	ASPEED_PINCTRL_GROUP(GPIT0),
>  	ASPEED_PINCTRL_GROUP(GPIT1),
> @@ -2152,6 +2158,7 @@ static const struct aspeed_pin_function 
> aspeed_g6_functions[] = {
>  	ASPEED_PINCTRL_FUNC(FSI2),
>  	ASPEED_PINCTRL_FUNC(FWSPIABR),
>  	ASPEED_PINCTRL_FUNC(FWSPID),
> +	ASPEED_PINCTRL_FUNC(FWQSPI),
>  	ASPEED_PINCTRL_FUNC(FWSPIWP),
>  	ASPEED_PINCTRL_FUNC(GPIT0),
>  	ASPEED_PINCTRL_FUNC(GPIT1),
> -- 
> 2.25.1