Re: [PATCH v2 02/10] pinctrl: sh-pfc: Validate pinmux tables at runtime when debugging

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Simon,

On Fri, Feb 15, 2019 at 9:17 AM Simon Horman <horms@xxxxxxxxxxxx> wrote:
> On Fri, Jan 25, 2019 at 05:52:57PM +0100, Geert Uytterhoeven wrote:
> > Perform some basic sanity checks on all built-in pinmux tables when
> > DEBUG is defined, to help catching bugs early.
> >
> > For now the following checks are included:
> >   - Check register and field widths in descriptors for config registers
> >     with variable-width fields,
> >   - Check relations between pin groups and functions:
> >       - All pin functions must refer to existing pin groups,
> >       - All pin groups must be referred to by a pin function,
> >       - Warn if a pin group is referred to by multiple pin functions
> >       (which is OK for backwards-compatibility aliases),
> >   - Provide suggestions for reducing table sizes: reserved fields of
> >     more than 3 bits can better be split in smaller subfields, as the
> >     storage need is proportional to the square of the width of the
> >     (sub)field,
> >
> > Note that a dummy non-matching entry is added to the DT match table for
> > checking r8a7795es1_pinmux_info, as R-Car H3 ES1.0 is matched using
> > soc_device_match() in r8a7795_pinmux_init(), instead of by the DT match
> > table.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
>
> Hi Geert,
>
> I noted a few nits below, but in writing them I got to feel
> that perhaps its just me not appreciating the merit of
> some of the idioms used. So feel free to ignore them.
>
> Nits aside,
>
> Reviewed-by: Simon Horman <horms+renesas@xxxxxxxxxxxx>

Thank you!

> > --- a/drivers/pinctrl/sh-pfc/core.c
> > +++ b/drivers/pinctrl/sh-pfc/core.c

> > +static void sh_pfc_check_cfg_reg(const char *drvname,
> > +                              const struct pinmux_cfg_reg *cfg_reg)
> > +{
> > +     unsigned int i, n, rw, fw;
> > +
> > +     if (cfg_reg->field_width) {
> > +             /* Checked at build time */
> > +             return;
> > +     }
> > +
> > +     for (i = 0, n = 0, rw = 0; (fw = cfg_reg->var_field_width[i]); i++) {
>
> nit: The assignment in the conditional expression in the for loop is
>      perhaps not the clearest way to express this, and its

The alternative is:

        for (i = 0, n = 0, rw = 0; cfg_reg->var_field_width[i]; i++) {
                fw = cfg_reg->var_field_width[i];

Or a while (1) { ... } with a break.
IMHO all more ugly and/or less concise than the above.

>      not clear that the parentheses are required.

Oh yes, just like in the conditions of "if" and "while" statements:

    warning: suggest parentheses around assignment used as truth value

>
> > +             if (fw > 3 && is0s(&cfg_reg->enum_ids[n], 1 << fw)) {
> > +                     pr_warn("%s: reg 0x%x: reserved field [%u:%u] can be split to reduce table size\n",
> > +                             drvname, cfg_reg->reg, rw, rw + fw - 1);
> > +                     sh_pfc_warnings++;
> > +             }
> > +             n += 1 << fw;
> > +             rw += fw;
> > +     }

> > +static void sh_pfc_check_info(const struct sh_pfc_soc_info *info)
> > +{
> > +     const struct sh_pfc_function *func;
> > +     const char *drvname = info->name;
> > +     unsigned int *refcnts;
> > +     unsigned int i, j, k;
> > +
> > +     pr_info("Checking %s\n", drvname);
> > +
> > +     /* Check groups and functions */
> > +     refcnts = kcalloc(info->nr_groups, sizeof(*refcnts), GFP_KERNEL);
> > +     if (!refcnts)
> > +             return;
> > +
> > +     for (i = 0; func = &info->functions[i], i < info->nr_functions; i++) {
>
> nit: It might be clearer to set (and declare) func inside the loop.

I agree (probably it made sense at some point during development).

> > +             for (j = 0; j < func->nr_groups; j++) {
> > +                     for (k = 0; k < info->nr_groups; k++) {
> > +                             if (!strcmp(func->groups[j],
> > +                                         info->groups[k].name)) {
> > +                                     refcnts[k]++;
> > +                                     goto next;
>
> nit: goto

Yeah, it's a pity C, unlike PERL, doesn't have "next <LABEL>", to escape
out of one or more loop constructs ;-)

The alternative is "break"...

>
> > +                             }
> > +                     }

... and wrap the below in "if (k == info->nr_groups) { ... }", which makes
sense, as it still doesn't make indentation too deep (probably it did at
some point during development).

> > +
> > +                     pr_err("%s: function %s: group %s not found\n",
> > +                            drvname, func->name, func->groups[j]);
> > +                     sh_pfc_errors++;
> > +
> > +next:                        ;
> > +             }

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

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