Re: [PATCH v3] pinctrl: renesas: checker: Rework drive and bias pin iteration

[Geert Uytterhoeven <geert@linux-m68k.org>]

On Tue, Apr 19, 2022 at 10:08 AM Geert Uytterhoeven
<geert+renesas@glider.be> wrote:
> The checker code to iterate over all drive strength and bias register
> description items is cumbersome, due to the repeated calculation of
> indices, and the use of hardcoded array sizes.  The latter was done
> under the assumption they would never need to be changed, which turned
> out to be false.
>
> Increase readability by introducing helper macros to access drive
> strength and bias register description items.
> Increase maintainability by replacing hardcoded numbers by array sizes
> calculated at compile-time.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v3:
>   - Add and use drive_ofs() and bias_ofs() helpers, as suggested by
>     Wolfram,

> --- a/drivers/pinctrl/renesas/core.c
> +++ b/drivers/pinctrl/renesas/core.c
> @@ -1007,7 +1007,18 @@ static void __init sh_pfc_compare_groups(const char *drvname,
>  static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info)
>  {
>         const struct pinmux_drive_reg *drive_regs = info->drive_regs;
> +#define drive_nfields  ARRAY_SIZE(drive_regs->fields)
> +#define drive_ofs(i)   drive_regs[(i) / drive_nfields]
> +#define drive_reg(i)   drive_ofs(i).reg
> +#define drive_bit(i)   ((i) % drive_nfields)
> +#define drive_field(i) drive_ofs(i).fields[drive_bit(i)]
>         const struct pinmux_bias_reg *bias_regs = info->bias_regs;
> +#define bias_npins     ARRAY_SIZE(bias_regs->pins)
> +#define bias_ofs(i)    bias_regs[(i) / bias_npins]
> +#define bias_puen(i)   bias_ofs(i).puen
> +#define bias_pud(i)    bias_ofs(i).pud
> +#define bias_bit(i)    ((i) % bias_npins)
> +#define bias_pin(i)    bias_ofs(i).pins[bias_bit(i)]
>         const char *drvname = info->name;
>         unsigned int *refcnts;
>         unsigned int i, j, k;

> @@ -1164,20 +1175,17 @@ static void __init sh_pfc_check_info(const struct sh_pfc_soc_info *info)
>         for (i = 0; drive_regs && drive_regs[i].reg; i++)
>                 sh_pfc_check_drive_reg(info, &drive_regs[i]);
>
> -       for (i = 0; drive_regs && drive_regs[i / 8].reg; i++) {
> -               if (!drive_regs[i / 8].fields[i % 8].pin &&
> -                   !drive_regs[i / 8].fields[i % 8].offset &&
> -                   !drive_regs[i / 8].fields[i % 8].size)
> +       for (i = 0; drive_regs && drive_reg(i); i++) {
> +               if (!drive_field(i).pin && !drive_field(i).offset &&
> +                   !drive_field(i).size)
>                         continue;
>
>                 for (j = 0; j < i; j++) {
> -                       if (drive_regs[i / 8].fields[i % 8].pin ==
> -                           drive_regs[j / 8].fields[j % 8].pin &&
> -                           drive_regs[j / 8].fields[j % 8].offset &&
> -                           drive_regs[j / 8].fields[j % 8].size) {
> +                       if (drive_field(i).pin == drive_field(j).pin &&
> +                           drive_field(j).offset && drive_field(j).size) {
>                                 sh_pfc_err("drive_reg 0x%x:%u/0x%x:%u: pin conflict\n",
                                                             ^^      ^^
> -                                          drive_regs[i / 8].reg, i % 8,
> -                                          drive_regs[j / 8].reg, j % 8);
> +                                          drive_reg(i), drive_bit(i),
> +                                          drive_reg(j), drive_bit(j));

Whoops, as reported by kernel test robot for 64-bit builds, drive_bit()
is no longer unsigned int, but size_t[*], hence "%zu" should be used
for printing.  The same is true for bias_bit().

Will fix up tomorrow...

[*] A bit counter-intuitive from the mathematical point of view,
    but as "size_t" is either "unsigned int" or "unsigned long",
    "unsigned int % size_t" is ... "size_t"!

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