diff mbox series

[v2,3/4] pinctrl: s32cc: refactor pin config parsing

Message ID 20230320163823.886-4-clin@suse.com
State New
Headers
Series pinctrl: s32: driver improvements and generic struct use |

Commit Message

Chester Lin March 20, 2023, 4:38 p.m. UTC 
  Move common codes into smaller inline functions and remove some argument
handlings that are not actually used by either S32 MSCR register or generic
config params.

Signed-off-by: Chester Lin <clin@suse.com>
---
Changes in v2:
- Drop unnecessary marcos and s32_pin_config() implemented in v1 and set/clear
  mask/config values transparently.
- Put pull-function related cases together in s32_pin_set_pull().

 drivers/pinctrl/nxp/pinctrl-s32cc.c | 62 +++++++++++++++++------------
 1 file changed, 36 insertions(+), 26 deletions(-)

Comments

Andy Shevchenko March 20, 2023, 5:06 p.m. UTC | #1 
On Mon, Mar 20, 2023 at 6:39 PM Chester Lin <clin@suse.com> wrote:
>
> Move common codes into smaller inline functions and remove some argument
> handlings that are not actually used by either S32 MSCR register or generic
> config params.

...

>         case PIN_CONFIG_OUTPUT_ENABLE:
> -               if (arg)
> -                       *config |= S32_MSCR_OBE;
> -               else
> -                       *config &= ~S32_MSCR_OBE;
> +               *config |= S32_MSCR_OBE;
>                 *mask |= S32_MSCR_OBE;
>                 break;
>         case PIN_CONFIG_INPUT_ENABLE:
> -               if (arg)
> -                       *config |= S32_MSCR_IBE;
> -               else
> -                       *config &= ~S32_MSCR_IBE;
> +               *config |= S32_MSCR_IBE;
>                 *mask |= S32_MSCR_IBE;
>                 break;

Isn't it a regression here?
Otherwise needs an explicit explanation in the commit message on
what's going on here and why it's not a regression.

...

>         case PIN_CONFIG_BIAS_DISABLE:
> -               *config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
> -               *mask |= S32_MSCR_PUS | S32_MSCR_PUE;
> +               s32_pin_set_pull(param, mask, config);
>                 break;

This now can be unified with PU and PD cases above.
Chester Lin March 21, 2023, 5:01 a.m. UTC | #2 
On Mon, Mar 20, 2023 at 07:06:53PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 20, 2023 at 6:39 PM Chester Lin <clin@suse.com> wrote:
> >
> > Move common codes into smaller inline functions and remove some argument
> > handlings that are not actually used by either S32 MSCR register or generic
> > config params.
> 
> ...
> 
> >         case PIN_CONFIG_OUTPUT_ENABLE:
> > -               if (arg)
> > -                       *config |= S32_MSCR_OBE;
> > -               else
> > -                       *config &= ~S32_MSCR_OBE;
> > +               *config |= S32_MSCR_OBE;
> >                 *mask |= S32_MSCR_OBE;
> >                 break;
> >         case PIN_CONFIG_INPUT_ENABLE:
> > -               if (arg)
> > -                       *config |= S32_MSCR_IBE;
> > -               else
> > -                       *config &= ~S32_MSCR_IBE;
> > +               *config |= S32_MSCR_IBE;
> >                 *mask |= S32_MSCR_IBE;
> >                 break;
> 
> Isn't it a regression here?
> Otherwise needs an explicit explanation in the commit message on
> what's going on here and why it's not a regression.

Oops, it's wrong implementation. The argument checks of OUTPUT_EN and INPUT_EN
shouldn't be dropped. Thanks for the reminder and I will fix it.

Regards,
Chester

> 
> ...
> 
> >         case PIN_CONFIG_BIAS_DISABLE:
> > -               *config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
> > -               *mask |= S32_MSCR_PUS | S32_MSCR_PUE;
> > +               s32_pin_set_pull(param, mask, config);
> >                 break;
> 
> This now can be unified with PU and PD cases above.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
diff mbox series

Patch

diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index f698e1a240ef..cb8a0844c0fa 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -474,11 +474,38 @@  static int s32_get_slew_regval(int arg)
 	return -EINVAL;
 }
 
-static int s32_get_pin_conf(enum pin_config_param param, u32 arg,
-			    unsigned int *mask, unsigned int *config)
+static inline void s32_pin_set_pull(enum pin_config_param param,
+				   unsigned int *mask, unsigned int *config)
 {
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		*config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		*config |= S32_MSCR_PUS | S32_MSCR_PUE;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		*config &= ~S32_MSCR_PUS;
+		*config |= S32_MSCR_PUE;
+		break;
+	default:
+		return;
+	}
+
+	*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+}
+
+static int s32_parse_pincfg(unsigned long pincfg, unsigned int *mask,
+			    unsigned int *config)
+{
+	enum pin_config_param param;
+	u32 arg;
 	int ret;
 
+	param = pinconf_to_config_param(pincfg);
+	arg = pinconf_to_config_argument(pincfg);
+
 	switch (param) {
 	/* All pins are persistent over suspend */
 	case PIN_CONFIG_PERSIST_STATE:
@@ -488,17 +515,11 @@  static int s32_get_pin_conf(enum pin_config_param param, u32 arg,
 		*mask |= S32_MSCR_ODE;
 		break;
 	case PIN_CONFIG_OUTPUT_ENABLE:
-		if (arg)
-			*config |= S32_MSCR_OBE;
-		else
-			*config &= ~S32_MSCR_OBE;
+		*config |= S32_MSCR_OBE;
 		*mask |= S32_MSCR_OBE;
 		break;
 	case PIN_CONFIG_INPUT_ENABLE:
-		if (arg)
-			*config |= S32_MSCR_IBE;
-		else
-			*config &= ~S32_MSCR_IBE;
+		*config |= S32_MSCR_IBE;
 		*mask |= S32_MSCR_IBE;
 		break;
 	case PIN_CONFIG_SLEW_RATE:
@@ -509,25 +530,16 @@  static int s32_get_pin_conf(enum pin_config_param param, u32 arg,
 		*mask |= S32_MSCR_SRE(~0);
 		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
-		if (arg)
-			*config |= S32_MSCR_PUS;
-		else
-			*config &= ~S32_MSCR_PUS;
-		fallthrough;
 	case PIN_CONFIG_BIAS_PULL_DOWN:
-		if (arg)
-			*config |= S32_MSCR_PUE;
-		else
-			*config &= ~S32_MSCR_PUE;
-		*mask |= S32_MSCR_PUE | S32_MSCR_PUS;
+		s32_pin_set_pull(param, mask, config);
 		break;
 	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
 		*config &= ~(S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE);
 		*mask |= S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE;
-		fallthrough;
+		s32_pin_set_pull(param, mask, config);
+		break;
 	case PIN_CONFIG_BIAS_DISABLE:
-		*config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
-		*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+		s32_pin_set_pull(param, mask, config);
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -553,9 +565,7 @@  static int s32_pinconf_mscr_update(struct pinctrl_dev *pctldev,
 		pin_get_name(pctldev, pin_id), num_configs);
 
 	for (i = 0; i < num_configs; i++) {
-		ret = s32_get_pin_conf(pinconf_to_config_param(configs[i]),
-				       pinconf_to_config_argument(configs[i]),
-				       &mask, &config);
+		ret = s32_parse_pincfg(configs[i], &mask, &config);
 		if (ret)
 			return ret;
 	}