From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from silver.osuosl.org (smtp3.osuosl.org [140.211.166.136]) by ash.osuosl.org (Postfix) with ESMTP id 68E141C0BBC for ; Sat, 19 May 2018 11:18:13 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by silver.osuosl.org (Postfix) with ESMTP id 664202D654 for ; Sat, 19 May 2018 11:18:13 +0000 (UTC) Received: from silver.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qkW0avfr1r3c for ; Sat, 19 May 2018 11:18:12 +0000 (UTC) Received: from mail-wr0-f195.google.com (mail-wr0-f195.google.com [209.85.128.195]) by silver.osuosl.org (Postfix) with ESMTPS id 16D642471E for ; Sat, 19 May 2018 11:18:12 +0000 (UTC) Received: by mail-wr0-f195.google.com with SMTP id j1-v6so630375wrm.1 for ; Sat, 19 May 2018 04:18:12 -0700 (PDT) Date: Sat, 19 May 2018 13:18:04 +0200 From: Sergio Paracuellos Subject: Re: [PATCH v3 05/11] staging: mt7621-gpio: avoid use of globals and use platform_data instead Message-ID: <20180519111804.GA15817@foobar> References: <1526461910-20650-1-git-send-email-sergio.paracuellos@gmail.com> <1526461910-20650-6-git-send-email-sergio.paracuellos@gmail.com> <87wovz2a0w.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87wovz2a0w.fsf@notabene.neil.brown.name> List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: NeilBrown Cc: gregkh@linuxfoundation.org, driverdev-devel@linuxdriverproject.org 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 > > --- > > 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@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel