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, before 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()")
Reviewed-by: Linus Walleij <[email protected]>
Signed-off-by: Dmitry Torokhov <[email protected]>
---
SoC team, the problematic patch has been in next for a while and it
would be great to get the fix in to make sure the driver is not broken
in 6.2. Thanks!
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(-)
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 64a64140c2fd..92794ffc25c8 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);
}
--
2.39.0.rc0.267.gcb52ba06e7-goog
--
Dmitry
On Sun, Dec 4, 2022, at 05:50, Dmitry Torokhov wrote:
>
> SoC team, the problematic patch has been in next for a while and it
> would be great to get the fix in to make sure the driver is not broken
> in 6.2. Thanks!
I have no problem taking thsi patch, but I get a merge conflict that
I'm not sure how to resolve:
@@@ -186,23 -182,27 +180,43 @@@ struct qe_pin *qe_pin_request(struct de
if (WARN_ON(!gc)) {
err = -ENODEV;
goto err0;
++<<<<<<< HEAD
+ }
+ 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 (!of_device_is_compatible(gc->of_node, "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__);
++>>>>>>> soc: fsl: qe: request pins non-exclusively
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;
}
Could you rebase the patch on top of the soc/driver branch in the
soc tree and send the updated version?
Arnd
On Sun, Dec 04, 2022 at 01:10:19PM +0100, Arnd Bergmann wrote:
> On Sun, Dec 4, 2022, at 05:50, Dmitry Torokhov wrote:
> >
> > SoC team, the problematic patch has been in next for a while and it
> > would be great to get the fix in to make sure the driver is not broken
> > in 6.2. Thanks!
>
> I have no problem taking thsi patch, but I get a merge conflict that
> I'm not sure how to resolve:
>
>
> @@@ -186,23 -182,27 +180,43 @@@ struct qe_pin *qe_pin_request(struct de
> if (WARN_ON(!gc)) {
> err = -ENODEV;
> goto err0;
> ++<<<<<<< HEAD
> + }
> + 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 (!of_device_is_compatible(gc->of_node, "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__);
> ++>>>>>>> soc: fsl: qe: request pins non-exclusively
> 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;
> }
>
> Could you rebase the patch on top of the soc/driver branch in the
> soc tree and send the updated version?
I see, it conflicts with:
c9eb6e546a23 soc: fsl: qe: Switch to use fwnode instead of of_node
that is in next but not in soc/driver tree/branch. OK, I'll rebase and I
just noticed that I was leaking gpiod in case we could not locate gc
(unlikely but still...).
Thanks.
--
Dmitry
On Sun, Dec 04, 2022 at 03:55:19PM -0800, Dmitry Torokhov wrote:
> On Sun, Dec 04, 2022 at 01:10:19PM +0100, Arnd Bergmann wrote:
> > On Sun, Dec 4, 2022, at 05:50, Dmitry Torokhov wrote:
> > >
> > > SoC team, the problematic patch has been in next for a while and it
> > > would be great to get the fix in to make sure the driver is not broken
> > > in 6.2. Thanks!
> >
> > I have no problem taking thsi patch, but I get a merge conflict that
> > I'm not sure how to resolve:
> >
> >
> > @@@ -186,23 -182,27 +180,43 @@@ struct qe_pin *qe_pin_request(struct de
> > if (WARN_ON(!gc)) {
> > err = -ENODEV;
> > goto err0;
> > ++<<<<<<< HEAD
> > + }
> > + 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 (!of_device_is_compatible(gc->of_node, "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__);
> > ++>>>>>>> soc: fsl: qe: request pins non-exclusively
> > 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;
> > }
> >
> > Could you rebase the patch on top of the soc/driver branch in the
> > soc tree and send the updated version?
>
> I see, it conflicts with:
>
> c9eb6e546a23 soc: fsl: qe: Switch to use fwnode instead of of_node
>
> that is in next but not in soc/driver tree/branch.
That's due to no reaction on the patch [1] from Freescale maintainers (*).
Either soc subsystem can pull this [2] or your patch can go via
pin control subsystem.
*) Note, there is not Arnd's name nor soc mailing list in the MAINTAINERS
regarding those files, so I had had no idea about the correct route of
the change.
[1]: https://lore.kernel.org/lkml/[email protected]/
[2]: https://lore.kernel.org/linux-gpio/Y3YY%2Fm0F%[email protected]/
--
With Best Regards,
Andy Shevchenko