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:
> > + for_each_child_of_node(np, child) {
> > + int npins;
> > + int i;
> > +
> > + ret = -ENOMEM;
> > + grpname = devm_kasprintf(dev, GFP_KERNEL, "%s.%s", np->name, child->name);
> > + if (!grpname)
> > + goto put_child;
> > +
> > + pgnames[ngroups++] = grpname;
> > +
> > + if ((npins = of_property_count_u32_elems(child, "pinmux")) > 0) {
> > + pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
> > + if (!pins)
> > + goto free_grpname;
> > +
> > + pinmux = devm_kcalloc(dev, npins, sizeof(*pinmux), GFP_KERNEL);
> > + if (!pinmux)
> > + goto free_pins;
> > +
> > + for (i = 0; i < npins; i++) {
> > + u32 v;
> > +
> > + ret = of_property_read_u32_index(child, "pinmux", i, &v);
> > + if (ret)
> > + goto free_pinmux;
> > + pins[i] = starfive_gpio_to_pin(sfp, starfive_pinmux_to_gpio(v));
> > + pinmux[i] = v;
> > + }
>
> Why you can't use of_property_read_u32_array() APIs?
I can here, but..
> > + map[nmaps].type = PIN_MAP_TYPE_MUX_GROUP;
> > + map[nmaps].data.mux.function = np->name;
> > + map[nmaps].data.mux.group = grpname;
> > + nmaps += 1;
> > + } 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?
> > + } else {
> > + ret = -EINVAL;
> > + goto free_grpname;
> > + }
> > +
> > + ret = pinctrl_generic_add_group(pctldev, grpname, pins, npins, pinmux);
> > + if (ret < 0) {
> > + dev_err(dev, "error adding group %pOFn.%pOFn: %d\n",
> > + np, child, ret);
> > + goto free_pinmux;
> > + }
> > +
> > + ret = pinconf_generic_parse_dt_config(child, pctldev,
> > + &map[nmaps].data.configs.configs,
> > + &map[nmaps].data.configs.num_configs);
> > + if (ret) {
> > + dev_err(dev, "invalid pinctrl group %pOFn.%pOFn: %s\n",
> > + np, child, "error parsing pin config");
> > + goto put_child;
> > + }
> > +
> > + /* don't create a map if there are no pinconf settings */
> > + if (map[nmaps].data.configs.num_configs == 0)
> > + continue;
> > +
> > + map[nmaps].type = PIN_MAP_TYPE_CONFIGS_GROUP;
> > + map[nmaps].data.configs.group_or_pin = grpname;
> > + nmaps += 1;
> > + }
>
> ...
>
> > +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?
> > + raw_spin_lock_irqsave(&sfp->lock, flags);
> > + writel_relaxed(dout, reg_dout);
> > + writel_relaxed(doen, reg_doen);
> > + 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.
> > + mask = 0;
> > + value = 0;
> > + for (i = 0; i < num_configs; i++) {
> > + int param = pinconf_to_config_param(configs[i]);
> > + u32 arg = pinconf_to_config_argument(configs[i]);
> >
> +
> > + switch (param) {
> > + 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.
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_DOWN:
> > + if (arg == 0)
> > + return -ENOTSUPP;
> > + mask |= PAD_BIAS_MASK;
> > + value = (value & ~PAD_BIAS_MASK) | PAD_BIAS_PULL_DOWN;
> > + break;
> > + case PIN_CONFIG_BIAS_PULL_UP:
> > + if (arg == 0)
> > + return -ENOTSUPP;
> > + mask |= PAD_BIAS_MASK;
> > + value = value & ~PAD_BIAS_MASK;
> > + break;
> > + case PIN_CONFIG_DRIVE_STRENGTH:
> > + mask |= PAD_DRIVE_STRENGTH_MASK;
> > + value = (value & ~PAD_DRIVE_STRENGTH_MASK) |
> > + starfive_drive_strength_from_max_mA(arg);
> > + break;
> > + case PIN_CONFIG_INPUT_ENABLE:
> > + mask |= PAD_INPUT_ENABLE;
> > + if (arg)
> > + value |= PAD_INPUT_ENABLE;
> > + else
> > + value &= ~PAD_INPUT_ENABLE;
> > + break;
> > + case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > + mask |= PAD_INPUT_SCHMITT_ENABLE;
> > + if (arg)
> > + value |= PAD_INPUT_SCHMITT_ENABLE;
> > + else
> > + value &= ~PAD_INPUT_SCHMITT_ENABLE;
> > + break;
> > + case PIN_CONFIG_SLEW_RATE:
> > + mask |= PAD_SLEW_RATE_MASK;
> > + value = (value & ~PAD_SLEW_RATE_MASK) |
> > + ((arg << PAD_SLEW_RATE_POS) & PAD_SLEW_RATE_MASK);
> > + break;
> > + case PIN_CONFIG_STARFIVE_STRONG_PULL_UP:
> > + if (arg) {
> > + mask |= PAD_BIAS_MASK;
> > + value = (value & ~PAD_BIAS_MASK) |
> > + PAD_BIAS_STRONG_PULL_UP;
> > + } else {
> > + mask |= PAD_BIAS_STRONG_PULL_UP;
> > + value = value & ~PAD_BIAS_STRONG_PULL_UP;
> > + }
> > + break;
> > + default:
> > + return -ENOTSUPP;
> > + }
> > + }
>
> ...
>
> > +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.
> > + /* enable input and schmitt trigger */
>
> Use capitalization consistently.
I am?
> > + case IRQ_TYPE_EDGE_RISING:
> > + irq_type = mask; /* 1: edge triggered */
> > + edge_both = 0; /* 0: single edge */
> > + polarity = mask; /* 1: rising edge */
> > + handler = handle_edge_irq;
> > + break;
> > + case IRQ_TYPE_EDGE_FALLING:
> > + irq_type = mask; /* 1: edge triggered */
> > + edge_both = 0; /* 0: single edge */
> > + polarity = 0; /* 0: falling edge */
> > + handler = handle_edge_irq
> > + break;
> > + case IRQ_TYPE_EDGE_BOTH:
> > + irq_type = mask; /* 1: edge triggered */
> > + edge_both = mask; /* 1: both edges */
> > + polarity = 0; /* 0: ignored */
> > + 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.
> > + break;
> > + case IRQ_TYPE_LEVEL_HIGH:
> > + irq_type = 0; /* 0: level triggered */
> > + edge_both = 0; /* 0: ignored */
> > + polarity = mask; /* 1: high level */
> > + handler = handle_level_irq;
> > + break;
> > + case IRQ_TYPE_LEVEL_LOW:
> > + irq_type = 0; /* 0: level triggered */
> > + edge_both = 0; /* 0: ignored */
> > + polarity = 0; /* 0: low level */
> > + 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?
> > + 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?
/Emil
