Hi Shenwei,
thanks for your patch!
On Mon, Aug 18, 2025 at 10:45 PM Shenwei Wang <shenwei.w...@nxp.com> wrote:
> On i.MX SoCs, the system may include two processors:
> - An MCU running an RTOS
> - An MPU running Linux
>
> These processors communicate via the RPMSG protocol.
> The driver implements the standard GPIO interface, allowing
> the Linux side to control GPIO controllers which reside in
> the remote processor via RPMSG protocol.
>
> Signed-off-by: Shenwei Wang <shenwei.w...@nxp.com>
Since this is a first RPMSG GPIO driver, I'd like if Björn and/or
Mathieu have a look at it so I'm sure it is RPMSG-proper!
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a437fe652dbc..2ce4e9b5225e 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -402,6 +402,17 @@ config GPIO_ICH
>
> If unsure, say N.
>
> +config GPIO_IMX_RPMSG
> + tristate "NXP i.MX SoC RPMSG GPIO support"
> + depends on IMX_REMOTEPROC && RPMSG && GPIOLIB
> + default IMX_REMOTEPROC
> + help
> + Say yes here to support the RPMSG GPIO functions on i.MX SoC based
> + platform. Currently supported devices: i.MX7ULP, i.MX8ULP, i.MX8x,
> + and i.MX9x.
> +
> + If unsure, say N.
This is sorted under memory-mapped GPIO, but it isn't.
Create a new submenu:
menu "RPMSG GPIO drivers"
depends on RPMSG
And put it here as the first such driver.
No need to have a dependency on RPMSG in the GPIO_IMX_RPMSG
Kconfig entry after this.
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>
bitops.h or just bits.h? Check which one you actually use.
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/imx_rpmsg.h>
> +#include <linux/init.h>
> +#include <linux/irqdomain.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_qos.h>
Are you really using pm_qos?
> +#include <linux/rpmsg.h>
> +#include <linux/virtio.h>
> +#include <linux/workqueue.h>
(...)
> +struct imx_rpmsg_gpio_port {
> + struct gpio_chip gc;
> + struct irq_chip chip;
This irqchip doesn't look very immutable.
Look at other patches rewriting irqchips to be immutable
and break this out to a static const struct irq_chip with
IRQCHIP_IMMUTABLE set instead.
> +static int imx_rpmsg_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> + struct gpio_rpmsg_data *msg = NULL;
> + int ret;
> +
> + mutex_lock(&port->info.lock);
Please use guards for all the mutexes:
#include <linux/cleanup.h>
guard(mutex)(&port->info.lock);
and it will be released as you exit the function.
> +static int imx_rpmsg_gpio_direction_input(struct gpio_chip *gc,
> + unsigned int gpio)
> +{
> + struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> + struct gpio_rpmsg_data *msg = NULL;
> + int ret;
> +
> + mutex_lock(&port->info.lock);
Dito for all these instances.
(Saves you a bunch of lines!)
> +static void imx_rpmsg_irq_bus_lock(struct irq_data *d)
> +{
> + struct imx_rpmsg_gpio_port *port = irq_data_get_irq_chip_data(d);
> +
> + mutex_lock(&port->info.lock);
> +}
Here you need to keep the classic mutex_lock() though,
because of the irqchip locking abstraction helper.
> +static struct irq_chip imx_rpmsg_irq_chip = {
const
> + .irq_mask = imx_rpmsg_mask_irq,
> + .irq_unmask = imx_rpmsg_unmask_irq,
> + .irq_set_wake = imx_rpmsg_irq_set_wake,
> + .irq_set_type = imx_rpmsg_irq_set_type,
> + .irq_shutdown = imx_rpmsg_irq_shutdown,
> + .irq_bus_lock = imx_rpmsg_irq_bus_lock,
> + .irq_bus_sync_unlock = imx_rpmsg_irq_bus_sync_unlock,
.flags = IRQCHIP_IMMUTABLE,
probably also:
GPIOCHIP_IRQ_RESOURCE_HELPERS,
?
I think you want to properly mark GPIO lines as used for
IRQs!
> +static int imx_rpmsg_gpio_to_irq(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct imx_rpmsg_gpio_port *port = gpiochip_get_data(gc);
> + int irq;
> +
> + irq = irq_find_mapping(port->domain, gpio);
> + if (irq > 0) {
> + irq_set_chip_data(irq, port);
> + irq_set_chip_and_handler(irq, &port->chip, handle_level_irq);
> + }
> +
> + return irq;
> +}
Ugh we try to to use custom to_irq() if we can...
Do you have to?
Can't you use
select GPIOLIB_IRQCHIP
and be inspired by other chips using the irqchip
helper library?
We almost always use that these days.
> + /* create an irq domain */
> + port->chip = imx_rpmsg_irq_chip;
> + port->chip.name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
> + pltdata->rproc_name, port->idx);
> + port->dev = &pdev->dev;
> +
> + irq_base = devm_irq_alloc_descs(&pdev->dev, -1, 0,
> IMX_RPMSG_GPIO_PER_PORT,
> + numa_node_id());
> + if (irq_base < 0) {
> + dev_err(&pdev->dev, "Failed to alloc irq_descs\n");
> + return irq_base;
> + }
> +
> + port->domain = irq_domain_create_legacy(of_node_to_fwnode(np),
> + IMX_RPMSG_GPIO_PER_PORT,
> + irq_base, 0,
> + &irq_domain_simple_ops, port);
> + if (!port->domain) {
> + dev_err(&pdev->dev, "Failed to allocate IRQ domain\n");
> + return -EINVAL;
> + }
This also looks unnecessarily custom.
Try to use GPIOLIB_IRQCHIP.
> +static struct platform_driver imx_rpmsg_gpio_driver = {
> + .driver = {
> + .name = "gpio-imx-rpmsg",
> + .of_match_table = imx_rpmsg_gpio_dt_ids,
> + },
> + .probe = imx_rpmsg_gpio_probe,
> +};
> +
> +static int __init gpio_imx_rpmsg_init(void)
> +{
> + return platform_driver_register(&imx_rpmsg_gpio_driver);
> +}
> +
> +device_initcall(gpio_imx_rpmsg_init);
No please just do:
module_platform_driver(imx_rpmsg_gpio_driver);
Fix up these things to begin with and then we can
look at details!
Yours,
Linus Walleij
