diff mbox series

[2/3] pinctrl: s32cc: refactor pin config parsing

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

Commit Message

Chester Lin March 14, 2023, 1:46 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>
---
 drivers/pinctrl/nxp/pinctrl-s32cc.c | 82 ++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 32 deletions(-)

Comments

Andy Shevchenko March 14, 2023, 5:15 p.m. UTC | #1 
On Tue, Mar 14, 2023 at 3:47 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.

...

> +#define S32_CFG_SET            true
> +#define S32_CFG_CLR            false

Have no value, use boolean values directly.

...

> +static inline void s32_pin_config(unsigned int *mask, unsigned int *config,
> +                                 unsigned int bits, bool set)
> +{
> +       if (set)
> +               *config |= bits;
> +       else
> +               *config &= ~bits;
> +       *mask |= bits;
> +}
> +
> +static inline void s32_pull_enable(enum pin_config_param param,
> +                                  unsigned int *mask, unsigned int *config)
> +{

> +

Unneeded blank line

> +       if (param == PIN_CONFIG_BIAS_PULL_UP) {
> +               s32_pin_config(mask, config, S32_MSCR_PUS | S32_MSCR_PUE,
> +                              S32_CFG_SET);
> +       } else if (param == PIN_CONFIG_BIAS_PULL_DOWN) {
> +               *config &= ~S32_MSCR_PUS;
> +               *config |= S32_MSCR_PUE;
> +               *mask |= S32_MSCR_PUS | S32_MSCR_PUE;
> +       }

This looks intransparent.

Just use your common sense and write it without that s32_pin_config() helper.

> +}
> +
> +static inline void s32_pull_disable(unsigned int *mask, unsigned int *config)
> +{
> +       s32_pin_config(mask, config, S32_MSCR_PUS | S32_MSCR_PUE, S32_CFG_CLR);
> +}

This should go with above together, just use switch and make the
configuration based on it.
Before the switch-case, clear what has to be cleared for all cases
(some cases may override some bits afterwards), set what has to be set
for all cases (some cases may override some bits afterwards) and
adjust.

Look into pinctrl-intel.c for the rough example.
diff mbox series

Patch

diff --git a/drivers/pinctrl/nxp/pinctrl-s32cc.c b/drivers/pinctrl/nxp/pinctrl-s32cc.c
index 7a38e3216b0c..9508fc1e9a90 100644
--- a/drivers/pinctrl/nxp/pinctrl-s32cc.c
+++ b/drivers/pinctrl/nxp/pinctrl-s32cc.c
@@ -39,6 +39,9 @@ 
 #define S32_MSCR_ODE		BIT(20)
 #define S32_MSCR_OBE		BIT(21)
 
+#define S32_CFG_SET		true
+#define S32_CFG_CLR		false
+
 static struct regmap_config s32_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
@@ -475,32 +478,57 @@  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_config(unsigned int *mask, unsigned int *config,
+				  unsigned int bits, bool set)
+{
+	if (set)
+		*config |= bits;
+	else
+		*config &= ~bits;
+	*mask |= bits;
+}
+
+static inline void s32_pull_enable(enum pin_config_param param,
+				   unsigned int *mask, unsigned int *config)
+{
+
+	if (param == PIN_CONFIG_BIAS_PULL_UP) {
+		s32_pin_config(mask, config, S32_MSCR_PUS | S32_MSCR_PUE,
+			       S32_CFG_SET);
+	} else if (param == PIN_CONFIG_BIAS_PULL_DOWN) {
+		*config &= ~S32_MSCR_PUS;
+		*config |= S32_MSCR_PUE;
+		*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+	}
+}
+
+static inline void s32_pull_disable(unsigned int *mask, unsigned int *config)
+{
+	s32_pin_config(mask, config, S32_MSCR_PUS | S32_MSCR_PUE, S32_CFG_CLR);
+}
+
+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:
 		return 0;
 	case PIN_CONFIG_DRIVE_OPEN_DRAIN:
-		*config |= S32_MSCR_ODE;
-		*mask |= S32_MSCR_ODE;
+		s32_pin_config(mask, config, S32_MSCR_ODE, S32_CFG_SET);
 		break;
 	case PIN_CONFIG_OUTPUT_ENABLE:
-		if (arg)
-			*config |= S32_MSCR_OBE;
-		else
-			*config &= ~S32_MSCR_OBE;
-		*mask |= S32_MSCR_OBE;
+		s32_pin_config(mask, config, S32_MSCR_OBE, S32_CFG_SET);
 		break;
 	case PIN_CONFIG_INPUT_ENABLE:
-		if (arg)
-			*config |= S32_MSCR_IBE;
-		else
-			*config &= ~S32_MSCR_IBE;
-		*mask |= S32_MSCR_IBE;
+		s32_pin_config(mask, config, S32_MSCR_IBE, S32_CFG_SET);
 		break;
 	case PIN_CONFIG_SLEW_RATE:
 		ret = s32_get_slew_regval(arg);
@@ -510,25 +538,17 @@  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_pull_enable(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_config(mask, config,
+			       S32_MSCR_ODE | S32_MSCR_OBE | S32_MSCR_IBE,
+			       S32_CFG_CLR);
+		s32_pull_disable(mask, config);
+		break;
 	case PIN_CONFIG_BIAS_DISABLE:
-		*config &= ~(S32_MSCR_PUS | S32_MSCR_PUE);
-		*mask |= S32_MSCR_PUS | S32_MSCR_PUE;
+		s32_pull_disable(mask, config);
 		break;
 	default:
 		return -EOPNOTSUPP;
@@ -554,9 +574,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;
 	}