Re: [PATCH v5 5/6] pinctrl: amd: Add amd_get_iomux_res function

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, Jun 01, 2022 at 05:44:37PM +0530, Basavaraj Natikar wrote:
> Presently there is no way to change pinmux configuration run time.
> Hence add a function to get IOMUX resource which can be used to
> configure IOMUX GPIO pins run time.

With below comment addressed,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>

> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@xxxxxxx>
> ---
>  drivers/pinctrl/pinctrl-amd.c | 20 ++++++++++++++++++++
>  drivers/pinctrl/pinctrl-amd.h |  1 +
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
> index 0645c2c24f50..6bd85660287d 100644
> --- a/drivers/pinctrl/pinctrl-amd.c
> +++ b/drivers/pinctrl/pinctrl-amd.c
> @@ -963,6 +963,25 @@ static struct pinctrl_desc amd_pinctrl_desc = {
>  	.owner = THIS_MODULE,
>  };
>  
> +static void amd_get_iomux_res(struct amd_gpio *gpio_dev)
> +{
> +	struct pinctrl_desc *desc = &amd_pinctrl_desc;
> +	struct device *dev = &gpio_dev->pdev->dev;
> +	int index;
> +
> +	index = device_property_match_string(dev, "pinctrl-resource-names",  "iomux");
> +	if (index >= 0) {
> +		gpio_dev->iomux_base = devm_platform_ioremap_resource(gpio_dev->pdev, index);
> +		if (IS_ERR(gpio_dev->iomux_base)) {
> +			desc->pmxops = NULL;
> +			dev_warn(dev, "Failed to get iomux %d io resource\n", index);
> +		}
> +	} else {
> +		desc->pmxops = NULL;
> +		dev_warn(dev, "failed to get iomux index\n");
> +	}

So, now it's much cleaner, but we may do even better (no nested conditionals):

	index = device_property_match_string(dev, "pinctrl-resource-names",  "iomux");
	if (index < 0) {
		desc->pmxops = NULL;
		dev_warn(dev, "failed to get iomux index\n");
		return;
	}

	gpio_dev->iomux_base = devm_platform_ioremap_resource(gpio_dev->pdev, index);
	if (IS_ERR(gpio_dev->iomux_base)) {
		desc->pmxops = NULL;
		dev_warn(dev, "Failed to get iomux %d io resource\n", index);
	}

Alternatively (error path consolidated):

	index = device_property_match_string(dev, "pinctrl-resource-names",  "iomux");
	if (index < 0) {
		dev_warn(dev, "failed to get iomux index\n");
		goto out_no_pinmux;
	}

	gpio_dev->iomux_base = devm_platform_ioremap_resource(gpio_dev->pdev, index);
	if (IS_ERR(gpio_dev->iomux_base)) {
		dev_warn(dev, "Failed to get iomux %d io resource\n", index);
		goto out_no_pinmux;
	}

	return;

out_no_pinmux:
	desc->pmxops = NULL;

(Personally I prefer the latter, but okay with the former)

> +}
> +
>  static int amd_gpio_probe(struct platform_device *pdev)
>  {
>  	int ret = 0;
> @@ -1020,6 +1039,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
>  	gpio_dev->ngroups = ARRAY_SIZE(kerncz_groups);
>  
>  	amd_pinctrl_desc.name = dev_name(&pdev->dev);
> +	amd_get_iomux_res(gpio_dev);
>  	gpio_dev->pctrl = devm_pinctrl_register(&pdev->dev, &amd_pinctrl_desc,
>  						gpio_dev);
>  	if (IS_ERR(gpio_dev->pctrl)) {
> diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
> index e2523738fe51..76538792ac78 100644
> --- a/drivers/pinctrl/pinctrl-amd.h
> +++ b/drivers/pinctrl/pinctrl-amd.h
> @@ -83,6 +83,7 @@ struct amd_function {
>  struct amd_gpio {
>  	raw_spinlock_t          lock;
>  	void __iomem            *base;
> +	void __iomem            *iomux_base;
>  
>  	const struct pingroup *groups;
>  	u32 ngroups;
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko