On Sun, May 20, 2018 at 08:44:18AM +1000, NeilBrown wrote:
> 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@xxxxxxxxx>
> >> > ---
> >> > 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.
>
You are totally right. Retrieved pointers in irq related functions
weren't the correct ones. Also gpio_data wasn't being passed into
irq_domain_add_linear where NULL was being passed instead. Hopefully
the next one is the good one :-).
> 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.
I think is better to send v5 of whole series but before that I will send
only a PATCH made against v4 of PATCH 5 to your better review.
>
> Thanks,
> NeilBrown
Thanks,
Sergio Paracuellos
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
