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

NeilBrown neil at brown.name
Sat May 19 22:44:18 UTC 2018


On Sat, May 19 2018, Sergio Paracuellos wrote:

> 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.
>

I think you were a bit hasty here.
Yes, irq_set_chip_data() needs to be called there, but that isn't a
complete fix.
Rather than explain exactly the problem, I rather give you the
opportunity to look through the code and work out exactly what
is happening, and then see why it is wrong.
Interrupts are very close to working.  My patch to make them work is:
$ git diff --stat
 drivers/staging/mt7621-gpio/gpio-mt7621.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

Let me know if you cannot find the problem.

BTW I don't know what Greg prefers, but my preference for this sort of
change is to just resend the problematic patch, as a reply to the
original.  It is easier to be sure that I didn't miss anything that way.
Maybe once you get Reviewed-by for all the patches you can resend
the complete series.
Up to you and Greg though.

Thanks,
NeilBrown
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://driverdev.linuxdriverproject.org/pipermail/driverdev-devel/attachments/20180520/82554a5d/attachment.asc>


More information about the devel mailing list