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

[Shenwei Wang]

> -----Original Message-----
> From: Peng Fan (OSS) <peng.fan at oss.nxp.com>
> Sent: Sunday, September 28, 2025 10:37 PM
> To: Shenwei Wang <shenwei.wang at nxp.com>
> Cc: Bjorn Andersson <andersson at kernel.org>; Mathieu Poirier
> <mathieu.poirier at linaro.org>; Rob Herring <robh at kernel.org>; Krzysztof
> Kozlowski <krzk+dt at kernel.org>; Conor Dooley <conor+dt at kernel.org>; Shawn
> Guo <shawnguo at kernel.org>; Sascha Hauer <s.hauer at pengutronix.de>; Linus
> Walleij <linus.walleij at linaro.org>; Bartosz Golaszewski <brgl at bgdev.pl>;
> Pengutronix Kernel Team <kernel at pengutronix.de>; Fabio Estevam
> <festevam at gmail.com>; Peng Fan <peng.fan at nxp.com>; linux-
> remoteproc at vger.kernel.org; devicetree at vger.kernel.org; imx at lists.linux.dev;
> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; dl-linux-imx
> >+	if (!port)
> >+		return -ENODEV;
> >+
> >+	if (msg->header.type == GPIO_RPMSG_REPLY) {
> >+		port->info.reply_msg = msg;
> >+		complete(&port->info.cmd_complete);
> >+	} else if (msg->header.type == GPIO_RPMSG_NOTIFY) {
> >+		port->info.notify_msg = msg;
> >+		local_irq_save(flags);
> 
> Would you explain a bit on why need to disable IRQ on current core.
> 

The generic_handle_domain_irq is required to be called in an IRQ context.  Seems it can 
be replaced with the generic_handle_domain_irq_safe here.

Thanks,
Shenwei

> >+		generic_handle_domain_irq(port->gc.irq.domain, msg->pin_idx);
> >+		local_irq_restore(flags);
> >+	} else
> >+		dev_err(&rpdev->dev, "wrong command type!\n");
> >+
> >+	return 0;
> >+}
> >+
> >+static void imx_rpmsg_gpio_remove_action(void *data) {
> >+	struct imx_rpmsg_gpio_port *port = data;
> >+
> >+	port->info.port_store[port->idx] = 0; }
> >+
> >+static int imx_rpmsg_gpio_probe(struct platform_device *pdev) {
> >+	struct imx_rpmsg_driver_data *pltdata = pdev->dev.platform_data;
> >+	struct device_node *np = pdev->dev.of_node;
> >+	struct imx_rpmsg_gpio_port *port;
> >+	struct gpio_irq_chip *girq;
> >+	struct gpio_chip *gc;
> >+	int ret;
> >+
> >+	if (!pltdata)
> >+		return -EPROBE_DEFER;
> >+
> >+	port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
> >+	if (!port)
> >+		return -ENOMEM;
> >+
> >+	ret = of_property_read_u32(np, "reg", &port->idx);
> 
> "device_property_read_u32" should be better.
> 
> >+	if (ret)
> >+		return ret;
> >+
> >+	if (port->idx > MAX_DEV_PER_CHANNEL)
> >+		return -EINVAL;
> >+
> >+	mutex_init(&port->info.lock);
> >+	init_completion(&port->info.cmd_complete);
> >+	port->info.rpdev = pltdata->rpdev;
> >+	port->info.port_store = pltdata->channel_devices;
> >+	port->info.port_store[port->idx] = port;
> >+	if (!pltdata->rx_callback)
> >+		pltdata->rx_callback = imx_rpmsg_gpio_callback;
> >+
> >+	gc = &port->gc;
> >+	gc->owner = THIS_MODULE;
> >+	gc->parent = &pltdata->rpdev->dev;
> >+	gc->label = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-gpio%d",
> >+				   pltdata->rproc_name, port->idx);
> >+	gc->ngpio = IMX_RPMSG_GPIO_PER_PORT;
> >+	gc->base = -1;
> >+
> >+	gc->direction_input = imx_rpmsg_gpio_direction_input;
> >+	gc->direction_output = imx_rpmsg_gpio_direction_output;
> >+	gc->get = imx_rpmsg_gpio_get;
> >+	gc->set = imx_rpmsg_gpio_set;
> >+
> >+	platform_set_drvdata(pdev, port);
> >+	girq = &gc->irq;
> >+	gpio_irq_chip_set_chip(girq, &imx_rpmsg_irq_chip);
> >+	girq->parent_handler = NULL;
> >+	girq->num_parents = 0;
> >+	girq->parents = NULL;
> >+	girq->chip->name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "%s-
> gpio%d",
> >+				 pltdata->rproc_name, port->idx);
> 
> Align pltdata->rproc_name with the upper line '('.
> 
> >+
> >+	devm_add_action_or_reset(&pdev->dev,
> imx_rpmsg_gpio_remove_action,
> >+port);
> 
> return value should be checked.
> 
> >+
> >+	return devm_gpiochip_add_data(&pdev->dev, gc, port); }
> >+
> 
> Thanks,
> Peng