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

Sergio Paracuellos sergio.paracuellos at gmail.com
Sat May 19 11:18:04 UTC 2018


On Sat, May 19, 2018 at 08:51:43PM +1000, NeilBrown wrote:
> 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 at 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 :-(

True, thanks for pointing this out.

> 
> 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 :-)

You are right. It seems irq_set_chip_data must be called in mediatek_gpio_gpio_map
function.

> 
> Thanks,
> NeilBrown

Sent v4 with this two changes.

Hope this helps.

Best regards,
    Sergio Paracuellos




More information about the devel mailing list