Re: [RFC PATCH 2/4] pinctrl: bcm2835: Refactor platform data

From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
To: Stefan Wahren <stefan.wahren@i2se.com>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>
Cc: linux-gpio@vger.kernel.org,
	bcm-kernel-feedback-list@broadcom.com,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org
Subject: Re: [RFC PATCH 2/4] pinctrl: bcm2835: Refactor platform data
Date: Wed, 29 Jan 2020 14:52:03 +0100	[thread overview]
Message-ID: <b4c4b18eabe845e4cc3e0e1d5af2f3aa7b9091c7.camel@suse.de> (raw)
In-Reply-To: <1580148908-4863-3-git-send-email-stefan.wahren@i2se.com>

[-- Attachment #1.1: Type: text/plain, Size: 2201 bytes --]

Hi Stefan,
thanks for the series!

On Mon, 2020-01-27 at 19:15 +0100, Stefan Wahren wrote:
> This prepares the platform data to be easier to extend for more GPIOs.
> Except of this there is no functional change.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---

[...]

> @@ -1083,6 +1112,7 @@ static int bcm2835_pinctrl_probe(struct platform_device
> *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	struct bcm2835_pinctrl *pc;
> +	struct bcm_plat_data *pdata;

You could define this as const...

>  	struct gpio_irq_chip *girq;
>  	struct resource iomem;
>  	int err, i;
> @@ -1108,7 +1138,13 @@ static int bcm2835_pinctrl_probe(struct platform_device
> *pdev)
>  	if (IS_ERR(pc->base))
>  		return PTR_ERR(pc->base);
>  
> -	pc->gpio_chip = bcm2835_gpio_chip;
> +	match = of_match_node(bcm2835_pinctrl_match, pdev->dev.of_node);
> +	if (!match)
> +		return -EINVAL;
> +
> +	pdata = (struct bcm_plat_data *)match->data;

...and drop this cast.

> +
> +	memcpy(&pc->gpio_chip, pdata->gpio_chip, sizeof(pc->gpio_chip));
>  	pc->gpio_chip.parent = dev;
>  	pc->gpio_chip.of_node = np;
>  
> @@ -1159,19 +1195,14 @@ static int bcm2835_pinctrl_probe(struct
> platform_device *pdev)
>  		return err;
>  	}
>  
> -	match = of_match_node(bcm2835_pinctrl_match, pdev->dev.of_node);
> -	if (match) {
> -		bcm2835_pinctrl_desc.confops =
> -			(const struct pinconf_ops *)match->data;
> -	}
> -
> -	pc->pctl_dev = devm_pinctrl_register(dev, &bcm2835_pinctrl_desc, pc);
> +	memcpy(&pc->pctl_desc, pdata->pctl_desc, sizeof(pc->pctl_desc));

I suggest doing (here and below):

	pc->pctl_desc = *pdata->pctl_desc;

> +	pc->pctl_dev = devm_pinctrl_register(dev, &pc->pctl_desc, pc);
>  	if (IS_ERR(pc->pctl_dev)) {
>  		gpiochip_remove(&pc->gpio_chip);
>  		return PTR_ERR(pc->pctl_dev);
>  	}
>  
> -	pc->gpio_range = bcm2835_pinctrl_gpio_range;
> +	memcpy(&pc->gpio_range, pdata->gpio_range, sizeof(pc->gpio_range));
>  	pc->gpio_range.base = pc->gpio_chip.base;
>  	pc->gpio_range.gc = &pc->gpio_chip;
>  	pinctrl_add_gpio_range(pc->pctl_dev, &pc->gpio_range);

Regards,
Nicolas

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel