From: Yash Shah <yash.shah@sifive.com>
To: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
"robh+dt@kernel.org" <robh+dt@kernel.org>,
"mark.rutland@arm.com" <mark.rutland@arm.com>,
"palmer@dabbelt.com" <palmer@dabbelt.com>,
"Paul Walmsley ( Sifive)" <paul.walmsley@sifive.com>,
"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
"tglx@linutronix.de" <tglx@linutronix.de>,
"jason@lakedaemon.net" <jason@lakedaemon.net>,
"maz@kernel.org" <maz@kernel.org>,
"bmeng.cn@gmail.com" <bmeng.cn@gmail.com>,
"atish.patra@wdc.com" <atish.patra@wdc.com>,
Sagar Kadam <sagar.kadam@sifive.com>,
"linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-riscv@lists.infradead.org"
<linux-riscv@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Sachin Ghadi <sachin.ghadi@sifive.com>
Subject: RE: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
Date: Mon, 18 Nov 2019 10:03:52 +0000 [thread overview]
Message-ID: <CH2PR13MB33680443C101511E66ECADF08C4D0@CH2PR13MB3368.namprd13.prod.outlook.com> (raw)
In-Reply-To: <CAMpxmJWcuV7goPWxOWv_Og9GwzGrioF62SfS1LCiHf9eDX=vdw@mail.gmail.com>
> -----Original Message-----
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Sent: 13 November 2019 18:41
> To: Yash Shah <yash.shah@sifive.com>
> Cc: linus.walleij@linaro.org; robh+dt@kernel.org; mark.rutland@arm.com;
> palmer@dabbelt.com; Paul Walmsley ( Sifive) <paul.walmsley@sifive.com>;
> aou@eecs.berkeley.edu; tglx@linutronix.de; jason@lakedaemon.net;
> maz@kernel.org; bmeng.cn@gmail.com; atish.patra@wdc.com; Sagar Kadam
> <sagar.kadam@sifive.com>; linux-gpio@vger.kernel.org;
> devicetree@vger.kernel.org; linux-riscv@lists.infradead.org; linux-
> kernel@vger.kernel.org; Sachin Ghadi <sachin.ghadi@sifive.com>
> Subject: Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs
>
> wt., 12 lis 2019 o 13:12 Yash Shah <yash.shah@sifive.com> napisał(a):
> >
> > Adds the GPIO driver for SiFive RISC-V SoCs.
> >
> > Signed-off-by: Wesley W. Terpstra <wesley@sifive.com>
> > [Atish: Various fixes and code cleanup]
> > Signed-off-by: Atish Patra <atish.patra@wdc.com>
> > Signed-off-by: Yash Shah <yash.shah@sifive.com>
[...]
> > +
> > +static int sifive_gpio_probe(struct platform_device *pdev) {
> > + struct device *dev = &pdev->dev;
> > + struct device_node *node = pdev->dev.of_node;
> > + struct device_node *irq_parent;
> > + struct irq_domain *parent;
> > + struct gpio_irq_chip *girq;
> > + struct sifive_gpio *chip;
> > + struct resource *res;
> > + int ret, ngpio;
> > +
> > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> > + if (!chip)
> > + return -ENOMEM;
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + chip->base = devm_ioremap_resource(dev, res);
>
> Use devm_platform_ioremap_resource() and drop the res variable.
>
Sure, will do that.
> > + if (IS_ERR(chip->base)) {
> > + dev_err(dev, "failed to allocate device memory\n");
> > + return PTR_ERR(chip->base);
> > + }
> > +
> > + chip->regs = devm_regmap_init_mmio(dev, chip->base,
> > +
> > + &sifive_gpio_regmap_config);
>
> Why do you need this regmap here? You initialize a new regmap, then use
> your own locking despite not having disabled the internal locking in regmap,
> and then you initialize the mmio generic GPIO code which will use yet
> another lock to operate on the same registers and in the end you write to
> those registers without taking any lock anyway.
> Doesn't make much sense to me.
>
As suggested in the comments received on the RFC version of this patch[0], I am trying to use regmap MMIO by looking at gpio-mvebu.c. I got your point regarding the usage of own locks is not making any sense.
Here is what I will do in v2:
1. drop the usage of own locks
2. consistently use regmap_* apis for register access (replace all iowrites).
Does this make sense now?
> > + if (IS_ERR(chip->regs))
> > + return PTR_ERR(chip->regs);
> > +
[...]
> > +
> > + ret = gpiochip_add_data(&chip->gc, chip);
> > + if (ret)
> > + return ret;
> > +
> > + platform_set_drvdata(pdev, chip);
> > + dev_info(dev, "SiFive GPIO chip registered %d GPIOs\n",
> > + ngpio);
>
> Core gpio library emits a very similar debug message from
> gpiochip_setup_dev(), I think you can drop it and directly return
> gpiochip_add_data().
>
> Bartosz
Ok. Will directly return gpiochip_add_data().
Thanks for your comments!
- Yash
[0] https://lore.kernel.org/linux-riscv/20181010123519.RVexDppaPFpIWl7QU_hpP8tc5qqWPJgeuLYn0FaGbeQ@z/
next prev parent reply other threads:[~2019-11-18 10:04 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-12 12:11 [PATCH 0/4] GPIO & Hierarchy IRQ support for HiFive Unleashed Yash Shah
2019-11-12 12:11 ` [PATCH 1/4] irqchip: sifive: Support hierarchy irq domain Yash Shah
2019-11-12 12:43 ` Marc Zyngier
2019-11-18 7:14 ` Yash Shah
2019-11-12 12:12 ` [PATCH 2/4] gpio: sifive: Add DT documentation for SiFive GPIO Yash Shah
2019-11-18 16:53 ` Rob Herring
2019-11-12 12:12 ` [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs Yash Shah
2019-11-12 12:58 ` Marc Zyngier
2019-11-18 7:50 ` Yash Shah
2019-11-13 13:10 ` Bartosz Golaszewski
2019-11-18 10:03 ` Yash Shah [this message]
2019-11-18 10:15 ` Bartosz Golaszewski
2019-11-19 15:02 ` Linus Walleij
2019-11-19 16:41 ` Bartosz Golaszewski
2019-11-22 12:28 ` Linus Walleij
2019-11-22 12:39 ` Bartosz Golaszewski
2019-11-25 4:54 ` Yash Shah
2019-11-12 12:12 ` [PATCH 4/4] riscv: dts: Add DT support for SiFive FU540 GPIO driver Yash Shah
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CH2PR13MB33680443C101511E66ECADF08C4D0@CH2PR13MB3368.namprd13.prod.outlook.com \
--to=yash.shah@sifive.com \
--cc=aou@eecs.berkeley.edu \
--cc=atish.patra@wdc.com \
--cc=bgolaszewski@baylibre.com \
--cc=bmeng.cn@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jason@lakedaemon.net \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=maz@kernel.org \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
--cc=robh+dt@kernel.org \
--cc=sachin.ghadi@sifive.com \
--cc=sagar.kadam@sifive.com \
--cc=tglx@linutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).