On Sat, Jun 27, 2020 at 01:10:29PM +0200, Linus Walleij wrote:
> This switches the PCH UDC driver to use GPIO descriptors. The way
> this is supposed to be used is confusing. The code contains the
> following:
>
> /* GPIO port for VBUS detecting */
> static int vbus_gpio_port = -1; /* GPIO port number (-1:Not used) */
>
> So a hardcoded GPIO number in the code. Further the probe() path
> very clearly will exit if the GPIO is not found, so this driver
> can only be configured by editing the code, hard-coding a GPIO
> number into this variable.
>
> This is simply not how we do things. My guess is that this is
> used in products by patching a GPIO number into this variable and
> shipping a kernel that is compile-time tailored for the target
> system.
>
> I switched this mechanism to using a GPIO descriptor associated
> with the parent PCI device. This can be added by using the 16bit
> subsystem ID or similar to identify which exact machine we are
> running on and what GPIO is present on that machine, and then
> add a GPIO descriptor using gpiod_add_lookup_table() from
> <linux/gpio/machine.h>. Since I don't have any target systems
> I cannot add this but I'm happy to help. I put in a FIXME so
> the people actually using this driver knows what to do.
In the future I will submit a patch to fix below FIXME since I found the means
of testing, but it requires some time. Meanwhile I have tested on two Intel
platforms with such UDC and found no regressions
Tested-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Felipe, I'm fine with this patch, please proceed if it's okay.
> Cc: Felipe Balbi <balbi@xxxxxxxxxx>
> Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
> Andy: your input would be appreciated, this kind of code
> customizing random embedded Intel systems is deep water for
> me, so this is just a rough guess on how it should be done.
> ---
> drivers/usb/gadget/udc/pch_udc.c | 55 +++++++++++++-------------------
> 1 file changed, 22 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/usb/gadget/udc/pch_udc.c b/drivers/usb/gadget/udc/pch_udc.c
> index 3344fb8c4181..a9e332130091 100644
> --- a/drivers/usb/gadget/udc/pch_udc.c
> +++ b/drivers/usb/gadget/udc/pch_udc.c
> @@ -12,12 +12,9 @@
> #include <linux/interrupt.h>
> #include <linux/usb/ch9.h>
> #include <linux/usb/gadget.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/irq.h>
>
> -/* GPIO port for VBUS detecting */
> -static int vbus_gpio_port = -1; /* GPIO port number (-1:Not used) */
> -
> #define PCH_VBUS_PERIOD 3000 /* VBUS polling period (msec) */
> #define PCH_VBUS_INTERVAL 10 /* VBUS polling interval (msec) */
>
> @@ -302,13 +299,13 @@ struct pch_udc_ep {
> /**
> * struct pch_vbus_gpio_data - Structure holding GPIO informaton
> * for detecting VBUS
> - * @port: gpio port number
> + * @port: gpio descriptor for the VBUS GPIO
> * @intr: gpio interrupt number
> * @irq_work_fall Structure for WorkQueue
> * @irq_work_rise Structure for WorkQueue
> */
> struct pch_vbus_gpio_data {
> - int port;
> + struct gpio_desc *port;
> int intr;
> struct work_struct irq_work_fall;
> struct work_struct irq_work_rise;
> @@ -1253,7 +1250,7 @@ static int pch_vbus_gpio_get_value(struct pch_udc_dev *dev)
> int vbus = 0;
>
> if (dev->vbus_gpio.port)
> - vbus = gpio_get_value(dev->vbus_gpio.port) ? 1 : 0;
> + vbus = gpiod_get_value(dev->vbus_gpio.port) ? 1 : 0;
> else
> vbus = -1;
>
> @@ -1355,42 +1352,30 @@ static irqreturn_t pch_vbus_gpio_irq(int irq, void *data)
> /**
> * pch_vbus_gpio_init() - This API initializes GPIO port detecting VBUS.
> * @dev: Reference to the driver structure
> - * @vbus_gpio Number of GPIO port to detect gpio
> *
> * Return codes:
> * 0: Success
> * -EINVAL: GPIO port is invalid or can't be initialized.
> */
> -static int pch_vbus_gpio_init(struct pch_udc_dev *dev, int vbus_gpio_port)
> +static int pch_vbus_gpio_init(struct pch_udc_dev *dev)
> {
> int err;
> int irq_num = 0;
> + struct gpio_desc *gpiod;
>
> - dev->vbus_gpio.port = 0;
> + dev->vbus_gpio.port = NULL;
> dev->vbus_gpio.intr = 0;
>
> - if (vbus_gpio_port <= -1)
> - return -EINVAL;
> -
> - err = gpio_is_valid(vbus_gpio_port);
> - if (!err) {
> - pr_err("%s: gpio port %d is invalid\n",
> - __func__, vbus_gpio_port);
> - return -EINVAL;
> - }
> -
> - err = gpio_request(vbus_gpio_port, "pch_vbus");
> - if (err) {
> - pr_err("%s: can't request gpio port %d, err: %d\n",
> - __func__, vbus_gpio_port, err);
> - return -EINVAL;
> - }
> + /* Retrieve the GPIO line from the USB gadget device */
> + gpiod = devm_gpiod_get(dev->gadget.dev.parent, NULL, GPIOD_IN);
> + if (IS_ERR(gpiod))
> + return PTR_ERR(gpiod);
> + gpiod_set_consumer_name(gpiod, "pch_vbus");
>
> - dev->vbus_gpio.port = vbus_gpio_port;
> - gpio_direction_input(vbus_gpio_port);
> + dev->vbus_gpio.port = gpiod;
> INIT_WORK(&dev->vbus_gpio.irq_work_fall, pch_vbus_gpio_work_fall);
>
> - irq_num = gpio_to_irq(vbus_gpio_port);
> + irq_num = gpiod_to_irq(gpiod);
> if (irq_num > 0) {
> irq_set_irq_type(irq_num, IRQ_TYPE_EDGE_BOTH);
> err = request_irq(irq_num, pch_vbus_gpio_irq, 0,
> @@ -1416,9 +1401,6 @@ static void pch_vbus_gpio_free(struct pch_udc_dev *dev)
> {
> if (dev->vbus_gpio.intr)
> free_irq(dev->vbus_gpio.intr, dev);
> -
> - if (dev->vbus_gpio.port)
> - gpio_free(dev->vbus_gpio.port);
> }
>
> /**
> @@ -2894,7 +2876,7 @@ static int pch_udc_pcd_init(struct pch_udc_dev *dev)
> {
> pch_udc_init(dev);
> pch_udc_pcd_reinit(dev);
> - pch_vbus_gpio_init(dev, vbus_gpio_port);
> + pch_vbus_gpio_init(dev);
> return 0;
> }
>
> @@ -3096,6 +3078,13 @@ static int pch_udc_probe(struct pci_dev *pdev,
>
> dev->base_addr = pcim_iomap_table(pdev)[bar];
>
> + /*
> + * FIXME: add a GPIO descriptor table to pdev.dev using
> + * gpiod_add_descriptor_table() from <linux/gpio/machine.h> based on
> + * the PCI subsystem ID. The system-dependent GPIO is necessary for
> + * VBUS operation.
> + */
> +
> /* initialize the hardware */
> if (pch_udc_pcd_init(dev))
> return -ENODEV;
> --
> 2.25.4
>
--
With Best Regards,
Andy Shevchenko
