Re: [PATCH v5 2/2] pinctrl: Add driver for Sunplus SP7021

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: "Wells Lu 呂芳騰" <wells.lu@sunplus.com>
Cc: Wells Lu <wellslutw@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	devicetree <devicetree@vger.kernel.org>,
	linux-arm Mailing List <linux-arm-kernel@lists.infradead.org>,
	"dvorkin@tibbo.com" <dvorkin@tibbo.com>
Subject: Re: [PATCH v5 2/2] pinctrl: Add driver for Sunplus SP7021
Date: Mon, 10 Jan 2022 20:41:37 +0200	[thread overview]
Message-ID: <CAHp75VcPB_K6RD8tnMarwGCeaOKcQ_knxvKEW9WNn_4ce41szw@mail.gmail.com> (raw)
In-Reply-To: <f87b21407ed44630a86b2661deab4a58@sphcmbx02.sunplus.com.tw>

On Tue, Dec 28, 2021 at 5:38 PM Wells Lu 呂芳騰 <wells.lu@sunplus.com> wrote:

...

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

...

> > > +               /*
> > > +                * 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);

...

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

...

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

...

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

...

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

...

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

...

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

...

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

...

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

-- 
With Best Regards,
Andy Shevchenko