Hi Simon,
On Fri, Feb 15, 2019 at 9:17 AM Simon Horman <[email protected]> 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 <[email protected]>
>
> 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 <[email protected]>
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 -- [email protected]
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
