Message-ID: <cf53f5dc57e342078ec14a771ba639ca@sphcmbx02.sunplus.com.tw>
Date:   Thu, 13 Jan 2022 17:04:32 +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 v5 2/2] pinctrl: Add driver for Sunplus SP7021

> ...
> 
> > > > +       bool "Sunplus SP7021 PinMux and GPIO driver"
> > >
> > > Why bool and not tristate?
> >
> > Pinctrl driver is selected by many drivers in SP7021 platform.
> > We never build it as a module, but build-in to kernel.
> > So we use "bool".
> >
> > Should we set it to tristate?
> 
> You still haven't answered "why", so I can't tell you.

I am puzzled because I think I have answered "why".

Because Pinctrl driver is necessary for all SP7021-based platforms.
Pinctrl driver is always built into kernel, never be built as module.
We use "bool" and select "y" for all platforms. No need "tristate".

"bool" means you can select "Y" for built in kernel or "N" not built.
"tristate" means besides "Y" and "N", you can also select "M" to 
build as module which can be loaded or unloaded manually.

Sorry, I really don't know what more I can answer.
Could you please give me some hits?


> ...
> 
> > > > +               /*
> > > > +                * Upper 16-bit word is mask. Lower 16-bit word is value.
> > > > +                * Refer to descriptions of function sppctl_master_get().
> > > > +                */
> > > > +               reg_off = (offset / 16) * 4;
> > > > +               bit_off = offset % 16;
> > > > +               reg = BIT(bit_off + SPPCTL_GPIO_MASK_SHIFT) |
> > > > + BIT(bit_off);
> > >
> > > As I commented above use helper function which takes offset as input
> > > and returns you reg and reg_off.
> >
> > I'll modify code as shown below:
> >
> >                 reg = SPPCTL_SET_MOON_REG_BIT(bit_off);
> >
> > Sorry, I don't understand your meaning "returns you reg and reg_off".
> > The helper macro will return reg but not reg_off, right?
> 
> Something like (fix types accordingly to your needs):
> 
> static inline u32 sppctl_get_reg_and_offset(unsigned int offset, u32 *roff) {
>               u32 boff = offset % 16;
>               *roff = (offset / 16) * 4;
> 
>                return  MY_COOL_MACRO(boff); // BIT(boff +
> SPPCTL_GPIO_MASK_SHIFT) | BIT(boff)
> }
> 
>     reg = sppctl_get_reg_and_offset(offset, &reg_off);

Thanks for sharing the code.
I'll add the inline function.


> ...
> 
> > > > +       if (!of_find_property(pdev->dev.of_node, "gpio-controller", NULL))
> > > > +               return dev_err_probe(&pdev->dev, -EINVAL, "Not a
> > > > + gpio-controller!\n");
> > >
> > > Why do you need this check for?
> >
> > By referring to other pinctrl driver, we check if property "gpio-controller" exists?
> > Will core help us check this?
> > Is this redundant?
> 
> You should answer this question, not me.

I'll remove the statements.
Only need to check if "gpio-controller" exists or not for child nodes.


> ...
> 
> > Should I also remove the assignment:
> >
> >                 gchip->base             = 0;
> 
> Actually this is a good catch. Why do you use 0 as a base? In the new code we are supposed
> to have -1 to be assigned.

I'll modify the statement to:

	gchip->base		= -1;

to "request dynamic ID allocation".

And I'll remove the statement:

	pctl->pctl_grange.base = gchip->base;

because the whole structure (*pctl) is initialized to zero already.


> ...
> 
> > > > +       case pinmux_type_fpmx:  /* fully-pinmux */
> > >
> > > Why do you need these comments?
> > > Shouldn't you rather to kernel doc your enum entries?
> >
> > I'll remove the comments.
> > Could you please tell me where I should write and put my kernel doc?
> > Is there any examples I can refer to?
> 
> In the enum definition you do something like this (and read documentation):
> 
> /**
>  * enum ...
>  * @pinmux_type_fpmx: fully pin muxing
>  * @pinmux_type_grp: group pin muxing
>  * ...
>  */

I see, thanks!
 
I searched and found the article "Writing kernel-doc comment" in:
https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html

I thought kernel-doc is a document put in folder Documentation/.
Now I know they are comments prefixed with /** and embedded
in sources.


> ...
> 
> > > > +       if (unlikely(check_mul_overflow(sppctl->unq_grps_sz + 1,
> > > > +                                       sizeof(*sppctl->g2fp_maps), &prod)))
> > > > +               return -EINVAL;
> > >
> > > What the point to check it after? What the point to use it with
> > > kcalloc()? Please, do your homework, i.e. read the code which implements that.
> >
> > I'll remove the "if (unlikely(check_mul_overflow()...) return -EINVAL" statement
> next patch.
> >
> > I think I mis-understood your previous comment.
> > I thought I was asked to add check_mul_overflow() function for devm_kcalloc(...).
> > Sorry for strange codes.
> 
> There were basically two iterative comments, i.e.
> first one suggested adding a check, but second one suggested switching to kcalloc()
> API.
> 
> > I should study devm_kcalloc() furthermore. Now I know
> > devm_kcalloc(...) does multiplication overflow check for us. That's
> > why we need to replace devm_kzalloc() with devm_kcalloc().
> >
> > One question left in my mind is, in this case, even we have 10,000
> > pins, we will never get overflow. It looks not so necessary.
> 
> But it's not your issue, the kcalloc() does it for you for the good sake.
>
> ...
> 
> > > > +       struct device_node *np = of_node_get(pdev->dev.of_node);
> > >
> > > What's the role of of_node_get()?
> >
> > I'll remove the unused codes.
> > I think it was used to check if OF node exists.
> 
> And if it doesn't, what is the difference?
> 
> You are the author of this code, please be prepared to explain every line in it.

From kernel-doc comment, of_node_get() increments refcount of a node.
I think as a platform driver, we don't need to check if the node exists or not.
If not exist, platform driver will not be probed.


> ...
> 
> > > > +       dev_info(&pdev->dev, "SP7021 PinCtrl by Sunplus/Tibbo
> > > > + Tech.");
> > >
> > > Is it useful?
> >
> > I think yes. It tells users that Pinctrl driver has probed successfully.
> > If no this message, users don't know if Pinctrl driver has probed
> > successfully or not. For example, because that dts node of pinctrl is
> > "disabled" or Pinctrl driver is even not enabled.
> >
> > Can I keep this?
> 
> You can, but I think it's not needed.
> Users may easily get this from other sources. Why do you need to have such noise in
> the valuable resource, i.e. kernel message buffer?

I'll remove the dev_info() next patch.


> ...
> 
> > > > + *    - mux_f_mux:  Select the pin to a fully-pinmux pin
> > > > + *    - mux_f_gpio: Select the pin to a GPIO or IOP pin
> > > > + *    - mux_f_keep: Don't change (keep intact)
> 
> > > > +       mux_f_mux = 0,          /* select fully-pinmux       */
> > > > +       mux_f_gpio = 1,         /* select GPIO or IOP pinmux */
> > > > +       mux_f_keep = 2,         /* keep no change            */
> 
> These comments are replaced by the kernel doc above, no need to keep them.

I'll remove them next patch.


> ...
> 
> > > Why is this in the header?
> >
> > Do you mean I need to move this "struct sppctl_gpio_chip { ... }"
> > declaration to c file because it is only used by the c file?
> 
> Yes.

But "struct sppctl_gpio_chip" is not only used in c file, but also used in the
same header file just beneath it. Refer to code below:

struct sppctl_gpio_chip {
	:
	:
};

struct sppctl_pdata {
	:
	:
	struct sppctl_gpio_chip *spp_gchip;
	:
	:
};


> ...
> 
> > Your previous comments:
> > > > > > +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.
> > >
> > > Fine, can you reuse some library functions, etc? Please, consider refactoring to
> make it more readable.
> >
> > Could you please share me your idea about "refactoring"?
> > Or could you give me some hints?
> > I think many times, but have no idea about refactoring.
> 
> Just split it to a few logical parts so that code can be easier to read.

I see. I'll do it, thanks.

> --
> With Best Regards,
> Andy Shevchenko

Thanks,
Wells Lu

Powered by blists - more mailing lists