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
