Hi Andy,

Thank you for answers and comments.

I'll modify code accordingly and will
send a new patch when ready.


Thanks,

Wells Lu

...

quoted

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

...

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.

quoted

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.

quoted

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?

quoted

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.

quoted

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

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

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

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.

quoted

"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

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

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

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

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

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

quoted

Looking into this rather quite big function why you can't use what pin control core

provides?

quoted

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

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

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

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)

quoted

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.

quoted

If not, please teach me how to modify.

Read again what I wrote in (1).

quoted

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

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.

quoted

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

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.

quoted

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, :-)

quoted

does it mean all
necessary inclusions has included well?

quoted

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

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

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.

quoted

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