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

From: "Wells Lu 呂芳騰" <wells.lu@sunplus.com>
To: Andy Shevchenko <andy.shevchenko@gmail.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: Fri, 14 Jan 2022 02:51:55 +0000	[thread overview]
Message-ID: <9e15ccc8ee844f1eab320001bc8bc235@sphcmbx02.sunplus.com.tw> (raw)
In-Reply-To: <CAHp75Vf0=Sf8sGtgCo7bMjVFGYDcJOasLqdSHTnQ0YPgSbrr2g@mail.gmail.com>

> > > > > > +       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".
> 
> Nope. :-)
> 
> > Because Pinctrl driver is necessary for all SP7021-based platforms.
> 
> "Why?" Why is it necessary (to be built-in)?

Pinctrl is necessary to be built-in because drivers of boot-device, 
like eMMC, SD card, NAND flash, and NOR flash drivers, need it.

SP7021 supports booting from eMMC, SD card, NAND flash and NOR flash 
devices. Their drivers need Pinctrl driver probes in advance.

> ...
> 
> > > > > > +       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.
> 
> Right!
> 
> ...
> 
> > > > > 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:
> 
> Thanks for the snippet. It actually shows the opposite. No, below is the user of the
> _pointer_ to the struct of that type. You may easily use the "opaque pointer" approach.
> I.o.w. my comment stays.
> 
> > struct sppctl_gpio_chip {
> >         :
> >         :
> > };
> >
> > struct sppctl_pdata {
> >         :
> >         :
> >         struct sppctl_gpio_chip *spp_gchip;
> >         :
> >         :
> > };

I see!
I'll move struct sppctl_gpio_chip {...} to c file next patch.

> --
> With Best Regards,
> Andy Shevchenko

Thanks,
Wells Lu