Re: [PATCH v3 05/11] staging: mt7621-gpio: avoid use of globals and use platform_data instead

[NeilBrown <neil@brown.name>]

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

On Wed, May 16 2018, Sergio Paracuellos wrote:

> Gpio driver have a some globals which can be avoided just
> using platform_data in a proper form. This commit adds a new
> struct mtk_data which includes all of those globals setting them
> using platform_set_drvdata and devm_gpiochip_add_data functions.
> With this properly set we are able to retrieve driver data along
> the code using kernel api's so globals are not needed anymore.
>
> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
> ---
>  drivers/staging/mt7621-gpio/gpio-mt7621.c | 85 +++++++++++++++++++++----------
>  1 file changed, 59 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/staging/mt7621-gpio/gpio-mt7621.c b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> index 7d17949..c701259 100644
> --- a/drivers/staging/mt7621-gpio/gpio-mt7621.c
> +++ b/drivers/staging/mt7621-gpio/gpio-mt7621.c
> @@ -31,17 +31,20 @@ enum mediatek_gpio_reg {
>  	GPIO_REG_EDGE,
>  };
>  
> -static void __iomem *mediatek_gpio_membase;
> -static int mediatek_gpio_irq;
> -static struct irq_domain *mediatek_gpio_irq_domain;
> -
> -static struct mtk_gc {
> +struct mtk_gc {
>  	struct gpio_chip chip;
>  	spinlock_t lock;
>  	int bank;
>  	u32 rising;
>  	u32 falling;
> -} *gc_map[MTK_MAX_BANK];
> +};
> +
> +struct mtk_data {
> +	void __iomem *gpio_membase;
> +	int gpio_irq;
> +	struct irq_domain *gpio_irq_domain;
> +	struct mtk_gc *gc_map[MTK_MAX_BANK];
> +};
>  
>  static inline struct mtk_gc
>  *to_mediatek_gpio(struct gpio_chip *chip)
> @@ -56,15 +59,19 @@ static inline struct mtk_gc
>  static inline void
>  mtk_gpio_w32(struct mtk_gc *rg, u8 reg, u32 val)
>  {
> -	iowrite32(val, mediatek_gpio_membase + (reg * 0x10) + (rg->bank * 0x4));
> +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
> +	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
> +
> +	iowrite32(val, gpio_data->gpio_membase + offset);
>  }
>  
>  static inline u32
>  mtk_gpio_r32(struct mtk_gc *rg, u8 reg)
>  {
> +	struct mtk_data *gpio_data = gpiochip_get_data(&rg->chip);
>  	u32 offset = (reg * 0x10) + (rg->bank * 0x4);
>  
> -	return ioread32(mediatek_gpio_membase + offset);
> +	return ioread32(gpio_data->gpio_membase + offset);
>  }
>  
>  static void
> @@ -137,23 +144,26 @@ mediatek_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>  static int
>  mediatek_gpio_to_irq(struct gpio_chip *chip, unsigned int pin)
>  {
> +	struct mtk_data *gpio_data = gpiochip_get_data(chip);
>  	struct mtk_gc *rg = to_mediatek_gpio(chip);
>  
> -	return irq_create_mapping(mediatek_gpio_irq_domain,
> +	return irq_create_mapping(gpio_data->gpio_irq_domain,
>  				  pin + (rg->bank * MTK_BANK_WIDTH));
>  }
>  
>  static int
>  mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  {
> +	struct mtk_data *gpio_data = dev_get_drvdata(&pdev->dev);
>  	const __be32 *id = of_get_property(bank, "reg", NULL);
>  	struct mtk_gc *rg = devm_kzalloc(&pdev->dev,
>  				sizeof(struct mtk_gc), GFP_KERNEL);
> +	int ret;
>  
>  	if (!rg || !id || be32_to_cpu(*id) > MTK_MAX_BANK)
>  		return -ENOMEM;
>  
> -	gc_map[be32_to_cpu(*id)] = rg;
> +	gpio_data->gc_map[be32_to_cpu(*id)] = rg;
>  
>  	memset(rg, 0, sizeof(struct mtk_gc));
>  
> @@ -169,10 +179,17 @@ mediatek_gpio_bank_probe(struct platform_device *pdev, struct device_node *bank)
>  	rg->chip.get_direction = mediatek_gpio_get_direction;
>  	rg->chip.get = mediatek_gpio_get;
>  	rg->chip.set = mediatek_gpio_set;
> -	if (mediatek_gpio_irq_domain)
> +	if (gpio_data->gpio_irq_domain)
>  		rg->chip.to_irq = mediatek_gpio_to_irq;
>  	rg->bank = be32_to_cpu(*id);
>  
> +	ret = devm_gpiochip_add_data(&pdev->dev, &rg->chip, gpio_data);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Could not register gpio %d, ret=%d\n",
> +			rg->chip.ngpio, ret);
> +		return ret;
> +	}
> +

Calling devm_gpiochip_add_data() here looks good.
Not removing
	return gpiochip_add(&rg->chip);
from the end of the function isn't so good :-(

BTW interrupt definitely don't work yet.  The calls
	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
get NULL.  I think some sort of irq_set_chip_data(irq,...) is needed
in mediatek_gpio_gpio_map(), but it is too late at night for it to
all make sense to me :-)

Thanks,
NeilBrown

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