On Wed, Apr 15, 2020 at 09:24:48PM +0200, Linus Walleij wrote:
> This converts the FUSB302 driver to use GPIO descriptors.
> The conversion to descriptors per se is pretty straight-forward.
>
> In the process I discovered that:
>
> 1. The driver uses a completely undocumented device tree binding
> for the interrupt GPIO line, "fcs,int_n". Ooops.
>
> 2. The undocumented binding, presumably since it has not seen
> review, is just "fcs,int_n", lacking the compulsory "-gpios"
> suffix and also something that is not a good name because
> the "_n" implies the line is inverted which is something we
> handle with flags in the device tree. Ooops.
>
> 3. Possibly the driver should not be requesting the line as a
> GPIO and request the corresponding interrupt line by open
> coding, the GPIO chip is very likely doubleing as an IRQ
> controller and can probably provide an interrupt directly
> for this line with interrupts-extended = <&gpio0 ...>;
>
> 4. Possibly the IRQ should just be tagged on the I2C client node
> in the device tree like apparently ACPI does, as it overrides
> this IRQ with client->irq if that exists.
>
> But now it is too late to do much about that and as I can see
> this is used like this in the Pinebook which is a shipping product
> so let'a just contain the mess and move on.
>
> The property currently appears in:
> arch/arm64/boot/dts/rockchip/rk3399-pinebook-pro.dts
>
> Create a quirk in the GPIO OF library to allow this property
> specifically to be specified without the "-gpios" suffix, we have
> other such bindings already.
>
> Cc: Tobias Schramm <t.schramm@xxxxxxxxxxx>
> Cc: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> Cc: Yueyao Zhu <yueyao@xxxxxxxxxx>
> Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
> Cc: devicetree@xxxxxxxxxxxxxxx
> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
FWIW:
Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> This is now covered as far as GPIO is concerned but you might
> want to look into creating proper bindings for this or
> correcting the devicetree.
> ---
> drivers/gpio/gpiolib-of.c | 21 +++++++++++++++++++++
> drivers/usb/typec/tcpm/fusb302.c | 32 +++++++++-----------------------
> 2 files changed, 30 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
> index ccc449df3792..20c2c428168e 100644
> --- a/drivers/gpio/gpiolib-of.c
> +++ b/drivers/gpio/gpiolib-of.c
> @@ -460,6 +460,24 @@ static struct gpio_desc *of_find_arizona_gpio(struct device *dev,
> return of_get_named_gpiod_flags(dev->of_node, con_id, 0, of_flags);
> }
>
> +static struct gpio_desc *of_find_usb_gpio(struct device *dev,
> + const char *con_id,
> + enum of_gpio_flags *of_flags)
> +{
> + /*
> + * Currently this USB quirk is only for the Fairchild FUSB302 host which is using
> + * an undocumented DT GPIO line named "fcs,int_n" without the compulsory "-gpios"
> + * suffix.
> + */
> + if (!IS_ENABLED(CONFIG_TYPEC_FUSB302))
> + return ERR_PTR(-ENOENT);
> +
> + if (!con_id || strcmp(con_id, "fcs,int_n"))
> + return ERR_PTR(-ENOENT);
> +
> + return of_get_named_gpiod_flags(dev->of_node, con_id, 0, of_flags);
> +}
> +
> struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
> unsigned int idx, unsigned long *flags)
> {
> @@ -504,6 +522,9 @@ struct gpio_desc *of_find_gpio(struct device *dev, const char *con_id,
> if (PTR_ERR(desc) == -ENOENT)
> desc = of_find_arizona_gpio(dev, con_id, &of_flags);
>
> + if (PTR_ERR(desc) == -ENOENT)
> + desc = of_find_usb_gpio(dev, con_id, &of_flags);
> +
> if (IS_ERR(desc))
> return desc;
>
> diff --git a/drivers/usb/typec/tcpm/fusb302.c b/drivers/usb/typec/tcpm/fusb302.c
> index b498960ff72b..b28facece43c 100644
> --- a/drivers/usb/typec/tcpm/fusb302.c
> +++ b/drivers/usb/typec/tcpm/fusb302.c
> @@ -9,14 +9,13 @@
> #include <linux/delay.h>
> #include <linux/errno.h>
> #include <linux/extcon.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
> #include <linux/i2c.h>
> #include <linux/interrupt.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> #include <linux/mutex.h>
> #include <linux/of_device.h>
> -#include <linux/of_gpio.h>
> #include <linux/pinctrl/consumer.h>
> #include <linux/proc_fs.h>
> #include <linux/regulator/consumer.h>
> @@ -83,7 +82,7 @@ struct fusb302_chip {
> struct work_struct irq_work;
> bool irq_suspended;
> bool irq_while_suspended;
> - int gpio_int_n;
> + struct gpio_desc *gpio_int_n;
> int gpio_int_n_irq;
> struct extcon_dev *extcon;
>
> @@ -1618,30 +1617,17 @@ static void fusb302_irq_work(struct work_struct *work)
>
> static int init_gpio(struct fusb302_chip *chip)
> {
> - struct device_node *node;
> + struct device *dev = chip->dev;
> int ret = 0;
>
> - node = chip->dev->of_node;
> - chip->gpio_int_n = of_get_named_gpio(node, "fcs,int_n", 0);
> - if (!gpio_is_valid(chip->gpio_int_n)) {
> - ret = chip->gpio_int_n;
> - dev_err(chip->dev, "cannot get named GPIO Int_N, ret=%d", ret);
> - return ret;
> - }
> - ret = devm_gpio_request(chip->dev, chip->gpio_int_n, "fcs,int_n");
> - if (ret < 0) {
> - dev_err(chip->dev, "cannot request GPIO Int_N, ret=%d", ret);
> - return ret;
> - }
> - ret = gpio_direction_input(chip->gpio_int_n);
> - if (ret < 0) {
> - dev_err(chip->dev,
> - "cannot set GPIO Int_N to input, ret=%d", ret);
> - return ret;
> + chip->gpio_int_n = devm_gpiod_get(dev, "fcs,int_n", GPIOD_IN);
> + if (IS_ERR(chip->gpio_int_n)) {
> + dev_err(dev, "failed to request gpio_int_n\n");
> + return PTR_ERR(chip->gpio_int_n);
> }
> - ret = gpio_to_irq(chip->gpio_int_n);
> + ret = gpiod_to_irq(chip->gpio_int_n);
> if (ret < 0) {
> - dev_err(chip->dev,
> + dev_err(dev,
> "cannot request IRQ for GPIO Int_N, ret=%d", ret);
> return ret;
> }
> --
> 2.25.2
thanks,
--
heikki
