On Tue, Dec 14, 2021 at 5:08 PM Wells Lu [off-list ref] 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