| Message ID | 20240827131722.89359-3-prabhakar.mahadev-lad.rj@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pinctrl: renesas: rzg2l: Simplify noise filter cfg macro | expand |
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
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 --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);