diff mbox series

[2/2] pinctrl: renesas: rzg2l: Move pinconf_to_config_argument() call outside of switch cases

Message ID 20240827131722.89359-3-prabhakar.mahadev-lad.rj@bp.renesas.com
State New
Headers show
Series pinctrl: renesas: rzg2l: Simplify noise filter cfg macro | expand

Commit Message

Lad, Prabhakar Aug. 27, 2024, 1:17 p.m. UTC 
From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Refactor the `rzg2l_pinctrl_pinconf_set()` function by moving the call to
`arg = pinconf_to_config_argument(_configs[i])` to the beginning of the
loop. Previously, this call was redundantly made in each case of the
switch statement.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/pinctrl/renesas/pinctrl-rzg2l.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

Comments

Geert Uytterhoeven Aug. 29, 2024, 1:15 p.m. UTC | #1 
Hi Prabhakar,

Thanks for your patch!

On Tue, Aug 27, 2024 at 3:17 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Refactor the `rzg2l_pinctrl_pinconf_set()` function by moving the call to
> `arg = pinconf_to_config_argument(_configs[i])` to the beginning of the
> loop. Previously, this call was redundantly made in each case of the
> switch statement.

This is not 100% true: the PIN_CONFIG_BIAS_* cases do not
call pinconf_to_config_argument(). But I agree that calling it
unconditionally doesn't harm.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> @@ -1395,7 +1395,6 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
>                         break;
>
>                 case PIN_CONFIG_OUTPUT_ENABLE:
> -                       arg = pinconf_to_config_argument(_configs[i]);
>                         if (!(cfg & PIN_CFG_OEN))
>                                 return -EINVAL;
>                         if (!pctrl->data->oen_write)

Missed opportunity for simplification:

                case PIN_CONFIG_POWER_SOURCE:
-                       settings.power_source =
pinconf_to_config_argument(_configs[i]);
+                       settings.power_source = arg;
                        break;

> @@ -1432,8 +1429,6 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
>                         break;
>
>                 case PIN_CONFIG_DRIVE_STRENGTH:
> -                       arg = pinconf_to_config_argument(_configs[i]);
> -
>                         if (!(cfg & PIN_CFG_IOLH_A) || hwcfg->drive_strength_ua)
>                                 return -EINVAL;
>

                case PIN_CONFIG_DRIVE_STRENGTH_UA:
                        ...
-                       settings.drive_strength_ua =
pinconf_to_config_argument(_configs[i]);
+                       settings.drive_strength_ua = arg;
                        break;

The rest LGTM.

Gr{oetje,eeting}s,

                        Geert
Lad, Prabhakar Aug. 29, 2024, 7:17 p.m. UTC | #2 
Hi Geert,

Thank you for the review.

On Thu, Aug 29, 2024 at 2:15 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> Thanks for your patch!
>
> On Tue, Aug 27, 2024 at 3:17 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Refactor the `rzg2l_pinctrl_pinconf_set()` function by moving the call to
> > `arg = pinconf_to_config_argument(_configs[i])` to the beginning of the
> > loop. Previously, this call was redundantly made in each case of the
> > switch statement.
>
> This is not 100% true: the PIN_CONFIG_BIAS_* cases do not
> call pinconf_to_config_argument(). But I agree that calling it
> unconditionally doesn't harm.
>
Ok, I'll update the commit description to below:

Refactor the `rzg2l_pinctrl_pinconf_set()` function by moving the call to
`arg = pinconf_to_config_argument(_configs[i])` to the beginning of the
loop. Previously, this call was redundantly made in most cases within the
switch statement.

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -1395,7 +1395,6 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
> >                         break;
> >
> >                 case PIN_CONFIG_OUTPUT_ENABLE:
> > -                       arg = pinconf_to_config_argument(_configs[i]);
> >                         if (!(cfg & PIN_CFG_OEN))
> >                                 return -EINVAL;
> >                         if (!pctrl->data->oen_write)
>
> Missed opportunity for simplification:
>
>                 case PIN_CONFIG_POWER_SOURCE:
> -                       settings.power_source =
> pinconf_to_config_argument(_configs[i]);
> +                       settings.power_source = arg;
>                         break;
>
And while at it I'll replace as above and below in the v2.

Cheers,
Prabhakar

> > @@ -1432,8 +1429,6 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
> >                         break;
> >
> >                 case PIN_CONFIG_DRIVE_STRENGTH:
> > -                       arg = pinconf_to_config_argument(_configs[i]);
> > -
> >                         if (!(cfg & PIN_CFG_IOLH_A) || hwcfg->drive_strength_ua)
> >                                 return -EINVAL;
> >
>
>                 case PIN_CONFIG_DRIVE_STRENGTH_UA:
>                         ...
> -                       settings.drive_strength_ua =
> pinconf_to_config_argument(_configs[i]);
> +                       settings.drive_strength_ua = arg;
>                         break;
>
> The rest LGTM.
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> 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
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
index 8fc1f28d02d1..e742a4e3eb4a 100644
--- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
+++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
@@ -1384,9 +1384,9 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 
 	for (i = 0; i < num_configs; i++) {
 		param = pinconf_to_config_param(_configs[i]);
+		arg = pinconf_to_config_argument(_configs[i]);
 		switch (param) {
 		case PIN_CONFIG_INPUT_ENABLE:
-			arg = pinconf_to_config_argument(_configs[i]);
 
 			if (!(cfg & PIN_CFG_IEN))
 				return -EINVAL;
@@ -1395,7 +1395,6 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 			break;
 
 		case PIN_CONFIG_OUTPUT_ENABLE:
-			arg = pinconf_to_config_argument(_configs[i]);
 			if (!(cfg & PIN_CFG_OEN))
 				return -EINVAL;
 			if (!pctrl->data->oen_write)
@@ -1410,8 +1409,6 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 			break;
 
 		case PIN_CONFIG_SLEW_RATE:
-			arg = pinconf_to_config_argument(_configs[i]);
-
 			if (!(cfg & PIN_CFG_SR) || arg > 1)
 				return -EINVAL;
 
@@ -1432,8 +1429,6 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 			break;
 
 		case PIN_CONFIG_DRIVE_STRENGTH:
-			arg = pinconf_to_config_argument(_configs[i]);
-
 			if (!(cfg & PIN_CFG_IOLH_A) || hwcfg->drive_strength_ua)
 				return -EINVAL;
 
@@ -1457,8 +1452,6 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 			break;
 
 		case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS:
-			arg = pinconf_to_config_argument(_configs[i]);
-
 			if (!(cfg & PIN_CFG_IOLH_B) || !hwcfg->iolh_groupb_oi[0])
 				return -EINVAL;
 
@@ -1476,7 +1469,6 @@  static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
 			if (!(cfg & PIN_CFG_IOLH_RZV2H))
 				return -EINVAL;
 
-			arg = pinconf_to_config_argument(_configs[i]);
 			if (arg > 3)
 				return -EINVAL;
 			rzg2l_rmw_pin_config(pctrl, IOLH(off), bit, IOLH_MASK, arg);