Am Donnerstag, 23. April 2020, 14:02:42 CEST schrieb Robin Murphy:
> On 2020-04-15 8:24 pm, 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.
>
> FWIW, the datasheet makes it clear than INT_N is just a regular IRQ
> output, so it really should be specified as an interrupt targeting the
> GPIO controller just like any other I2C chip, and the FUSB302 binding
> itself supports.
>
> The Pinebook Pro devicetree has only just landed this cycle, so there
> should still be plenty of time to fix it and not have to worry about an
> ugly undocumented quirk. The downstream vendor kernels use a totally
> different FUSB302 driver from the upstream one (based on extcon rather
> than the typec framework) so I don't think downstream DTBs are a concern.
I guess it's just about removing the "fcs,int_n" property, right?
Does someone want to send a patch removing that?
I'm hopeful the whole extcon mess will go away ... cros-ec-power-delivery
now seems to have moved to the typec framework, so at some point
the rk3399 typec-phy-driver could be converted as well, enabling us to
also have real type-c on non-chromebook rk3399 boards.
Heiko
> > 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>
> > ---
> > 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;
> > }
> >
>
