linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: Yash Shah <yash.shah@sifive.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 v2 4/5] gpio: sifive: Add GPIO driver for SiFive SoCs
Date: Wed, 20 Nov 2019 11:01:11 +0100	[thread overview]
Message-ID: <CAMpxmJX0A+_6LmrWPe=Ne5KGhG9r_HySpstDhV5ZTnpz2QXF7Q@mail.gmail.com> (raw)
In-Reply-To: <1574233128-28114-5-git-send-email-yash.shah@sifive.com>

śr., 20 lis 2019 o 07:59 Yash Shah <yash.shah@sifive.com> napisał(a):
>
> Adds the GPIO driver for SiFive RISC-V SoCs.
>

This looks much better just a couple nits.

> 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>
> ---
>  drivers/gpio/Kconfig       |   9 ++
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/gpio-sifive.c | 256 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 266 insertions(+)
>  create mode 100644 drivers/gpio/gpio-sifive.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 38e096e..05e8a41 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -453,6 +453,15 @@ config GPIO_SAMA5D2_PIOBU
>           The difference from regular GPIOs is that they
>           maintain their value during backup/self-refresh.
>
> +config GPIO_SIFIVE
> +       bool "SiFive GPIO support"
> +       depends on OF_GPIO
> +       select GPIO_GENERIC
> +       select GPIOLIB_IRQCHIP
> +       select REGMAP_MMIO
> +       help
> +         Say yes here to support the GPIO device on SiFive SoCs.
> +
>  config GPIO_SIOX
>         tristate "SIOX GPIO support"
>         depends on SIOX
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index d2fd19c..bf7984e 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -121,6 +121,7 @@ obj-$(CONFIG_ARCH_SA1100)           += gpio-sa1100.o
>  obj-$(CONFIG_GPIO_SAMA5D2_PIOBU)       += gpio-sama5d2-piobu.o
>  obj-$(CONFIG_GPIO_SCH311X)             += gpio-sch311x.o
>  obj-$(CONFIG_GPIO_SCH)                 += gpio-sch.o
> +obj-$(CONFIG_GPIO_SIFIVE)              += gpio-sifive.o
>  obj-$(CONFIG_GPIO_SIOX)                        += gpio-siox.o
>  obj-$(CONFIG_GPIO_SODAVILLE)           += gpio-sodaville.o
>  obj-$(CONFIG_GPIO_SPEAR_SPICS)         += gpio-spear-spics.o
> diff --git a/drivers/gpio/gpio-sifive.c b/drivers/gpio/gpio-sifive.c
> new file mode 100644
> index 0000000..02666ae
> --- /dev/null
> +++ b/drivers/gpio/gpio-sifive.c
> @@ -0,0 +1,256 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 SiFive
> + */

I prefer to have a newline between the copyright notice and the headers.

> +#include <linux/bitops.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/of_irq.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/init.h>
> +#include <linux/of.h>

Is this really needed? I only see functions defined in of_irq.h.

> +#include <linux/platform_device.h>
> +#include <linux/pm.h>

Same here - I don't see any functions from this header being called.

> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/regmap.h>
> +
> +#define GPIO_INPUT_VAL 0x00
> +#define GPIO_INPUT_EN  0x04
> +#define GPIO_OUTPUT_EN 0x08
> +#define GPIO_OUTPUT_VAL        0x0C
> +#define GPIO_RISE_IE   0x18
> +#define GPIO_RISE_IP   0x1C
> +#define GPIO_FALL_IE   0x20
> +#define GPIO_FALL_IP   0x24
> +#define GPIO_HIGH_IE   0x28
> +#define GPIO_HIGH_IP   0x2C
> +#define GPIO_LOW_IE    0x30
> +#define GPIO_LOW_IP    0x34
> +#define GPIO_OUTPUT_XOR        0x40
> +
> +#define MAX_GPIO               32
> +#define SIFIVE_GPIO_IRQ_OFFSET 7

Please use a single prefix for all symbols in this driver. Let's stick
to sifive_gpio and SIFIVE_GPIO for defines.

> +
> +struct sifive_gpio {
> +       void __iomem            *base;
> +       struct gpio_chip        gc;
> +       struct regmap           *regs;
> +       u32                     enabled;

The name of this field is a bit confusing - do you mind renaming it to
something like irq_state? Maybe something else, but simply using
'enabled' makes me think this has more to do with the chip being
enabled.

> +       unsigned int            trigger[MAX_GPIO];
> +       unsigned int            irq_parent[MAX_GPIO];
> +};
> +
> +static void sifive_set_ie(struct sifive_gpio *chip, unsigned int offset)
> +{
> +       unsigned long flags;
> +       unsigned int trigger;
> +
> +       spin_lock_irqsave(&chip->gc.bgpio_lock, flags);
> +       trigger = (chip->enabled & BIT(offset)) ? chip->trigger[offset] : 0;
> +       regmap_update_bits(chip->regs, GPIO_RISE_IE, BIT(offset),
> +                          (trigger & IRQ_TYPE_EDGE_RISING) ? BIT(offset) : 0);
> +       regmap_update_bits(chip->regs, GPIO_FALL_IE, BIT(offset),
> +                          (trigger & IRQ_TYPE_EDGE_FALLING) ? BIT(offset) : 0);
> +       regmap_update_bits(chip->regs, GPIO_HIGH_IE, BIT(offset),
> +                          (trigger & IRQ_TYPE_LEVEL_HIGH) ? BIT(offset) : 0);
> +       regmap_update_bits(chip->regs, GPIO_LOW_IE, BIT(offset),
> +                          (trigger & IRQ_TYPE_LEVEL_LOW) ? BIT(offset) : 0);
> +       spin_unlock_irqrestore(&chip->gc.bgpio_lock, flags);
> +}
> +
> +static int sifive_irq_set_type(struct irq_data *d, unsigned int trigger)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct sifive_gpio *chip = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(d);
> +
> +       if (offset < 0 || offset >= gc->ngpio)
> +               return -EINVAL;
> +
> +       chip->trigger[offset] = trigger;
> +       sifive_set_ie(chip, offset);
> +       return 0;
> +}
> +
> +static void sifive_irq_enable(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct sifive_gpio *chip = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(d) % MAX_GPIO;
> +       u32 bit = BIT(offset);
> +       unsigned long flags;
> +
> +       irq_chip_enable_parent(d);
> +
> +       /* Switch to input */
> +       gc->direction_input(gc, offset);
> +
> +       spin_lock_irqsave(&gc->bgpio_lock, flags);
> +       /* Clear any sticky pending interrupts */
> +       regmap_write(chip->regs, GPIO_RISE_IP, bit);
> +       regmap_write(chip->regs, GPIO_FALL_IP, bit);
> +       regmap_write(chip->regs, GPIO_HIGH_IP, bit);
> +       regmap_write(chip->regs, GPIO_LOW_IP, bit);
> +       spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> +
> +       /* Enable interrupts */
> +       assign_bit(offset, (unsigned long *)&chip->enabled, 1);
> +       sifive_set_ie(chip, offset);
> +}
> +
> +static void sifive_irq_disable(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct sifive_gpio *chip = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(d) % MAX_GPIO;
> +
> +       assign_bit(offset, (unsigned long *)&chip->enabled, 0);
> +       sifive_set_ie(chip, offset);
> +       irq_chip_disable_parent(d);
> +}
> +
> +static void sifive_irq_eoi(struct irq_data *d)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct sifive_gpio *chip = gpiochip_get_data(gc);
> +       int offset = irqd_to_hwirq(d) % MAX_GPIO;
> +       u32 bit = BIT(offset);
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&gc->bgpio_lock, flags);
> +       /* Clear all pending interrupts */
> +       regmap_write(chip->regs, GPIO_RISE_IP, bit);
> +       regmap_write(chip->regs, GPIO_FALL_IP, bit);
> +       regmap_write(chip->regs, GPIO_HIGH_IP, bit);
> +       regmap_write(chip->regs, GPIO_LOW_IP, bit);
> +       spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> +
> +       irq_chip_eoi_parent(d);
> +}
> +
> +static struct irq_chip sifive_irqchip = {
> +       .name           = "sifive-gpio",
> +       .irq_set_type   = sifive_irq_set_type,
> +       .irq_mask       = irq_chip_mask_parent,
> +       .irq_unmask     = irq_chip_unmask_parent,
> +       .irq_enable     = sifive_irq_enable,
> +       .irq_disable    = sifive_irq_disable,
> +       .irq_eoi        = sifive_irq_eoi,
> +};
> +
> +static int sifive_gpio_child_to_parent_hwirq(struct gpio_chip *gc,
> +                                            unsigned int child,
> +                                            unsigned int child_type,
> +                                            unsigned int *parent,
> +                                            unsigned int *parent_type)
> +{
> +       *parent_type = IRQ_TYPE_NONE;
> +       *parent = child + SIFIVE_GPIO_IRQ_OFFSET;
> +       return 0;
> +}
> +
> +static const struct regmap_config sifive_gpio_regmap_config = {
> +       .reg_bits = 32,
> +       .reg_stride = 4,
> +       .val_bits = 32,
> +       .fast_io = true,
> +       .disable_locking = true,
> +};
> +
> +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;
> +       unsigned long flags;
> +       int ret, ngpio;
> +
> +       chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> +       if (!chip)
> +               return -ENOMEM;
> +
> +       chip->base = devm_platform_ioremap_resource(pdev, 0);
> +       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);
> +       if (IS_ERR(chip->regs))
> +               return PTR_ERR(chip->regs);
> +
> +       ngpio = of_irq_count(node);
> +       if (ngpio >= MAX_GPIO) {
> +               dev_err(dev, "Too many GPIO interrupts (max=%d)\n", MAX_GPIO);
> +               return -ENXIO;
> +       }
> +
> +       irq_parent = of_irq_find_parent(node);
> +       if (!irq_parent) {
> +               dev_err(dev, "no IRQ parent node\n");
> +               return -ENODEV;
> +       }
> +       parent = irq_find_host(irq_parent);
> +       if (!parent) {
> +               dev_err(dev, "no IRQ parent domain\n");
> +               return -ENODEV;
> +       }
> +
> +       ret = bgpio_init(&chip->gc, dev, 4,
> +                        chip->base + GPIO_INPUT_VAL,
> +                        chip->base + GPIO_OUTPUT_VAL,
> +                        NULL,
> +                        chip->base + GPIO_OUTPUT_EN,
> +                        chip->base + GPIO_INPUT_EN,
> +                        0);
> +       if (ret) {
> +               dev_err(dev, "unable to init generic GPIO\n");
> +               return ret;
> +       }
> +
> +       spin_lock_irqsave(&chip->gc.bgpio_lock, flags);
> +       /* Disable all GPIO interrupts before enabling parent interrupts */
> +       regmap_write(chip->regs, GPIO_RISE_IE, 0);
> +       regmap_write(chip->regs, GPIO_FALL_IE, 0);
> +       regmap_write(chip->regs, GPIO_HIGH_IE, 0);
> +       regmap_write(chip->regs, GPIO_LOW_IE, 0);
> +       spin_unlock_irqrestore(&chip->gc.bgpio_lock, flags);

No need for locking in probe().

> +       chip->enabled = 0;
> +
> +       chip->gc.base = -1;
> +       chip->gc.ngpio = ngpio;
> +       chip->gc.label = dev_name(dev);
> +       chip->gc.parent = dev;
> +       chip->gc.owner = THIS_MODULE;
> +       girq = &chip->gc.irq;
> +       girq->chip = &sifive_irqchip;
> +       girq->fwnode = of_node_to_fwnode(node);
> +       girq->parent_domain = parent;
> +       girq->child_to_parent_hwirq = sifive_gpio_child_to_parent_hwirq;
> +       girq->handler = handle_bad_irq;
> +       girq->default_type = IRQ_TYPE_NONE;
> +
> +       platform_set_drvdata(pdev, chip);
> +       return gpiochip_add_data(&chip->gc, chip);
> +}
> +
> +static const struct of_device_id sifive_gpio_match[] = {
> +       { .compatible = "sifive,gpio0" },
> +       { .compatible = "sifive,fu540-c000-gpio" },
> +       { },
> +};
> +
> +static struct platform_driver sifive_gpio_driver = {
> +       .probe          = sifive_gpio_probe,
> +       .driver = {
> +               .name   = "sifive_gpio",
> +               .of_match_table = of_match_ptr(sifive_gpio_match),
> +       },
> +};
> +builtin_platform_driver(sifive_gpio_driver)
> --
> 2.7.4
>

Bartosz

  reply	other threads:[~2019-11-20 10:01 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-20  6:59 [PATCH v2 0/5] GPIO & Hierarchy IRQ support for HiFive Unleashed Yash Shah
2019-11-20  6:59 ` [PATCH v2 1/5] genirq: introduce irq_domain_translate_onecell Yash Shah
2019-11-20  9:34   ` Thomas Gleixner
2019-11-20 10:24     ` Marc Zyngier
2019-11-20 10:48       ` Thomas Gleixner
2019-11-20 10:38   ` Marc Zyngier
2019-11-21  8:35     ` Yash Shah
2019-11-21  8:55       ` Yash Shah
2019-11-21  9:20         ` Marc Zyngier
2019-11-20  6:59 ` [PATCH v2 2/5] irqchip: sifive: Support hierarchy irq domain Yash Shah
2019-11-22 10:17   ` Marc Zyngier
2019-11-20  6:59 ` [PATCH v2 3/5] gpio: sifive: Add DT documentation for SiFive GPIO Yash Shah
2019-11-20  6:59 ` [PATCH v2 4/5] gpio: sifive: Add GPIO driver for SiFive SoCs Yash Shah
2019-11-20 10:01   ` Bartosz Golaszewski [this message]
2019-11-21  8:32     ` Yash Shah
2019-11-20  6:59 ` [PATCH v2 5/5] riscv: dts: Add DT support for SiFive FU540 GPIO driver Yash Shah
2019-11-20  9:14   ` Andreas Schwab
2019-11-21  8:26     ` 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='CAMpxmJX0A+_6LmrWPe=Ne5KGhG9r_HySpstDhV5ZTnpz2QXF7Q@mail.gmail.com' \
    --to=bgolaszewski@baylibre.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=atish.patra@wdc.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 \
    --cc=yash.shah@sifive.com \
    --subject='Re: [PATCH v2 4/5] gpio: sifive: Add GPIO driver for SiFive SoCs' \
    /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

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