Message-ID: <83d865b82e574ab49ae0c5fc160774c0@sphcmbx02.sunplus.com.tw>
Date:   Wed, 15 Dec 2021 09:40:51 +0000
From:   Wells Lu 呂芳騰 <wells.lu@...plus.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Wells Lu <wellslutw@...il.com>
CC:     Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        devicetree <devicetree@...r.kernel.org>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>,
        "dvorkin@...bo.com" <dvorkin@...bo.com>
Subject: RE: [PATCH v4 2/2] pinctrl: Add driver for Sunplus SP7021

Hi Andy,

Thank you very much for your review!

Please see my replies and questions below.


> >
> > 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.

I'll modify all multi-line comments, for example, as shown below:

/*
 * SP7021 Pin Controller Driver.
 * Copyright (C) Sunplus Tech/Tibbo Tech.
 */

Now, I realized that each subsystem has its own comment style.


> ...
> 
> > +#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.

I am not sure what order should I keep for inclusions.
Reversed x'mas tree order? Alphabetic order?

Some reviewers ask to remove unnecessary header files.
So I removed all unnecessary header files if compilation 
completes without any errors or warnings.

I suppose <linux/of.h> has included by other inclusion.
Need I add <linux/of.h> or other inclusions back?


> > +
> > +#include <dt-bindings/pinctrl/sppctl-sp7021.h>
> 
> + blank line
> 
> > +#include "../pinctrl-utils.h"
> > +#include "../core.h"
> 
> + blank line
> 
> > +#include "sppctl.h"

I'll add the blank lines.


> ...
> 
> > +       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.

I'll modify it as your suggestion:

mask = GENMASK(bit_sz - 1, 0) << (bit_off + SPPCTL_GROUP_PINMUX_MASK_SHIFT);


> ...
> 
> > +       val = (reg & BIT(bit_off)) ? 1 : 0;
> 
> !!(...) may also work, but it's rather style preference.

The return value is integer 0 or 1, not Boolean.


> ...
> 
> > +       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?

Do I need to add spin_lock() for all gpio operation functions?
Please teach me what operation functions I need to add lock or 
all operation functions need 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).

I think this is the simplest in-line form:

"spp_gchip->gpioxt2_base" is base address.
SPPCTL_GPIO_OFF_OD is register offset to base of OD (open-drain) registers.
reg_off is register offset to an OD register (SP7021 has 7 OD registers totally).

Need I add macros (wrappers) for accessing registers?

For example,

#define SPPCTL_GPIO_OD(off)	(spp_gchip->gpioxt2_base + SPPCTL_GPIO_OFF_OD + off)

reg = readl(SPPCTL_GPIO_OD(reg_off));

or 

writel(reg, SPPCTL_GPIO_OD(reg_off));


Mr. Linus Walleij told me that he likes in-line (direct) form, instead of macro
because in-line form has better readability (No need to jump to other file for
reading macros).

Could you please share me with your idea?


> ...
> 
> > +       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.

I'd like to keep showing status of all GPIOs.
This helps us know status of all GPIOs when debugging hardware issue.


> > +               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.

I'll remove extra parentheses.


> > +               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.

I'll modify code as shown below:

	return dev_err_probe(&pdev->dev, -EINVAL, "Not a gpio-controller!\n");


> ...
> 
> > +       gchip->parent =            &pdev->dev;
> 
> > +       gchip->of_node =           pdev->dev.of_node;
> 
> Drop this dup. GPIO library already does it for you.

But when I removed the two statements, I found both 'gchip->parent' and
'gchip->of_node' are always 0. No one helps set it.

Do I miss doing somethings?


> ...
> 
> > +       int i = 0;
> 
> What for this assignment?

The following statement "if (configs[i] == 0xFF) {"
needs "i" to be initialized to 0.

I'll remove the initialization and revise the statement

	if (configs[i] == 0x0ff) {

to

	if (configs[0] == 0xff) {


> > +       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.

What should I do?
Should I remove it?


> > +       /* Special handling for IOP */
> > +       if (configs[i] == 0xFF) {
> 
> Why out of a sudden capitilazed hex value?

I'll modify it to lowercase.
'configs' is set to 0xff to remember we need special process for IOP pin-mux pins.


> > +               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...

Should I remove dev_dbg? Or modify it?
But it will not print out anything in normal run, only for debugging.


> ...
> 
> > +       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.

Should I need to remove all __func__ in this driver?


> ...
> 
> > +       seq_printf(s, "%s", dev_name(pctldev->dev));
> 
> Isn't it printed by core?

I'll remove the "seq_printf(...);"


> ...
> 
> > +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?

No, we cannot use functions pin-control core provides.
Please refer to dt-bindings document, "pinctrl/sunplus,sp7021-pinctrl.yaml".
We have more explanation there.


> > +}
> ...
> 
> > +       /* 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.

I'll modify code to use devm_kcalloc().

I'll modify sizeof() to use type of variable, that is:
	sizeof(*sppctl->unq_grps)


> > +       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).

I'll modify code to use devm_kcalloc().

I'll modify sizeof() to use type of variable, that is:
	sizeof(*sppctl->g2fp_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.

I'll modify code to use devm_kcalloc().

I'll modify sizeof() to use type of variable, that is:
	sizeof(*sppctl->groups_name)

Please teach me what macros should I check?
There are many macros in overflow.h.


> > +       if (!sppctl->groups_name)
> > +               return -ENOMEM;
> ...
> 
> > +       /* gpio */
> 
> GPIO, but either way seems not so valuable comment.

I'll modify the comment to let it more meaningful.


> ...
> 
> > +       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();

I'll modify it.


> > +               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.

I'll modify code to use devm_platform_ioremap_resource_byname().


> > +       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.

There are 5 devm_ioremap_resource() in this function.
To avoid from duplication, goto an error-handling when ioremap failed.


> > +       }
> 
> > +       dev_dbg(&pdev->dev, "MOON2:   %pr\n", rp);
> 
> This cryptic noise has to be removed.
> 
> Above comments are applicable to all similar cases.

I'll remove all them.


> ...
> 
> > +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.

I'll modify code to as shown below (error-handling here):

ioremap_failed:
	return dev_err_probe(&pdev->dev, ret, "ioremap failed!\n");
}

Is this ok?
If not, please teach me how to modify.


> ...
> 
> > +       pdev->dev.platform_data = sppctl;
> 
> Don't we have special setter for this field?

I know platform_set_drvdata() function is used to set "dev->driver_data".
I cannot find a function to set "dev->platform_data".
Please teach me what function should I use to set "dev->platform_data"?


> ...
> 
> > +       dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo Tech.
> > + (c)");
> 
> No value.

This shows that pinctrl driver has probed successfully.
Many drivers show this kind of information.
Do I need to remove it? Or change to dev_dbg(...).


> ...
> 
> > +       { /* zero */ }
> 
> Comment with no value.

I'll remove the comment.


> > +};
> 
> ...
> 
> > +               .owner          = THIS_MODULE,
> 
> It's probably 5+ years that we don't need this (it's applied implicitly).

I'll remove it.


> ...
> 
> > +#ifndef __SPPCTL_H__
> > +#define __SPPCTL_H__
> 
> This header misses the inclusions such as bits.h.
> And I believe more than that.

Some reviewers ask to remove unnecessary header files.
I removed all unnecessary header files if compilation completes 
without any errors or warnings.

If compilation has done successfully, does it mean all 
necessary inclusions has included well?

Besides, before private header files are included,
Linux or system header files will be included.
No need extra inclusion here, right?


> ...
> 
> > +/* 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)

The comment explains usage of 'enum mux_f_mg'
The 'enum' is only used in the driver.
It helps programmers to remember or look-up the define of the enum.
Need we add this kind of comment to kernel doc?


> ...
> 
> > +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.

The constant array 'sp7021grps_spif[]' is declared and initialized 
to have 2 elements. 'EGRP("SPI_FLASH2", 2, pins_spif2)' is the 
latest element.
Why do we need to add 'comma' for the latest element of an array?
If we add extra comma, the array will have one more element.


> > +};
> 
> --
> With Best Regards,
> Andy Shevchenko

Best regards,
Wells Lu

Powered by blists - more mailing lists