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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux