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
