Re: [PATCH v2 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sat, Oct 23, 2021 at 9:46 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:
> On Fri, 22 Oct 2021 at 15:32, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> > On Thu, Oct 21, 2021 at 8:44 PM Emil Renner Berthing <kernel@xxxxxxxx> wrote:

...

> > > +               } else if ((npins = of_property_count_u32_elems(child, "pins")) > 0) {
> > > +                       pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
> > > +                       if (!pins)
> > > +                               goto free_grpname;
> > > +
> > > +                       pinmux = NULL;
> > > +
> > > +                       for (i = 0; i < npins; i++) {
> > > +                               u32 v;
> > > +
> > > +                               ret = of_property_read_u32_index(child, "pins", i, &v);
> > > +                               if (ret)
> > > +                                       goto free_pins;
> > > +                               pins[i] = v;
> > > +                       }
> >
> > NIH _array() APIs.
>
> .. here the pins array is an int array and not u32 array. I can cast
> it and and hope Linux will never run on a machine where sizeof(int) !=
> 4 if you think that's better?

Can you make it u32?

...

> > > +free_pinmux:
> > > +       devm_kfree(dev, pinmux);
> > > +free_pins:
> > > +       devm_kfree(dev, pins);
> > > +free_grpname:
> > > +       devm_kfree(dev, grpname);
> >
> > > +free_pgnames:
> > > +       devm_kfree(dev, pgnames);
> >
> > Just no, please get rid of them either way as I explained in previous reviews.
>
> So I asked you if you thought it was better to leave these unused
> allocations when parsing the device tree node fails but you never
> answered that. I didn't want put words in your mouth so I could only
> assume you didn't. I'd really like a straight answer to that so I have
> something to refer to when people ask why this driver doesn't do the
> same as fx. the pinctrl-single. So just to be clear: do you think it's
> better to leave this unused garbage allocated if parsing the device
> tree node fails?

If it's only one time use, I don't think it's good to have it hanging
around, BUT at the same time devm_*() is not suitable for such
allocations.

...

> > > +               if (reg_din)
> > > +                       writel_relaxed(gpio + 2, reg_din);
> >
> > Why 0 can't be written?
>
> Because signal 0 is a special "always 0" signal and signal 1 is a
> special "always 1" signal, and after that signal n is the input value
> of GPIO n - 2. We don't want to overwrite the PoR defaults.

Okay, this, perhaps, needs a comment (if I have not missed the existing one).

And what about checking for reg_din? Do you have some blocks output-only?

...

> > > +               case PIN_CONFIG_BIAS_DISABLE:
> > > +                       mask |= PAD_BIAS_MASK;
> > > +                       value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_DISABLE;
> >
> > Okay, I have got why you are masking on each iteration, but here is
> > the question, shouldn't you apply the cnages belonged to each of the
> > group of options as it's requested by the user? Here you basically
> > ignore all previous changes to bias.
> >
> > I would expect that you have something like
> >
> > for () {
> >   switch (type) {
> >   case BIAS*:
> >     return apply_bias();
> >   ...other types...
> >   default:
> >     return err;
> >   }
> > }
>
> I such cases where you get conflicting PIN_CONFIG_BIAS_* settings I
> don't see why it's better to do the rmw on the padctl register for the
> first bias setting only to then change the bits again a few
> microseconds later when the loop encounters the second bias setting.
> After the loop is done the end result would still be just the last
> bias setting.

It could be bias X followed by something else followed by bias Y. You
will write something else with bias Y. I admit I don't know this
hardware and you and maintainers are supposed to decide what's better,
but my guts are telling me that current algo is buggy.

> > > +                       break;

...

> > > +static int starfive_gpio_request(struct gpio_chip *gc, unsigned int gpio)
> > > +{
> > > +       return pinctrl_gpio_request(gc->base + gpio);
> > > +}
> > > +
> > > +static void starfive_gpio_free(struct gpio_chip *gc, unsigned int gpio)
> > > +{
> > > +       pinctrl_gpio_free(gc->base + gpio);
> > > +}
> >
> > Point of having these function is...?
>
> These calls tells the pinctrl system that a certain pin is now used
> for GPIO. Conversely it'll also prevent fx. userspace from doing GPIO
> on a pin that's already used by I2C, a UART or some other peripheral.

Isn't pin control doing it by default?

...

> > > +       /* enable input and schmitt trigger */
> >
> > Use capitalization consistently.
>
> I am?

In the comment is one style, in other comments it's another.

...

> > > +       case IRQ_TYPE_EDGE_RISING:

> > > +               handler   = handle_edge_irq;
> > > +               break;
> > > +       case IRQ_TYPE_EDGE_FALLING:

> > > +               handler   = handle_edge_irq
> > > +               break;
> > > +       case IRQ_TYPE_EDGE_BOTH:

> > > +               handler   = handle_edge_irq;
> >
> > Dup. You may do it once without any temporary variable.
> > I haven't got why you haven't addressed this.
>
> So you want two switches on the trigger variable, one for irq_type,
> edge_both and polarity, and one for the handler? If this is not what
> you have in mind please be a lot more explicit. Trying to guess what
> you mean gets really old.

switch (type) {
case bla bla bla:
  ...everything except handler...
}

if (type & EDGE)
 irq_lock(edge_handler)
else if (type & LEVEL)
 irq_lock(level_handler)

>
> > > +               break;
> > > +       case IRQ_TYPE_LEVEL_HIGH:

> > > +               handler   = handle_level_irq;
> > > +               break;
> > > +       case IRQ_TYPE_LEVEL_LOW:

> > > +               handler   = handle_level_irq;
> >
> > Ditto.
> >
> > > +               break;

...

> > > +       clk = devm_clk_get(dev, NULL);
> > > +       if (IS_ERR(clk)) {
> >
> > > +               ret = PTR_ERR(clk);
> >
> > Inline into below.
> >
> > > +               return dev_err_probe(dev, ret, "could not get clock: %d\n", ret);
> > > +       }
> >
> > Ditto for all other similar cases.
>
> So you would rather want this?
>   return dev_err_probe(dev, PTR_ERR(clk), "could not get clock: %d\n",
> PTR_ERR(clk));
> or just not tell why getting the clock failed?

Of course not, no dup of the printing error code is needed. I guess I
mentioned it in another patch.

return dev_err_probe(dev, PTR_ERR($error), "$msg\n");

...

> > > +       if (!device_property_read_u32(dev, "starfive,signal-group", &value)) {
> >
> > Since you are using of_property_* elsewhere, makes sense to use same
> > here, or otherwise, use device_*() APIs there.
>
> Wait, so now you want of_property_read_u32(dev->of_node, ...) here
> again, is that right?

Before I missed that there are other of_property_read*() calls, now
since you used them elsewhere it makes sense to be consistent over the
code.

-- 
With Best Regards,
Andy Shevchenko