diff mbox series

soc: fsl: qe: request pins non-exclusively

Message ID Y2nKIbKxHAMimxII@google.com
State New
Headers show
Series soc: fsl: qe: request pins non-exclusively | expand

Commit Message

Dmitry Torokhov Nov. 8, 2022, 3:16 a.m. UTC
Commit 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()") changed
qe_pin_request() to request and hold GPIO corresponding to a given pin.
Unfortunately this does not work, as fhci-hcd requests these GPIOs
first, befor calling qe_pin_request() (see
drivers/usb/host/fhci-hcd.c::of_fhci_probe()).
To fix it change qe_pin_request() to request GPIOs non-exclusively, and
free them once the code determines GPIO controller and offset for each
GPIO/pin.

Also reaching deep into gpiolib implementation is not the best idea. We
should either export gpio_chip_hwgpio() or keep converting to the global
gpio numbers space until we fix the driver to implement proper pin
control.

Fixes: 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()")
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

Compiled only, not tested on hardware.

 drivers/soc/fsl/qe/gpio.c   | 71 ++++++++++++++++++-------------------
 drivers/usb/host/fhci-hcd.c |  2 +-
 include/soc/fsl/qe/qe.h     |  5 +--
 3 files changed, 38 insertions(+), 40 deletions(-)

Comments

Linus Walleij Nov. 8, 2022, 10:50 a.m. UTC | #1
On Tue, Nov 8, 2022 at 4:16 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> Commit 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()") changed
> qe_pin_request() to request and hold GPIO corresponding to a given pin.
> Unfortunately this does not work, as fhci-hcd requests these GPIOs
> first, befor calling qe_pin_request() (see
> drivers/usb/host/fhci-hcd.c::of_fhci_probe()).
> To fix it change qe_pin_request() to request GPIOs non-exclusively, and
> free them once the code determines GPIO controller and offset for each
> GPIO/pin.
>
> Also reaching deep into gpiolib implementation is not the best idea. We
> should either export gpio_chip_hwgpio() or keep converting to the global
> gpio numbers space until we fix the driver to implement proper pin
> control.
>
> Fixes: 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()")
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Wow! Thanks for fixing this!

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I just sent that patch into the SoC patch tracker (soc@kernel.org)
with a not to apply it directly, I suggest you do the same (or ask me
to sign it off and send it).

Yours,
Linus Walleij
Dmitry Torokhov Nov. 8, 2022, 4:20 p.m. UTC | #2
On November 8, 2022 2:50:07 AM PST, Linus Walleij <linus.walleij@linaro.org> wrote:
>On Tue, Nov 8, 2022 at 4:16 AM Dmitry Torokhov
><dmitry.torokhov@gmail.com> wrote:
>
>> Commit 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()") changed
>> qe_pin_request() to request and hold GPIO corresponding to a given pin.
>> Unfortunately this does not work, as fhci-hcd requests these GPIOs
>> first, befor calling qe_pin_request() (see
>> drivers/usb/host/fhci-hcd.c::of_fhci_probe()).
>> To fix it change qe_pin_request() to request GPIOs non-exclusively, and
>> free them once the code determines GPIO controller and offset for each
>> GPIO/pin.
>>
>> Also reaching deep into gpiolib implementation is not the best idea. We
>> should either export gpio_chip_hwgpio() or keep converting to the global
>> gpio numbers space until we fix the driver to implement proper pin
>> control.
>>
>> Fixes: 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()")
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
>Wow! Thanks for fixing this!
>
>Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>
>I just sent that patch into the SoC patch tracker (soc@kernel.org)
>with a not to apply it directly, I suggest you do the same (or ask me
>to sign it off and send it).

I am not really plugged into soc patch/workflow so if you could do that that would be wonderful. 

Thanks!
diff mbox series

Patch

diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
index 0ee887f89deb..5bb71a2b5b7a 100644
--- a/drivers/soc/fsl/qe/gpio.c
+++ b/drivers/soc/fsl/qe/gpio.c
@@ -13,7 +13,7 @@ 
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/of.h>
-#include <linux/of_gpio.h>
+#include <linux/of_gpio.h>	/* for of_mm_gpio_chip */
 #include <linux/gpio/consumer.h>
 #include <linux/gpio/driver.h>
 #include <linux/slab.h>
@@ -21,13 +21,6 @@ 
 #include <linux/property.h>
 
 #include <soc/fsl/qe/qe.h>
-/*
- * FIXME: this is legacy code that is accessing gpiolib internals in order
- * to implement a custom pin controller. The proper solution is to create
- * a real combined pin control and GPIO driver in drivers/pinctrl. However
- * this hack is here for legacy code reasons.
- */
-#include "../../../gpio/gpiolib.h"
 
 struct qe_gpio_chip {
 	struct of_mm_gpio_chip mm_gc;
@@ -149,20 +142,19 @@  struct qe_pin {
 	 * something like qe_pio_controller. Someday.
 	 */
 	struct qe_gpio_chip *controller;
-	struct gpio_desc *gpiod;
 	int num;
 };
 
 /**
  * qe_pin_request - Request a QE pin
- * @np:		device node to get a pin from
- * @index:	index of a pin in the device tree
+ * @dev:	device to get the pin from
+ * @index:	index of the pin in the device tree
  * Context:	non-atomic
  *
  * This function return qe_pin so that you could use it with the rest of
  * the QE Pin Multiplexing API.
  */
-struct qe_pin *qe_pin_request(struct device_node *np, int index)
+struct qe_pin *qe_pin_request(struct device *dev, int index)
 {
 	struct qe_pin *qe_pin;
 	struct gpio_chip *gc;
@@ -171,40 +163,46 @@  struct qe_pin *qe_pin_request(struct device_node *np, int index)
 
 	qe_pin = kzalloc(sizeof(*qe_pin), GFP_KERNEL);
 	if (!qe_pin) {
-		pr_debug("%s: can't allocate memory\n", __func__);
+		dev_dbg(dev, "%s: can't allocate memory\n", __func__);
 		return ERR_PTR(-ENOMEM);
 	}
 
-	gpiod = fwnode_gpiod_get_index(of_fwnode_handle(np), NULL, index, GPIOD_ASIS, "qe");
-	if (IS_ERR(gpiod)) {
-		err = PTR_ERR(gpiod);
-		goto err0;
-	}
-	if (!gpiod) {
-		err = -EINVAL;
+	/*
+	 * Request gpio as nonexclusive as it was likely was reserved by
+	 * the caller, and we are not planning on controlling it, we only
+	 * need the descriptor to the to the gpio chip structure.
+	 */
+	gpiod = gpiod_get_index(dev, NULL, index,
+			        GPIOD_ASIS | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
+	err = PTR_ERR_OR_ZERO(gpiod);
+	if (err)
 		goto err0;
-	}
+
 	gc = gpiod_to_chip(gpiod);
 	if (WARN_ON(!gc)) {
 		err = -ENODEV;
 		goto err0;
-	}
-	qe_pin->gpiod = gpiod;
-	qe_pin->controller = gpiochip_get_data(gc);
-	/*
-	 * FIXME: this gets the local offset on the gpio_chip so that the driver
-	 * can manipulate pin control settings through its custom API. The real
-	 * solution is to create a real pin control driver for this.
-	 */
-	qe_pin->num = gpio_chip_hwgpio(gpiod);
-
-	if (!fwnode_device_is_compatible(gc->fwnode, "fsl,mpc8323-qe-pario-bank")) {
-		pr_debug("%s: tried to get a non-qe pin\n", __func__);
-		gpiod_put(gpiod);
+	} else if (!fwnode_device_is_compatible(gc->fwnode,
+						"fsl,mpc8323-qe-pario-bank")) {
+		dev_dbg(dev, "%s: tried to get a non-qe pin\n", __func__);
 		err = -EINVAL;
-		goto err0;
+	} else {
+		qe_pin->controller = gpiochip_get_data(gc);
+		/*
+		 * FIXME: this gets the local offset on the gpio_chip so that
+		 * the driver can manipulate pin control settings through its
+		 * custom API. The real solution is to create a real pin control
+		 * driver for this.
+		 */
+		qe_pin->num = desc_to_gpio(gpiod) - gc->base;
 	}
-	return qe_pin;
+
+	/* We no longer need this descriptor */
+	gpiod_put(gpiod);
+
+	if (!err)
+		return qe_pin;
+
 err0:
 	kfree(qe_pin);
 	pr_debug("%s failed with status %d\n", __func__, err);
@@ -222,7 +220,6 @@  EXPORT_SYMBOL(qe_pin_request);
  */
 void qe_pin_free(struct qe_pin *qe_pin)
 {
-	gpiod_put(qe_pin->gpiod);
 	kfree(qe_pin);
 }
 EXPORT_SYMBOL(qe_pin_free);
diff --git a/drivers/usb/host/fhci-hcd.c b/drivers/usb/host/fhci-hcd.c
index 95a44462bed0..1f666804fa91 100644
--- a/drivers/usb/host/fhci-hcd.c
+++ b/drivers/usb/host/fhci-hcd.c
@@ -651,7 +651,7 @@  static int of_fhci_probe(struct platform_device *ofdev)
 	}
 
 	for (j = 0; j < NUM_PINS; j++) {
-		fhci->pins[j] = qe_pin_request(node, j);
+		fhci->pins[j] = qe_pin_request(dev, j);
 		if (IS_ERR(fhci->pins[j])) {
 			ret = PTR_ERR(fhci->pins[j]);
 			dev_err(dev, "can't get pin %d: %d\n", j, ret);
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index b02e9fe69146..eb5079904cc8 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -172,14 +172,15 @@  static inline int par_io_data_set(u8 port, u8 pin, u8 val) { return -ENOSYS; }
 /*
  * Pin multiplexing functions.
  */
+struct device;
 struct qe_pin;
 #ifdef CONFIG_QE_GPIO
-extern struct qe_pin *qe_pin_request(struct device_node *np, int index);
+extern struct qe_pin *qe_pin_request(struct device *dev, int index);
 extern void qe_pin_free(struct qe_pin *qe_pin);
 extern void qe_pin_set_gpio(struct qe_pin *qe_pin);
 extern void qe_pin_set_dedicated(struct qe_pin *pin);
 #else
-static inline struct qe_pin *qe_pin_request(struct device_node *np, int index)
+static inline struct qe_pin *qe_pin_request(struct device *dev, int index)
 {
 	return ERR_PTR(-ENOSYS);
 }