pon., 18 lis 2019 o 11:03 Yash Shah [off-list ref] napisał(a):

quoted

-----Original Message-----
From: Bartosz Golaszewski <redacted>
Sent: 13 November 2019 18:41
To: Yash Shah <redacted>
Cc: linus.walleij@linaro.org; robh+dt@kernel.org; mark.rutland@arm.com;
palmer@dabbelt.com; Paul Walmsley ( Sifive) [off-list ref];
aou@eecs.berkeley.edu; tglx@linutronix.de; jason@lakedaemon.net;
maz@kernel.org; bmeng.cn@gmail.com; atish.patra@wdc.com; Sagar Kadam
[off-list ref]; linux-gpio@vger.kernel.org;
devicetree@vger.kernel.org; linux-riscv@lists.infradead.org; linux-
kernel@vger.kernel.org; Sachin Ghadi [off-list ref]
Subject: Re: [PATCH 3/4] gpio: sifive: Add GPIO driver for SiFive SoCs

wt., 12 lis 2019 o 13:12 Yash Shah [off-list ref] napisał(a):

quoted

Adds the GPIO driver for SiFive RISC-V SoCs.

Signed-off-by: Wesley W. Terpstra <redacted>
[Atish: Various fixes and code cleanup]
Signed-off-by: Atish Patra <redacted>
Signed-off-by: Yash Shah <redacted>

[...]

quoted

quoted

+
+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.

quoted

quoted

+       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?

The thing is: the gpio-mmio code you're (correctly) reusing uses a
different lock - namely: bgpio_lock in struct gpio_chip. If you want
to use regmap for register operations, then you need to set
disable_locking in regmap_config to true and then take this lock
manually on every access.

Bart

quoted

quoted

+       if (IS_ERR(chip->regs))
+               return PTR_ERR(chip->regs);
+

[...]

quoted

quoted

+
+       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/