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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Tue, Mar 14, 2023 at 3:47 PM Chester Lin <clin@xxxxxxxx> 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.

-- 
With Best Regards,
Andy Shevchenko