Messages in this thread | | | From | Andy Shevchenko <> | Date | Wed, 15 Dec 2021 01:14:41 +0200 | Subject | Re: [PATCH v4 2/2] pinctrl: Add driver for Sunplus SP7021 |
| |
On Tue, Dec 14, 2021 at 5:08 PM Wells Lu <wellslutw@gmail.com> wrote: > > Add driver for Sunplus SP7021 SoC.
It needs much more work, my comments below.
...
> +/* SP7021 Pin Controller Driver. > + * Copyright (C) Sunplus Tech/Tibbo Tech. > + */
This is wrong style for multi-line comments. Fix it everywhere accordingly.
...
> +#include <linux/platform_device.h> > +#include <linux/pinctrl/pinmux.h> > +#include <linux/gpio/driver.h> > +#include <linux/module.h> > +#include <linux/bitfield.h>
Keep them in order. Besides that it seems missed a few headers, such as of.h.
> + > +#include <dt-bindings/pinctrl/sppctl-sp7021.h>
+ blank line
> +#include "../pinctrl-utils.h" > +#include "../core.h"
+ blank line
> +#include "sppctl.h"
...
> + mask = GENMASK(bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT + bit_sz - 1, > + bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT);
GENMASK() with non-const arguments may be not a good choice and see, even
mask = GENMASK(bit_sz - 1, 0) << (bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT);
is way much better.
...
> + val = (reg & BIT(bit_off)) ? 1 : 0;
!!(...) may also work, but it's rather style preference.
...
> + reg = readl(spp_gchip->gpioxt_base + SPPCTL_GPIO_OFF_MASTER + reg_off);
I noticed a potentially big issue with this driver. Are you sure it's brave enough to do I/O without any synchronisation? Did I miss a lock?
...
> + reg = readl(spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OD + reg_off);
You may create an I/O wrappers to achieve a much better to read code (no repetition of this arithmetics, etc).
...
> + for (i = 0; i < chip->ngpio; i++) { > + label = gpiochip_is_requested(chip, i); > + if (!label) > + label = "";
Perhaps to show only requested ones? In such case you may use for_each_requested_gpio() macro.
> + seq_printf(s, " gpio-%03d (%-16.16s | %-16.16s)", i + chip->base, > + chip->names[i], label); > + seq_printf(s, " %c", sppctl_gpio_get_direction(chip, i) == 0 ? 'O' : 'I'); > + seq_printf(s, ":%d", sppctl_gpio_get(chip, i)); > + seq_printf(s, " %s", (sppctl_first_get(chip, i) ? "gpi" : "mux")); > + seq_printf(s, " %s", (sppctl_master_get(chip, i) ? "gpi" : "iop")); > + seq_printf(s, " %s", (sppctl_gpio_inv_get(chip, i) ? "inv" : " ")); > + seq_printf(s, " %s", (sppctl_gpio_output_od_get(chip, i) ? "oDr" : ""));
Too many parentheses in a few of above lines.
> + seq_puts(s, "\n"); > + }
...
> + dev_err_probe(&pdev->dev, -EINVAL, "Not a gpio-controller!\n"); > + return -EINVAL;
Unite them in one return statement. Ditto for zillion similar cases in this driver.
...
> + gchip->parent = &pdev->dev;
> + gchip->of_node = pdev->dev.of_node;
Drop this dup. GPIO library already does it for you.
...
> + int i = 0;
What for this assingment?
> + dev_dbg(pctldev->dev, "%s(%d, %ld, %d)\n", __func__, pin, *configs, num_configs);
Noise. Better to consider to add necessary tracepoints to pin control core.
> + /* Special handling for IOP */ > + if (configs[i] == 0xFF) {
Why out of a sudden capitilazed hex value?
> + sppctl_first_master_set(&pctl->spp_gchip->chip, pin, mux_f_gpio, mux_m_iop); > + return 0; > + }
...
> + dev_dbg(pctldev->dev, "%s(%d)\n", __func__, offset);
Noise. And so on, so on...
...
> + dev_dbg(pctldev->dev, "%s(%d), f_idx: %d, g_idx: %d, freg: %d\n", > + __func__, selector, g2fpm.f_idx, g2fpm.g_idx, f->freg);
No need to use __func__, and especially in case of _dbg / _debug. It can be enabled at run-time with help of Dynamic Debug.
...
> + seq_printf(s, "%s", dev_name(pctldev->dev));
Isn't it printed by core?
...
> +static int sppctl_dt_node_to_map(struct pinctrl_dev *pctldev, struct device_node *np_config, > + struct pinctrl_map **map, unsigned int *num_maps) > +{
Looking into this rather quite big function why you can't use what pin control core provides?
> +}
...
> + /* Fill up unique group names array. */ > + sppctl->unq_grps = devm_kzalloc(&pdev->dev, (sppctl->unq_grps_sz + 1) * > + sizeof(char *), GFP_KERNEL);
You need to use devm_kcalloc() variant for arrays.
> + if (!sppctl->unq_grps) > + return -ENOMEM;
> + sppctl->g2fp_maps = devm_kzalloc(&pdev->dev, (sppctl->unq_grps_sz + 1) * > + sizeof(struct grp2fp_map), GFP_KERNEL);
Ditto, besides the fact of better use of sizeof() of the type of variable, done by sizeof(*..._maps).
> + if (!sppctl->g2fp_maps) > + return -ENOMEM; > + > + sppctl->groups_name = devm_kzalloc(&pdev->dev, sppctl_list_funcs_sz * > + SPPCTL_MAX_GROUPS * sizeof(char *), GFP_KERNEL);
Ditto. And check some interesting macros in overflow.h.
> + if (!sppctl->groups_name) > + return -ENOMEM;
...
> + /* gpio */
GPIO, but either way seems not so valueable comment.
...
> + err = devm_pinctrl_register_and_init(&pdev->dev, &sppctl->pctl_desc, > + sppctl, &sppctl->pctl_dev); > + if (err) {
> + dev_err_probe(&pdev->dev, err, "Failed to register pinctrl!\n"); > + of_node_put(np);
Swap them and use the form of return dev_err_probe();
> + return err; > + }
...
> + /* MOON2 registers */ > + rp = platform_get_resource_byname(pdev, IORESOURCE_MEM, "moon2"); > + sppctl->moon2_base = devm_ioremap_resource(&pdev->dev, rp);
We have an API that provides two in one.
> + if (IS_ERR(sppctl->moon2_base)) { > + ret = PTR_ERR(sppctl->moon2_base);
> + goto ioremap_failed;
What is this for? Use return dev_err_probe() directly.
> + }
> + dev_dbg(&pdev->dev, "MOON2: %pr\n", rp);
This cryptic noise has to be removed.
Above comments are applicable to all similar cases.
...
> +ioremap_failed: > + dev_err_probe(&pdev->dev, ret, "ioremap failed!\n");
This doesn't bring any value, besides the fact that API you have used already prints a message.
...
> + pdev->dev.platform_data = sppctl;
Don't we have special setter for this field?
...
> + dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo Tech. (c)");
No value.
...
> + { /* zero */ }
Comment with no value.
> +};
...
> + .owner = THIS_MODULE,
It's probably 5+ years that we don't need this (it's applied implicitly).
...
> +#ifndef __SPPCTL_H__ > +#define __SPPCTL_H__
This header misses the inclusions such as bits.h. And I belive more than that.
...
> +/* FIRST register: > + * 0: MUX > + * 1: GPIO/IOP > + * 2: No change > + */
For all comments starting from here and for similar cases elsewhere: - why it is not in kernel doc? - what the value that add? (Some of them so cryptic or so obvious)
...
> +static const struct sppctl_grp sp7021grps_spif[] = { > + EGRP("SPI_FLASH1", 1, pins_spif1), > + EGRP("SPI_FLASH2", 2, pins_spif2)
Here and everywhere else, leave a comma if it's not a terminator entry.
> +};
-- With Best Regards, Andy Shevchenko
| |