[v6,0/2] pinctrl: Add RZ/A2 pin and gpio driver

The pin controller in the RZ/A2 is nothing like the pin controller in
the RZ/A1. That's a good thing! This pin controller is much more simple
and easier to configure.

So, this driver is faily simple (I hope).

Chris Brandt (2):
  pinctrl: Add RZ/A2 pin and gpio controller
  dt-bindings: pinctrl: Add RZ/A2 pinctrl and GPIO

 .../bindings/pinctrl/renesas,rza2-pinctrl.txt      |  87 ++++
 drivers/pinctrl/Kconfig                            |  11 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinctrl-rza2.c                     | 518 +++++++++++++++++++++
 include/dt-bindings/pinctrl/r7s9210-pinctrl.h      |  47 ++
 5 files changed, 664 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza2-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-rza2.c
 create mode 100644 include/dt-bindings/pinctrl/r7s9210-pinctrl.h

Hi Chris,

On Thu, Nov 15, 2018 at 09:00:44AM -0500, Chris Brandt wrote:
> Adds support for the pin and gpio controller found in R7S9210 (RZ/A2) SoCs.
>
> Signed-off-by: Chris Brandt <chris.brandt@renesas.com>
> Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> v5:
>  * Specify number of ports using of_device_id.data and save as priv->npins
>  * Use priv->npins everywhere instead of hard coded RZA2_NPINS
>  * Check gpio-ranges to make sure args matches SOC

Sorry about this, I didn't want to ask you to do this now, it might
have had post-poned to when a new SoC will have to be supported, but..

[snip]

> +
> +static const struct of_device_id rza2_pinctrl_of_match[] = {
> +	{ .compatible = "renesas,r7s9210-pinctrl", .data = (void *)22, },

... I really don't like this, I'm sorry.

I would rather make a 'struct rza_pinctrl_info' or similar which
contains all the fields you now hardcode (number of ports, pins per
port etc) and which is easily extensible in case you need to do so.

I'm sorry this is more work, and again, it might be post-poned imo,
provided you drop this change you have introduced here.

Thanks
   j


> +	{ /* sentinel */ }
> +};
> +
> +static struct platform_driver rza2_pinctrl_driver = {
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table = rza2_pinctrl_of_match,
> +	},
> +	.probe = rza2_pinctrl_probe,
> +};
> +
> +static int __init rza2_pinctrl_init(void)
> +{
> +	return platform_driver_register(&rza2_pinctrl_driver);
> +}
> +core_initcall(rza2_pinctrl_init);
> +
> +MODULE_AUTHOR("Chris Brandt <chris.brandt@renesas.com>");
> +MODULE_DESCRIPTION("Pin and gpio controller driver for RZ/A2 SoC");
> +MODULE_LICENSE("GPL v2");
> --
> 2.16.1
>

Hi Jacopo,

On Thursday, November 15, 2018 1, jacopo mondi wrote:
> > v5:
> >  * Specify number of ports using of_device_id.data and save as priv-
> >npins
> >  * Use priv->npins everywhere instead of hard coded RZA2_NPINS
> >  * Check gpio-ranges to make sure args matches SOC
> 
> Sorry about this, I didn't want to ask you to do this now, it might
> have had post-poned to when a new SoC will have to be supported, but..

As long as I can get this driver in for 4.21, I'll still be happy.


> > +static const struct of_device_id rza2_pinctrl_of_match[] = {
> > +	{ .compatible = "renesas,r7s9210-pinctrl", .data = (void *)22, },
> 
> ... I really don't like this, I'm sorry.
> 
> I would rather make a 'struct rza_pinctrl_info' or similar which
> contains all the fields you now hardcode (number of ports, pins per
> port etc) and which is easily extensible in case you need to do so.

I was going by if there is only 1 value being set, just pass in a number
(don't make a struct). That is what is being done for the R-Car/RZA 
SDHI driver (renesas_sdhi_internal_dmac.c), and what I was also asked to do
for the RZ/A watchdog timer (rza_wdt.c).

At the moment, the number of ports in the SOC is the only variable that 
would be different between SoCs. For example, "pins per port" will 
always be 8 (it's part of the HW design of this pin controller, it can never 
change).

We can have Geert give his opinion on the topic since it was his 
suggestion to begin with.


> I'm sorry this is more work, and again, it might be post-poned imo,
> provided you drop this change you have introduced here.

Since Geert is the maintainer of the Renesas pinctrl drivers, I'll let 
him decide if I should drop that part for now since only 1 SOC exists 
today.

Chris

Hi Chris,

On Thu, Nov 15, 2018 at 5:50 PM Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Thursday, November 15, 2018 1, jacopo mondi wrote:
> > > v5:
> > >  * Specify number of ports using of_device_id.data and save as priv-
> > >npins
> > >  * Use priv->npins everywhere instead of hard coded RZA2_NPINS
> > >  * Check gpio-ranges to make sure args matches SOC
> >
> > Sorry about this, I didn't want to ask you to do this now, it might
> > have had post-poned to when a new SoC will have to be supported, but..
>
> As long as I can get this driver in for 4.21, I'll still be happy

;-)

> > > +static const struct of_device_id rza2_pinctrl_of_match[] = {
> > > +   { .compatible = "renesas,r7s9210-pinctrl", .data = (void *)22, },
> >
> > ... I really don't like this, I'm sorry.
> >
> > I would rather make a 'struct rza_pinctrl_info' or similar which
> > contains all the fields you now hardcode (number of ports, pins per
> > port etc) and which is easily extensible in case you need to do so.
>
> I was going by if there is only 1 value being set, just pass in a number
> (don't make a struct). That is what is being done for the R-Car/RZA
> SDHI driver (renesas_sdhi_internal_dmac.c), and what I was also asked to do
> for the RZ/A watchdog timer (rza_wdt.c).

That's indeed what we do, typically.

> At the moment, the number of ports in the SOC is the only variable that
> would be different between SoCs. For example, "pins per port" will
> always be 8 (it's part of the HW design of this pin controller, it can never
> change).
>
> We can have Geert give his opinion on the topic since it was his
> suggestion to begin with.
>
>
> > I'm sorry this is more work, and again, it might be post-poned imo,
> > provided you drop this change you have introduced here.
>
> Since Geert is the maintainer of the Renesas pinctrl drivers, I'll let
> him decide if I should drop that part for now since only 1 SOC exists
> today.

I don't have a real preference.

Two cautions, though:
  1. You do have to remember to increase port_names[], when needed,
  2. Trying to predict the future may fail, and more driver updates may be
     needed when a new variant of this pin controller will be conceived.

Gr{oetje,eeting}s,

                        Geert

Hi Geert,

On Friday, November 16, 2018, Geert Uytterhoeven wrote:
> > We can have Geert give his opinion on the topic since it was his
> > suggestion to begin with.
> >
> >
> > > I'm sorry this is more work, and again, it might be post-poned imo,
> > > provided you drop this change you have introduced here.
> >
> > Since Geert is the maintainer of the Renesas pinctrl drivers, I'll let
> > him decide if I should drop that part for now since only 1 SOC exists
> > today.
> 
> I don't have a real preference.
> 
> Two cautions, though:
>   1. You do have to remember to increase port_names[], when needed,

Given your comment that we always explicitly add SOC support to a 
driver, even if that just means adding a compatible string, we will always 
have to edit the driver anyway.

>   2. Trying to predict the future may fail, and more driver updates may be
>      needed when a new variant of this pin controller will be conceived.

Very true.
I think changing the hard coded RZA2_NPINS into priv->npins was the 
correct thing to do. Of course the driver name is pinctrl-rza2.c which will 
be silly if used for other SoCs. But then again...I'm using lots of 
"rcar" drivers for RZ/A2!


Chris