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.
>
> Thanks,
> NeilBrown
Sent v4 with this two changes.
Hope this helps.
Best regards,
Sergio Paracuellos
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
