On Wed, Dec 15, 2021 at 11:40 AM Wells Lu 呂芳騰 [off-list ref] wrote:

...

quoted

quoted

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

Actually there are only two styles:
 - this one (as updated version) for entire kernel with the exception of
 - net / network one (as you used originally)

quoted

...

quoted

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

Alphabetical. When you don't see a direct answer the rule of thumb is
to go to the recent contributions and check a few new drivers to see
how they are doing.

Some reviewers ask to remove unnecessary header files.

So that reviewers are right and maybe wrong (see below for the
details), I don't see those reviews so I can't judge.

So I removed all unnecessary header files if compilation
completes without any errors or warnings.

Have you checked how deep and sudden the header inclusion went?

I suppose <linux/of.h> has included by other inclusion.

Of course and it's wrong in your case. No header from above guarantees
that. See below.

Need I add <linux/of.h> or other inclusions back?

The rule of thumb is that you have to include all headers you are a
direct user of.
There are some that guarantee inclusion of others, like
bits.h is always included if you use bitmap.h  (via bitops.h) or
compiler_attributes.h is always provided by types.h.

You have to find a golden ratio here (yes, it's kinda knowledge that
doesn't come at once, needs to have some experience).

...

quoted

quoted

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

Yes, and it's exactly what has been suggested. It's a C standard
allowed trick. With the -O2 compiler gives you the same code for both
(at least on x86).

...

quoted

quoted

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

I don't know. You need to answer this question, it's your code. And
you know how it's supposed to be used etc, etc.

...

quoted

quoted

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

No, it's harder to read and easy to mistake what the base and / or
offset is used, what value, etc.

"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?

It's an idea that is used in zillions of the drivers (and not via macros).

static inline spp_readl(struct ... *spp, u32 offset)
{
  return readl(...);
}

Same for _writel() and so on.

...

quoted

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.

Since it's a pin control driver, the pin control debug callback can
show this. For GPIO I would rather not be bothered with not requested
pins. But it's your decision.

...

quoted

quoted

+       gchip->parent =            &pdev->dev;

quoted

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

Yes, I put blank lines around the dup and left context (as a first
line) to show why the second one is a dup. When you miss something,
read the implementation code. It's open source at the end!

...

quoted

quoted

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

I wouldn't expect these to be in the production-ready driver. And
since you are committing to drivers/pinctrl and not to drivers/staging
I consider that you are trying to push this code as production-ready.

...

quoted

quoted

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

See above.

...

quoted

quoted

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

As the first step, the second one is to remove 90%+ of these messages
as I explained above.

...

quoted

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.

Fine, can you reuse some library functions, etc? Please, consider
refactoring to make it more readable.

...

quoted

quoted

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

Do your homework, it's not me who makes this contribution :-)
Hint: something about multiplication.

...

quoted

quoted

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

What error handling? You have the same point which does the same for
them, I don't see duplication avoidance. See below as well.

quoted

quoted

+       }

...

quoted

quoted

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

(1)

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?

No.

If not, please teach me how to modify.

Read again what I wrote in (1).

quoted

quoted

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

Ah, now I even may say that the above assignment is simply wrong.
It's not designed for what you think it's for. Use different ways.

...

quoted

quoted

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

And there is no value in this information.

Do I need to remove it? Or change to dev_dbg(...).

Neither in this case.

Explain the point "why?". In general you have to explain each line of
your code "why are you doing this or that?".

...

quoted

quoted

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

What do you mean by this in this case. This is a clear miss of bits.h
here as you use macros from it. Imagine if you include this file
somewhere where bits.h hasn't found its mysterious ways.

I removed all unnecessary header files if compilation completes
without any errors or warnings.

If compilation has done successfully,

So what? It doesn't mean the code is bad in one way or another, :-)

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?

See above.

...

quoted

quoted

+/* 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?

Why not?

quoted

quoted

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

To avoid the churn in the future when it will be expanded. Believe I
saw this kind of "proves" that this or that won't ever be expanded and
then... you may picture what happens.

If we add extra comma, the array will have one more element.

Yes, with touching the "last" one here. Please, add commas where it's
not a crystal clear a termination (which is not in many cases, usually
arrays with NULL entry or enums with MAX value at the end).

-- 
With Best Regards,
Andy Shevchenko