From: Doug Berger <opendmb@gmail.com>
To: Bartosz Golaszewski <brgl@bgdev.pl>,
Linus Walleij <linus.walleij@linaro.org>,
Keguang Zhang <keguang.zhang@gmail.com>,
Alban Bedel <albeu@free.fr>,
Florian Fainelli <florian.fainelli@broadcom.com>,
Broadcom internal kernel review list
<bcm-kernel-feedback-list@broadcom.com>,
Matthias Brugger <matthias.bgg@gmail.com>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com>,
Paul Walmsley <paul.walmsley@sifive.com>,
Samuel Holland <samuel.holland@sifive.com>,
Yixun Lan <dlan@gentoo.org>, Andy Shevchenko <andy@kernel.org>
Cc: linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-mips@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org,
linux-riscv@lists.infradead.org, spacemit@lists.linux.dev,
Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [PATCH v2 07/15] gpio: brcmstb: use new generic GPIO chip API
Date: Wed, 10 Sep 2025 17:11:26 -0700 [thread overview]
Message-ID: <e0941449-7a62-4bbb-8790-616f393f2cc8@gmail.com> (raw)
In-Reply-To: <20250910-gpio-mmio-gpio-conv-part4-v2-7-f3d1a4c57124@linaro.org>
On 9/10/2025 12:12 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Convert the driver to using the new generic GPIO chip interfaces from
> linux/gpio/generic.h.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
> drivers/gpio/gpio-brcmstb.c | 112 ++++++++++++++++++++++++--------------------
> 1 file changed, 60 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c
> index e29a9589b3ccbd17d10f6671088dca3e76537927..be3ff916e134a674d3e1d334a7d431b7ad767a33 100644
> --- a/drivers/gpio/gpio-brcmstb.c
> +++ b/drivers/gpio/gpio-brcmstb.c
> @@ -3,6 +3,7 @@
>
> #include <linux/bitops.h>
> #include <linux/gpio/driver.h>
> +#include <linux/gpio/generic.h>
> #include <linux/of.h>
> #include <linux/module.h>
> #include <linux/irqdomain.h>
> @@ -37,7 +38,7 @@ enum gio_reg_index {
> struct brcmstb_gpio_bank {
> struct list_head node;
> int id;
> - struct gpio_chip gc;
> + struct gpio_generic_chip chip;
> struct brcmstb_gpio_priv *parent_priv;
> u32 width;
> u32 wake_active;
> @@ -72,19 +73,18 @@ __brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
> {
> void __iomem *reg_base = bank->parent_priv->reg_base;
>
> - return bank->gc.read_reg(reg_base + GIO_STAT(bank->id)) &
> - bank->gc.read_reg(reg_base + GIO_MASK(bank->id));
> + return gpio_generic_read_reg(&bank->chip, reg_base + GIO_STAT(bank->id)) &
> + gpio_generic_read_reg(&bank->chip, reg_base + GIO_MASK(bank->id));
> }
>
> static unsigned long
> brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
> {
> unsigned long status;
> - unsigned long flags;
>
> - raw_spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
> + guard(gpio_generic_lock_irqsave)(&bank->chip);
> +
> status = __brcmstb_gpio_get_active_irqs(bank);
> - raw_spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
>
> return status;
> }
> @@ -92,26 +92,26 @@ brcmstb_gpio_get_active_irqs(struct brcmstb_gpio_bank *bank)
> static int brcmstb_gpio_hwirq_to_offset(irq_hw_number_t hwirq,
> struct brcmstb_gpio_bank *bank)
> {
> - return hwirq - bank->gc.offset;
> + return hwirq - bank->chip.gc.offset;
> }
>
> static void brcmstb_gpio_set_imask(struct brcmstb_gpio_bank *bank,
> unsigned int hwirq, bool enable)
> {
> - struct gpio_chip *gc = &bank->gc;
> struct brcmstb_gpio_priv *priv = bank->parent_priv;
> u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(hwirq, bank));
> u32 imask;
> - unsigned long flags;
>
> - raw_spin_lock_irqsave(&gc->bgpio_lock, flags);
> - imask = gc->read_reg(priv->reg_base + GIO_MASK(bank->id));
> + guard(gpio_generic_lock_irqsave)(&bank->chip);
> +
> + imask = gpio_generic_read_reg(&bank->chip,
> + priv->reg_base + GIO_MASK(bank->id));
> if (enable)
> imask |= mask;
> else
> imask &= ~mask;
> - gc->write_reg(priv->reg_base + GIO_MASK(bank->id), imask);
> - raw_spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> + gpio_generic_write_reg(&bank->chip,
> + priv->reg_base + GIO_MASK(bank->id), imask);
> }
>
> static int brcmstb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
> @@ -150,7 +150,8 @@ static void brcmstb_gpio_irq_ack(struct irq_data *d)
> struct brcmstb_gpio_priv *priv = bank->parent_priv;
> u32 mask = BIT(brcmstb_gpio_hwirq_to_offset(d->hwirq, bank));
>
> - gc->write_reg(priv->reg_base + GIO_STAT(bank->id), mask);
> + gpio_generic_write_reg(&bank->chip,
> + priv->reg_base + GIO_STAT(bank->id), mask);
> }
>
> static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> @@ -162,7 +163,6 @@ static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> u32 edge_insensitive, iedge_insensitive;
> u32 edge_config, iedge_config;
> u32 level, ilevel;
> - unsigned long flags;
>
> switch (type) {
> case IRQ_TYPE_LEVEL_LOW:
> @@ -194,23 +194,25 @@ static int brcmstb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> return -EINVAL;
> }
>
> - raw_spin_lock_irqsave(&bank->gc.bgpio_lock, flags);
> + guard(gpio_generic_lock_irqsave)(&bank->chip);
>
> - iedge_config = bank->gc.read_reg(priv->reg_base +
> - GIO_EC(bank->id)) & ~mask;
> - iedge_insensitive = bank->gc.read_reg(priv->reg_base +
> - GIO_EI(bank->id)) & ~mask;
> - ilevel = bank->gc.read_reg(priv->reg_base +
> - GIO_LEVEL(bank->id)) & ~mask;
> + iedge_config = gpio_generic_read_reg(&bank->chip,
> + priv->reg_base + GIO_EC(bank->id)) & ~mask;
> + iedge_insensitive = gpio_generic_read_reg(&bank->chip,
> + priv->reg_base + GIO_EI(bank->id)) & ~mask;
> + ilevel = gpio_generic_read_reg(&bank->chip,
> + priv->reg_base + GIO_LEVEL(bank->id)) & ~mask;
>
> - bank->gc.write_reg(priv->reg_base + GIO_EC(bank->id),
> - iedge_config | edge_config);
> - bank->gc.write_reg(priv->reg_base + GIO_EI(bank->id),
> - iedge_insensitive | edge_insensitive);
> - bank->gc.write_reg(priv->reg_base + GIO_LEVEL(bank->id),
> - ilevel | level);
> + gpio_generic_write_reg(&bank->chip,
> + priv->reg_base + GIO_EC(bank->id),
> + iedge_config | edge_config);
> + gpio_generic_write_reg(&bank->chip,
> + priv->reg_base + GIO_EI(bank->id),
> + iedge_insensitive | edge_insensitive);
> + gpio_generic_write_reg(&bank->chip,
> + priv->reg_base + GIO_LEVEL(bank->id),
> + ilevel | level);
>
> - raw_spin_unlock_irqrestore(&bank->gc.bgpio_lock, flags);
> return 0;
> }
>
> @@ -263,7 +265,7 @@ static void brcmstb_gpio_irq_bank_handler(struct brcmstb_gpio_bank *bank)
> {
> struct brcmstb_gpio_priv *priv = bank->parent_priv;
> struct irq_domain *domain = priv->irq_domain;
> - int hwbase = bank->gc.offset;
> + int hwbase = bank->chip.gc.offset;
> unsigned long status;
>
> while ((status = brcmstb_gpio_get_active_irqs(bank))) {
> @@ -303,7 +305,7 @@ static struct brcmstb_gpio_bank *brcmstb_gpio_hwirq_to_bank(
>
> /* banks are in descending order */
> list_for_each_entry_reverse(bank, &priv->bank_list, node) {
> - i += bank->gc.ngpio;
> + i += bank->chip.gc.ngpio;
> if (hwirq < i)
> return bank;
> }
> @@ -332,7 +334,7 @@ static int brcmstb_gpio_irq_map(struct irq_domain *d, unsigned int irq,
>
> dev_dbg(&pdev->dev, "Mapping irq %d for gpio line %d (bank %d)\n",
> irq, (int)hwirq, bank->id);
> - ret = irq_set_chip_data(irq, &bank->gc);
> + ret = irq_set_chip_data(irq, &bank->chip.gc);
> if (ret < 0)
> return ret;
> irq_set_lockdep_class(irq, &brcmstb_gpio_irq_lock_class,
> @@ -394,7 +396,7 @@ static void brcmstb_gpio_remove(struct platform_device *pdev)
> * more important to actually perform all of the steps.
> */
> list_for_each_entry(bank, &priv->bank_list, node)
> - gpiochip_remove(&bank->gc);
> + gpiochip_remove(&bank->chip.gc);
> }
>
> static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
> @@ -412,7 +414,7 @@ static int brcmstb_gpio_of_xlate(struct gpio_chip *gc,
> if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
> return -EINVAL;
>
> - offset = gpiospec->args[0] - bank->gc.offset;
> + offset = gpiospec->args[0] - bank->chip.gc.offset;
> if (offset >= gc->ngpio || offset < 0)
> return -EINVAL;
>
> @@ -493,19 +495,17 @@ static int brcmstb_gpio_irq_setup(struct platform_device *pdev,
> static void brcmstb_gpio_bank_save(struct brcmstb_gpio_priv *priv,
> struct brcmstb_gpio_bank *bank)
> {
> - struct gpio_chip *gc = &bank->gc;
> unsigned int i;
>
> for (i = 0; i < GIO_REG_STAT; i++)
> - bank->saved_regs[i] = gc->read_reg(priv->reg_base +
> - GIO_BANK_OFF(bank->id, i));
> + bank->saved_regs[i] = gpio_generic_read_reg(&bank->chip,
> + priv->reg_base + GIO_BANK_OFF(bank->id, i));
> }
>
> static void brcmstb_gpio_quiesce(struct device *dev, bool save)
> {
> struct brcmstb_gpio_priv *priv = dev_get_drvdata(dev);
> struct brcmstb_gpio_bank *bank;
> - struct gpio_chip *gc;
> u32 imask;
>
> /* disable non-wake interrupt */
> @@ -513,8 +513,6 @@ static void brcmstb_gpio_quiesce(struct device *dev, bool save)
> disable_irq(priv->parent_irq);
>
> list_for_each_entry(bank, &priv->bank_list, node) {
> - gc = &bank->gc;
> -
> if (save)
> brcmstb_gpio_bank_save(priv, bank);
>
> @@ -523,8 +521,9 @@ static void brcmstb_gpio_quiesce(struct device *dev, bool save)
> imask = bank->wake_active;
> else
> imask = 0;
> - gc->write_reg(priv->reg_base + GIO_MASK(bank->id),
> - imask);
> + gpio_generic_write_reg(&bank->chip,
> + priv->reg_base + GIO_MASK(bank->id),
> + imask);
> }
> }
>
> @@ -538,12 +537,12 @@ static void brcmstb_gpio_shutdown(struct platform_device *pdev)
> static void brcmstb_gpio_bank_restore(struct brcmstb_gpio_priv *priv,
> struct brcmstb_gpio_bank *bank)
> {
> - struct gpio_chip *gc = &bank->gc;
> unsigned int i;
>
> for (i = 0; i < GIO_REG_STAT; i++)
> - gc->write_reg(priv->reg_base + GIO_BANK_OFF(bank->id, i),
> - bank->saved_regs[i]);
> + gpio_generic_write_reg(&bank->chip,
> + priv->reg_base + GIO_BANK_OFF(bank->id, i),
> + bank->saved_regs[i]);
> }
>
> static int brcmstb_gpio_suspend(struct device *dev)
> @@ -585,6 +584,7 @@ static const struct dev_pm_ops brcmstb_gpio_pm_ops = {
>
> static int brcmstb_gpio_probe(struct platform_device *pdev)
> {
> + struct gpio_generic_chip_config config;
> struct device *dev = &pdev->dev;
> struct device_node *np = dev->of_node;
> void __iomem *reg_base;
> @@ -665,17 +665,24 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
> bank->width = bank_width;
> }
>
> + gc = &bank->chip.gc;
> +
> /*
> * Regs are 4 bytes wide, have data reg, no set/clear regs,
> * and direction bits have 0 = output and 1 = input
> */
> - gc = &bank->gc;
> - err = bgpio_init(gc, dev, 4,
> - reg_base + GIO_DATA(bank->id),
> - NULL, NULL, NULL,
> - reg_base + GIO_IODIR(bank->id), flags);
> +
> + config = (struct gpio_generic_chip_config) {
> + .dev = dev,
> + .sz = 4,
> + .dat = reg_base + GIO_DATA(bank->id),
> + .dirin = reg_base + GIO_IODIR(bank->id),
> + .flags = flags,
> + };
> +
> + err = gpio_generic_chip_init(&bank->chip, &config);
> if (err) {
> - dev_err(dev, "bgpio_init() failed\n");
> + dev_err(dev, "failed to initialize generic GPIO chip\n");
> goto fail;
> }
>
> @@ -700,7 +707,8 @@ static int brcmstb_gpio_probe(struct platform_device *pdev)
> * be retained from S5 cold boot
> */
> need_wakeup_event |= !!__brcmstb_gpio_get_active_irqs(bank);
> - gc->write_reg(reg_base + GIO_MASK(bank->id), 0);
> + gpio_generic_write_reg(&bank->chip,
> + reg_base + GIO_MASK(bank->id), 0);
>
> err = gpiochip_add_data(gc, bank);
> if (err) {
>
I suppose I'm OK with all of this, but I'm just curious about the longer
term plans for the member accesses. Is there an intent to have helpers
for things like?:
chip.gc.offset
chip.gc.ngpio
Thanks,
Doug
next prev parent reply other threads:[~2025-09-11 0:11 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-09-10 7:12 [PATCH v2 00/15] gpio: replace legacy bgpio_init() with its modernized alternative - part 4 Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 01/15] gpio: loongson1: allow building the module with COMPILE_TEST enabled Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 02/15] gpio: loongson1: use new generic GPIO chip API Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 03/15] gpio: hlwd: " Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 04/15] gpio: ath79: " Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 05/15] gpio: ath79: use the generic GPIO chip lock for IRQ handling Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 06/15] gpio: xgene-sb: use generic GPIO chip register read and write APIs Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 07/15] gpio: brcmstb: use new generic GPIO chip API Bartosz Golaszewski
2025-09-10 22:05 ` Florian Fainelli
2025-09-11 0:11 ` Doug Berger [this message]
2025-09-11 7:56 ` Bartosz Golaszewski
2025-09-11 8:02 ` Andy Shevchenko
2025-09-11 19:50 ` Doug Berger
2025-09-10 7:12 ` [PATCH v2 08/15] gpio: mt7621: " Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 09/15] gpio: mt7621: use the generic GPIO chip lock for IRQ handling Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 10/15] gpio: menz127: use new generic GPIO chip API Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 11/15] gpio: sifive: " Bartosz Golaszewski
2025-09-11 0:37 ` Samuel Holland
2025-09-11 7:58 ` Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 12/15] gpio: spacemit-k1: " Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 13/15] gpio: sodaville: " Bartosz Golaszewski
2025-09-10 7:19 ` Andy Shevchenko
2025-09-10 7:28 ` Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 14/15] gpio: mmio: " Bartosz Golaszewski
2025-09-10 7:12 ` [PATCH v2 15/15] gpio: move gpio-mmio-specific fields out of struct gpio_chip Bartosz Golaszewski
2025-09-10 21:32 ` [PATCH v2 00/15] gpio: replace legacy bgpio_init() with its modernized alternative - part 4 Linus Walleij
2025-09-11 7:38 ` Bartosz Golaszewski
2025-09-12 7:33 ` Linus Walleij
2025-09-12 7:26 ` Bartosz Golaszewski
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=e0941449-7a62-4bbb-8790-616f393f2cc8@gmail.com \
--to=opendmb@gmail.com \
--cc=albeu@free.fr \
--cc=andy@kernel.org \
--cc=angelogioacchino.delregno@collabora.com \
--cc=bartosz.golaszewski@linaro.org \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=brgl@bgdev.pl \
--cc=dlan@gentoo.org \
--cc=florian.fainelli@broadcom.com \
--cc=keguang.zhang@gmail.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-mips@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=paul.walmsley@sifive.com \
--cc=samuel.holland@sifive.com \
--cc=spacemit@lists.linux.dev \
/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