[PATCH v4 2/2] pinctrl: Add driver for Sunplus SP7021

[Andy Shevchenko]

On Tue, Dec 14, 2021 at 5:08 PM Wells Lu <wellslutw at 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