Message-ID: <6a387c61f561488fb21392b5e9462dc1@sphcmbx02.sunplus.com.tw>
Date:   Wed, 15 Dec 2021 15:32:55 +0000
From:   Wells Lu 呂芳騰 <wells.lu@...plus.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     Wells Lu <wellslutw@...il.com>,
        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 for answers and comments.

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


Thanks,

Wells Lu

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

Powered by blists - more mailing lists