Re: [PATCH v2] soc: fsl: qe: request pins non-exclusively

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

On Sun, Dec 04, 2022 at 03:59:29PM -0800, Dmitry Torokhov 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

before ?

> 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

GPIO number

> control.

With or without being rebased on top of the PR that in conflict with this
change,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
(see some nit-picks below)

> Fixes: 84582f9ed090 ("soc: fsl: qe: Avoid using gpio_to_desc()")
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> 
> v2:
> 
>  - rebased on top of soc/driver. This will conflict with
>    c9eb6e546a23 soc: fsl: qe: Switch to use fwnode instead of of_node
>    found in next, the resolution is trivial: accept
>    fwnode_device_is_compatible() line found in next.
>  - fixed leak of gpiod when gc is not found
>  - dropped Linus' reviewed-by as the patch changed
> 
>  drivers/soc/fsl/qe/gpio.c   | 51 +++++++++++++++++--------------------
>  drivers/usb/host/fhci-hcd.c |  2 +-
>  include/soc/fsl/qe/qe.h     |  5 ++--
>  3 files changed, 28 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/soc/fsl/qe/gpio.c b/drivers/soc/fsl/qe/gpio.c
> index af9193e7e49b..1440922341d8 100644
> --- a/drivers/soc/fsl/qe/gpio.c
> +++ b/drivers/soc/fsl/qe/gpio.c
> @@ -13,19 +13,12 @@
>  #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>
>  #include <linux/export.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;
> @@ -147,65 +140,70 @@ 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;
>  	struct gpio_desc *gpiod;
> +	int gpio_num;
>  	int err;
>  
>  	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 reserved by the

GPIO

> +	 * caller, and we are not planning on controlling it, we only need
> +	 * the descriptor to the to the gpio chip structure.

GPIO

> +	 */
> +	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);
> +	gpio_num = desc_to_gpio(gpiod);
> +	/* We no longer need this descriptor */
> +	gpiod_put(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);
> +	qe_pin->num = gpio_num - gc->base;
>  
>  	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);
> +		dev_dbg(dev, "%s: tried to get a non-qe pin\n", __func__);
>  		err = -EINVAL;
>  		goto err0;
>  	}
>  	return qe_pin;
>  err0:
>  	kfree(qe_pin);
> -	pr_debug("%s failed with status %d\n", __func__, err);

> +	dev_dbg(dev, "%s failed with status %d\n", __func__, err);

You can kill these __func__ in dev_dbg():s.

>  	return ERR_PTR(err);
>  }
>  EXPORT_SYMBOL(qe_pin_request);
> @@ -220,7 +218,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);
>  }
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 
> 
> -- 
> Dmitry

-- 
With Best Regards,
Andy Shevchenko