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

[Emil Renner Berthing <kernel@xxxxxxxx>]

On Sat, 23 Oct 2021 at 22:29, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
> 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?

No, the pinctrl API takes an int array.

> > > > +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.

So is that a yes or a no to my question? It's not clear to me.

> > > > +               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?

I don't know know what you mean by the first question, but yes fx. the
uart tx pins would be an example of pins that have their output signal
set to the uart peripheral, the output enable set to the special
"always enabled" signal, and no input signal is set to any of the tx
pins.

> > > > +               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.

So there is only one padctl register pr. pin. I don't see why first
setting the bias bits to X, then setting some other bits, and then
setting the bias bits to Y would be different from just setting all
the bits in one go. Except for during that little microsecond window
during the loop that I actually think it's better to avoid.

> > > > +                       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?

I actually tested that before posting and the answer seems to be no.

> ...
>
> > > > +       /* enable input and schmitt trigger */
> > >
> > > Use capitalization consistently.
> >
> > I am?
>
> In the comment is one style, in other comments it's another.

There are documentation comments in the beginning of the file and then
there are code comments in the code. I think it's a lot easier to read
like that, but if you insist I can lowercase the documentation.

> > > > +       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)

If you look at include/dt-bindings/interrupt-controller/irq.h I don't
think such a mask exists. I guess we could do

if (trigger & IRQ_TYPE_EDGE_BOTH)
  edge_handler
else if (trigger == IRQ_TYPE_LEVEL_HIGH || trigger == IRQ_TYPE_LEVEL_LOW)
 level_handler
else
  return -EINVAL

..but at that point I think it's probably easer to read a 2nd switch
case or just leave the code as it is. What do you think?

> >
> > > > +               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.

Gotcha.
> --
> With Best Regards,
> Andy Shevchenko