Re: [PATCH v6 1/2] pinctrl: Add RZ/A2 pin and gpio controller

[jacopo mondi <jacopo@jmondi.org>]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

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
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]