Re: [PATCH 3/4] gpio: imx-rpmsg: add imx-rpmsg GPIO driver

[Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>]

Hello,

On 8/21/25 11:01, Linus Walleij wrote:
> Hi Shenwei,
>
> thanks for your patch!
>
> On Mon, Aug 18, 2025 at 10:45 PM Shenwei Wang <shenwei.wang@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.wang@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!

Could this driver be generic (platform independent) ?
Perhaps i missed something, but it seems to me that there is no IMX
specific code.
Making it generic would allow other platforms to reuse it instead of
duplicating it.

Thanks,
Arnaud

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