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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VcTLjtkKx46fye_NCtHG4ofYPtnA1RTjE0m_PDXPJKa5Q@mail.gmail.com>
Date:   Tue, 14 Mar 2023 19:15:48 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Chester Lin <clin@...e.com>
Cc:     Linus Walleij <linus.walleij@...aro.org>,
        NXP S32 Linux Team <s32@....com>,
        linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        Ghennadi Procopciuc <Ghennadi.Procopciuc@....nxp.com>,
        Andrei Stefanescu <andrei.stefanescu@....com>,
        Radu Pirea <radu-nicolae.pirea@....com>,
        Andreas Färber <afaerber@...e.de>,
        Matthias Brugger <mbrugger@...e.com>
Subject: Re: [PATCH 2/3] pinctrl: s32cc: refactor pin config parsing

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

-- 
With Best Regards,
Andy Shevchenko