RE: [PATCH v4 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[Chris Brandt <Chris.Brandt@xxxxxxxxxxx>]

Hi Jacopo,

Thank you for your review....again ;)

On Tuesday, November 13, 2018, jacopo mondi wrote:
> I would prefer the reverse xmas order too, but I'm not saying out loud
> as I understand is something hard to enforce, as it's a very minor
> issue :)

The next version will be very festive!

        #
        #
  .#%###%##%##.
   .##%###%##.
    .%##%###.
     .#%##%.
      .###.
       .#.
        t

Christmas in Australia


> > +	if (!of_property_read_bool(np, "gpio-controller")) {
> > +		dev_info(priv->dev, "No gpio controller registered\n");
> > +		return 0;
> > +	}
> 
> The bindings say this is mandatory... What do you think, would that
> make sense to have no gpio-controller support for this driver (testing
> apart :)

Good point. I don't need to check for this.
If it doesn't work..."RTFM"


> > +	gpio_range.id = of_args.args[0];
> 
> Do you think of_args need to be validated?
> As I understand them, id and pin_base are fixed at 0, while the pin
> number depends on the soc version/revision.
> 
> Now I wonder if you would need a gpio-ranges property at all, but
> that's fine, it doesn't hurt...

My idea was that if another RZ/A2 with a different package size is 
created, or even a RZ/A3 with the same pin controller, nothing in the driver 
will have to change because the gpio-ranges is passed in and will tell 
you how many ports you have.

As for validating the values, the only thing I can really check is that:
  of_args.args[2] == RZA2_NPINS

Of course, now that I say that, I realize that if/when it does come time
to expand this driver beyond the 1 SOC that exists today, I will have 
to stop using that hard coded RZA2_NPINS value...but I'll deal with that 
when the time comes.


> > +	ret = rza2_pinctrl_register(priv);
> > +	if (ret)
> > +		return ret;
> > +
> > +	pr_info("RZ/A2 pin controller registered\n");
> nit: dev_info

Actually, I changed it to this to anticipate the day when more than one
SOC is supported:

	dev_info(&pdev->dev, "Registered ports P0 - P%c\n",
		 port_names[priv->desc.npins / RZA2_PINS_PER_PORT - 1]);


> With this minor fixed, please add my
> Reviewed-by: Jacopo Mondi <jacopo+renesas@xxxxxxxxxx>

Thank you.

Chris