On Wed, Dec 15, 2021 at 11:40 AM Wells Lu 呂芳騰 <wells.lu@xxxxxxxxxxx> wrote: ... > > > +/* 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) > > ... > > > > > +#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). ... > > > + 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). ... > > > + 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. ... > > > + 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. ... > > 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. ... > > > + 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? 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! ... > > > + 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. ... > > > + 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. ... > > > + 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. ... > > 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. ... > > > + 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. ... > > > + 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. > > > + } ... > > > +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). > > > + 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. ... > > > + 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?". ... > > > +#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. ... > > > +/* 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? > > > +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