Messages in this thread |  | | From | Yash Shah <> | Subject | RE: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs | Date | Mon, 18 Nov 2019 10:03:52 +0000 |
| |
> -----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/
|  |