Re: [PATCH v1 2/3] gpio: add support for Phytium platform GPIO controller

[Andy Shevchenko]

On Mon, Mar 02, 2026 at 05:51:46PM +0800, Zhu Ling wrote:
> Add support for the Phytium platform GPIO controller with:
> - shared core helpers and irqchip implementation
> - platform probe path for OF/ACPI
> - Kconfig/Makefile integration

Why you can't use one of the gpio-regmap, gpio-mmio?

> The driver supports GPIO direction and value configuration, plus
> interrupt delivery for platform devices.

...

> +/*
> + * Copyright (c) 2019-2023, Phytium Technology Co., Ltd.

My calendar shows 2026...

> + */

...

> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>
> +#include <linux/seq_file.h>
> +#include <linux/interrupt.h>

Follow IWYU principle.

...

I'm not going to review the rest as gpio-remap (or gpio-mmio) should make this
driver much better and smaller.

Take your time to study existing cases.

...

> +EXPORT_SYMBOL_GPL(phytium_gpio_irq_disable);

No way.  And if it's really needed, must be namespaced.

Btw, why the three files and not a standalone single one?

...

> + * Derived from drivers/gpio/gpio-pl061.c

Why? Does it uses the same or similar register layout?

> + *   Copyright (C) 2008, 2009 Provigent Ltd.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/err.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>

> +#include <linux/kernel.h>

No way this header should be anyhow needed in this driver.

> +#include <linux/module.h>

> +#include <linux/of.h>



> +#include <linux/platform_device.h>
> +#include <linux/property.h>
> +
> +#include "gpio-phytium-core.h"

...

> +static const struct of_device_id phytium_gpio_of_match[] = {
> +	{ .compatible = "phytium,gpio", },

No inner comma.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, phytium_gpio_of_match);
> +
> +static const struct acpi_device_id phytium_gpio_acpi_match[] = {
> +	{ "PHYT0001", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, phytium_gpio_acpi_match);

These tables should be moved closer to their user.

...

> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +	struct phytium_gpio *gpio;
> +	struct gpio_irq_chip *girq;
> +	struct fwnode_handle *fwnode;

> +	int i;

Why is 'i' signed?

> +	int err, irq_count;

...

> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	gpio->regs = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(gpio->regs))
> +		return PTR_ERR(gpio->regs);

There is a combined call devm_platform_...().


> +	if (!device_get_child_node_count(dev))
> +		return -ENODEV;

Why?!

...

> +	device_for_each_child_node(dev, fwnode) {

Use _scoped() variant

> +		int idx;
> +
> +		if (fwnode_property_read_u32(fwnode, "reg", &idx) ||
> +		    idx >= MAX_NPORTS) {
> +			dev_err(dev, "missing/invalid port index\n");

			return dev_err_probe(...);

> +			fwnode_handle_put(fwnode);
> +			return -EINVAL;
> +		}
> +
> +		if ((fwnode_property_read_u32(fwnode, "ngpios",
> +					      &gpio->ngpio[idx])) &&
> +		    (fwnode_property_read_u32(fwnode, "nr-gpios",
> +					      &gpio->ngpio[idx]))) {
> +			dev_info(dev,
> +				 "failed to get number of gpios for Port%c\n",
> +				 idx ? 'B' : 'A');

It rings a bell. Do you really have to create a brand new driver? Please check
existing ones and clarify why the brand new driver is required.

> +			gpio->ngpio[idx] = NGPIO_DEFAULT;
> +		}
> +	}

...

> +	dev_info(dev, "Phytium GPIO controller @%pa registered\n",
> +		 &res->start);

The successfully probed device is kept silent in the logs.

...

> +#ifdef CONFIG_PM_SLEEP

No way, please, use modern PM macros from pm.h.

> +static int phytium_gpio_suspend(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct phytium_gpio *gpio = platform_get_drvdata(pdev);

dev_get_drvdata()

> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);
> +
> +	gpio->ctx.swporta_dr = readl(gpio->regs + GPIO_SWPORTA_DR);
> +	gpio->ctx.swporta_ddr = readl(gpio->regs + GPIO_SWPORTA_DDR);
> +	gpio->ctx.swportb_dr = readl(gpio->regs + GPIO_SWPORTB_DR);
> +	gpio->ctx.swportb_ddr = readl(gpio->regs + GPIO_SWPORTB_DDR);
> +
> +	gpio->ctx.inten = readl(gpio->regs + GPIO_INTEN);
> +	gpio->is_resuming = 1;
> +	gpio->ctx.intmask = readl(gpio->regs + GPIO_INTMASK);
> +	gpio->ctx.inttype_level = readl(gpio->regs + GPIO_INTTYPE_LEVEL);
> +	gpio->ctx.int_polarity = readl(gpio->regs + GPIO_INT_POLARITY);
> +	gpio->ctx.debounce = readl(gpio->regs + GPIO_DEBOUNCE);
> +
> +	writel(~gpio->wake_en, gpio->regs + GPIO_INTMASK);
> +	writel(gpio->wake_en, gpio->regs + GPIO_INTEN);
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	return 0;
> +}
> +
> +static int phytium_gpio_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct phytium_gpio *gpio = platform_get_drvdata(pdev);

Ditto.

> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&gpio->lock, flags);

Use guard()().

> +	writel(gpio->ctx.swporta_dr, gpio->regs + GPIO_SWPORTA_DR);
> +	writel(gpio->ctx.swporta_ddr, gpio->regs + GPIO_SWPORTA_DDR);
> +	writel(gpio->ctx.swportb_dr, gpio->regs + GPIO_SWPORTB_DR);
> +	writel(gpio->ctx.swportb_ddr, gpio->regs + GPIO_SWPORTB_DDR);
> +
> +	writel(gpio->ctx.intmask, gpio->regs + GPIO_INTMASK);
> +	writel(gpio->ctx.inttype_level, gpio->regs + GPIO_INTTYPE_LEVEL);
> +	writel(gpio->ctx.int_polarity, gpio->regs + GPIO_INT_POLARITY);
> +	writel(gpio->ctx.debounce, gpio->regs + GPIO_DEBOUNCE);
> +
> +	writel(GPIO_CLEAR_IRQ, gpio->regs + GPIO_PORTA_EOI);
> +
> +	writel(gpio->ctx.inten, gpio->regs + GPIO_INTEN);
> +	gpio->is_resuming = 0;
> +
> +	raw_spin_unlock_irqrestore(&gpio->lock, flags);
> +
> +	return 0;
> +}
> +#endif

...

> +static SIMPLE_DEV_PM_OPS(phytium_gpio_pm_ops, phytium_gpio_suspend,
> +			 phytium_gpio_resume);

Use new PM macros.

...

> +static struct platform_driver phytium_gpio_driver = {
> +	.driver		= {
> +		.name	= "gpio-phytium-platform",
> +		.pm	= &phytium_gpio_pm_ops,

> +		.of_match_table = of_match_ptr(phytium_gpio_of_match),
> +		.acpi_match_table = ACPI_PTR(phytium_gpio_acpi_match),

No way of_match_ptr() and/or ACPI_PTR() should appear in a new code.

> +	},
> +	.probe		= phytium_gpio_probe,
> +};

> +

Redundant blank line.

> +module_platform_driver(phytium_gpio_driver);

-- 
With Best Regards,
Andy Shevchenko