Remaining sunxi musb patches

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 474

Quoted 5 lines · Hans de Goede

drivers/phy/phy-sun4i-usb.c +5 -0

+	/* We either want both gpio pins or neither (when in host mode) */
+	if (!data->id_det_gpio != !data->vbus_det_gpio) {
+		dev_err(dev, "failed to get id or vbus detect pin\n");
+		return -ENODEV;
+	}

The fact that the driver expects both to be set if one is should be in the binding documentation.

Quoted 9 lines · Maxime Ripard

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 474

Quoted 5 lines · Hans de Goede

drivers/phy/phy-sun4i-usb.c +5 -0

+	/* We either want both gpio pins or neither (when in host mode) */
+	if (!data->id_det_gpio != !data->vbus_det_gpio) {
+		dev_err(dev, "failed to get id or vbus detect pin\n");
+		return -ENODEV;
+	}

The fact that the driver expects both to be set if one is should be in the binding documentation.

That would be behavior description, and I've always been told that the binding should only document the hardware description, not the behavior.

Also this may change in the future as we add support for more boards, e.g. the cubieboard seems to be unable to turn vbus off ever, so vbus-det will always read as 1 / high, making it rather useless. This is something which I still need to figure out how to deal with.

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:

(quoted diff) +0 -2

-Do not call regulator_disable in an unbalanced manner when an external vbus
  is present
---
  .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
  drivers/phy/Kconfig                                |   1 +
  drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
  3 files changed, 281 insertions(+), 11 deletions(-)

Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt +16 -2

index 16528b9..557fa99 100644
@@ -23,6 +23,13 @@ Required properties:
    * "usb1_reset"
    * "usb2_reset" for sun4i, sun6i or sun7i
 
+Optional properties:
+- usb0_id_det-gpios : gpio phandle for reading the otg id pin value
+- usb0_vbus_det-gpios : gpio phandle for detecting the presence of usb0 vbus
+- usb0_vbus-supply : regulator phandle for controller usb0 vbus
+- usb1_vbus-supply : regulator phandle for controller usb1 vbus
+- usb2_vbus-supply : regulator phandle for controller usb2 vbus
+
  Example:
  	usbphy: phy at 0x01c13400 {
  		#phy-cells = <1>;

@@ -32,6 +39,13 @@ Example:
  		reg-names = "phy_ctrl", "pmu1", "pmu2";
  		clocks = <&usb_clk 8>;
  		clock-names = "usb_phy";
-		resets = <&usb_clk 1>, <&usb_clk 2>;
-		reset-names = "usb1_reset", "usb2_reset";
+		resets = <&usb_clk 0>, <&usb_clk 1>, <&usb_clk 2>;
+		reset-names = "usb0_reset", "usb1_reset", "usb2_reset";
+		pinctrl-names = "default";
+		pinctrl-0 = <&usb0_id_detect_pin>, <&usb0_vbus_detect_pin>;
+		usb0_id_det-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>; /* PH19 */
+		usb0_vbus_det-gpios = <&pio 7 22 GPIO_ACTIVE_HIGH>; /* PH22 */
+		usb0_vbus-supply = <&reg_usb0_vbus>;
+		usb1_vbus-supply = <&reg_usb1_vbus>;
+		usb2_vbus-supply = <&reg_usb2_vbus>;
  	};

drivers/phy/Kconfig +0 -0

index a53bd5b..4614fba 100644

drivers/phy/Kconfig +0 -0

  	select GENERIC_PHY
  	help
  	  Enable this to support the transceiver that is part of Allwinner

drivers/phy/phy-sun4i-usb.c +264 -9

index 91c5be4..b45d707 100644
@@ -1,7 +1,7 @@
  /*
   * Allwinner sun4i USB phy driver
   *
- * Copyright (C) 2014 Hans de Goede <[email protected]>
+ * Copyright (C) 2014-2015 Hans de Goede <[email protected]>
   *
   * Based on code from
   * Allwinner Technology Co., Ltd. <www.allwinnertech.com>

@@ -23,17 +23,23 @@
 
  #include <linux/clk.h>
  #include <linux/err.h>
+#include <linux/extcon.h>
  #include <linux/io.h>
+#include <linux/interrupt.h>
  #include <linux/kernel.h>
  #include <linux/module.h>
  #include <linux/mutex.h>
  #include <linux/of.h>
  #include <linux/of_address.h>
+#include <linux/of_gpio.h>
  #include <linux/phy/phy.h>
  #include <linux/phy/phy-sun4i-usb.h>
  #include <linux/platform_device.h>
  #include <linux/regulator/consumer.h>
  #include <linux/reset.h>
+#include <linux/workqueue.h>
+
+#define DRIVER_NAME "sun4i-usb-phy"
 
  #define REG_ISCR			0x00
  #define REG_PHYCTL			0x04

@@ -47,6 +53,17 @@
  #define SUNXI_AHB_INCRX_ALIGN_EN	BIT(8)
  #define SUNXI_ULPI_BYPASS_EN		BIT(0)
 
+/* ISCR, Interface Status and Control bits */
+#define ISCR_ID_PULLUP_EN		(1 << 17)
+#define ISCR_DPDM_PULLUP_EN	(1 << 16)
+/* sunxi has the phy id/vbus pins not connected, so we use the force bits */
+#define ISCR_FORCE_ID_MASK	(3 << 14)
+#define ISCR_FORCE_ID_LOW		(2 << 14)
+#define ISCR_FORCE_ID_HIGH	(3 << 14)
+#define ISCR_FORCE_VBUS_MASK	(3 << 12)
+#define ISCR_FORCE_VBUS_LOW	(2 << 12)
+#define ISCR_FORCE_VBUS_HIGH	(3 << 12)
+
  /* Common Control Bits for Both PHYs */
  #define PHY_PLL_BW			0x03
  #define PHY_RES45_CAL_EN		0x0c

@@ -63,6 +80,13 @@
 
  #define MAX_PHYS			3
 
+/*
+ * Note do not raise the debounce time, we must report Vusb high within 100ms
+ * otherwise we get Vbus errors
+ */
+#define DEBOUNCE_TIME			msecs_to_jiffies(50)
+#define POLL_TIME			msecs_to_jiffies(250)
+
  struct sun4i_usb_phy_data {
  	void __iomem *base;
  	struct mutex mutex;

@@ -74,13 +98,58 @@ struct sun4i_usb_phy_data {
  		struct regulator *vbus;
  		struct reset_control *reset;
  		struct clk *clk;
+		bool regulator_on;
  		int index;
  	} phys[MAX_PHYS];
+	/* phy0 / otg related variables */
+	struct extcon_dev extcon;
+	const char *extcon_cable_names[3];
+	bool phy0_init;
+	bool phy0_poll;
+	struct gpio_desc *id_det_gpio;
+	struct gpio_desc *vbus_det_gpio;
+	int id_det_irq;
+	int vbus_det_irq;
+	int id_det;
+	int vbus_det;
+	struct delayed_work detect;
  };
 
  #define to_sun4i_usb_phy_data(phy) \
  	container_of((phy), struct sun4i_usb_phy_data, phys[(phy)->index])
 
+static void sun4i_usb_phy0_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+	u32 iscr;
+
+	iscr = readl(data->base + REG_ISCR);
+	iscr &= ~clr;
+	iscr |= set;
+	writel(iscr, data->base + REG_ISCR);
+}
+
+static void sun4i_usb_phy0_set_id_detect(struct phy *phy, u32 val)
+{
+	if (val)
+		val = ISCR_FORCE_ID_HIGH;
+	else
+		val = ISCR_FORCE_ID_LOW;
+
+	sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_ID_MASK, val);
+}
+
+static void sun4i_usb_phy0_set_vbus_detect(struct phy *phy, u32 val)
+{
+	if (val)
+		val = ISCR_FORCE_VBUS_HIGH;
+	else
+		val = ISCR_FORCE_VBUS_LOW;
+
+	sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_VBUS_MASK, val);
+}
+
  static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
  				int len)
  {

@@ -171,12 +240,39 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 
  	sun4i_usb_phy_passby(phy, 1);
 
+	if (phy->index == 0) {
+		data->phy0_init = true;
+
+		/* Enable pull-ups */
+		sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_DPDM_PULLUP_EN);
+		sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_ID_PULLUP_EN);
+
+		if (data->id_det_gpio) {
+			/* OTG mode, force ISCR and cable state updates */
+			data->id_det = -1;
+			data->vbus_det = -1;
+			queue_delayed_work(system_wq, &data->detect, 0);
+		} else {
+			/* Host only mode */
+			sun4i_usb_phy0_set_id_detect(_phy, 0);
+			sun4i_usb_phy0_set_vbus_detect(_phy, 1);
+		}
+	}
+
  	return 0;
  }
 
  static int sun4i_usb_phy_exit(struct phy *_phy)
  {
  	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+	if (phy->index == 0) {
+		/* Disable pull-ups */
+		sun4i_usb_phy0_update_iscr(_phy, ISCR_DPDM_PULLUP_EN, 0);
+		sun4i_usb_phy0_update_iscr(_phy, ISCR_ID_PULLUP_EN, 0);
+		data->phy0_init = false;
+	}
 
  	sun4i_usb_phy_passby(phy, 0);
  	reset_control_assert(phy->reset);

@@ -188,20 +284,46 @@ static int sun4i_usb_phy_exit(struct phy *_phy)
  static int sun4i_usb_phy_power_on(struct phy *_phy)
  {
  	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
-	int ret = 0;
+	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+	int ret;
+
+	if (!phy->vbus || phy->regulator_on)
+		return 0;
+
+	/* For phy0 only turn on Vbus if we don't have an ext. Vbus */
+	if (phy->index == 0 && data->vbus_det)
+		return 0;
 
-	if (phy->vbus)
-		ret = regulator_enable(phy->vbus);
+	ret = regulator_enable(phy->vbus);
+	if (ret)
+		return ret;
 
-	return ret;
+	phy->regulator_on = true;
+
+	/* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
+	if (phy->index == 0 && data->phy0_poll)
+		mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+	return 0;
  }
 
  static int sun4i_usb_phy_power_off(struct phy *_phy)
  {
  	struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+	struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+	if (!phy->vbus || !phy->regulator_on)
+		return 0;
 
-	if (phy->vbus)
-		regulator_disable(phy->vbus);
+	regulator_disable(phy->vbus);
+	phy->regulator_on = false;
+
+	/*
+	 * phy0 vbus typically slowly discharges, sometimes this causes the
+	 * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan.
+	 */
+	if (phy->index == 0 && !data->phy0_poll)
+		mod_delayed_work(system_wq, &data->detect, POLL_TIME);
 
  	return 0;
  }

@@ -221,6 +343,61 @@ static struct phy_ops sun4i_usb_phy_ops = {
  	.owner		= THIS_MODULE,
  };
 
+static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
+{
+	struct sun4i_usb_phy_data *data =
+		container_of(work, struct sun4i_usb_phy_data, detect.work);
+	struct phy *phy0 = data->phys[0].phy;
+	int id_det, vbus_det, id_notify = 0, vbus_notify = 0;
+
+	id_det = gpiod_get_value_cansleep(data->id_det_gpio);
+	vbus_det = gpiod_get_value_cansleep(data->vbus_det_gpio);
+
+	mutex_lock(&phy0->mutex);
+
+	if (!data->phy0_init) {
+		mutex_unlock(&phy0->mutex);
+		return;
+	}
+
+	if (id_det != data->id_det) {
+		sun4i_usb_phy0_set_id_detect(phy0, id_det);
+		data->id_det = id_det;
+		id_notify = 1;
+	}
+
+	if (vbus_det != data->vbus_det) {
+		sun4i_usb_phy0_set_vbus_detect(phy0, vbus_det);
+		data->vbus_det = vbus_det;
+		vbus_notify = 1;
+	}
+
+	mutex_unlock(&phy0->mutex);
+
+	if (id_notify)
+		extcon_set_cable_state(&data->extcon,
+				       extcon_cable_name[EXTCON_USB_HOST],
+				       !id_det);
+
+	if (vbus_notify)
+		extcon_set_cable_state(&data->extcon,
+				       extcon_cable_name[EXTCON_USB],
+				       vbus_det);
+
+	if (data->phy0_poll)
+		queue_delayed_work(system_wq, &data->detect, POLL_TIME);
+}
+
+static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
+{
+	struct sun4i_usb_phy_data *data = dev_id;
+
+	/* vbus or id changed, let the pins settle and then scan them */
+	mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+	return IRQ_HANDLED;
+}
+
  static struct phy *sun4i_usb_phy_xlate(struct device *dev,
  					struct of_phandle_args *args)
  {

@@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
  	struct phy_provider *phy_provider;
  	bool dedicated_clocks;
  	struct resource *res;
-	int i;
+	int i, ret;
 
  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
  	if (!data)
  		return -ENOMEM;
 
  	mutex_init(&data->mutex);
+	INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
+	data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST];
+	data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB];
+	data->extcon_cable_names[2] = NULL;
+	data->extcon.name = DRIVER_NAME;
+	data->extcon.supported_cable = data->extcon_cable_names;
+	data->extcon.dev.parent = dev;
 
  	if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
  		data->num_phys = 2;

@@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
  	if (IS_ERR(data->base))
  		return PTR_ERR(data->base);
 
+	data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN);
+	if (IS_ERR(data->id_det_gpio)) {
+		if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		data->id_det_gpio = NULL;
+	}
+
+	data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN);
+	if (IS_ERR(data->vbus_det_gpio)) {
+		if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		data->vbus_det_gpio = NULL;
+	}
+
+	/* We either want both gpio pins or neither (when in host mode) */
+	if (!data->id_det_gpio != !data->vbus_det_gpio) {
+		dev_err(dev, "failed to get id or vbus detect pin\n");
+		return -ENODEV;
+	}
+
+	if (data->id_det_gpio) {
+		ret = devm_extcon_dev_register(dev, &data->extcon);
+		if (ret) {
+			dev_err(dev, "failed to register extcon: %d\n", ret);
+			return ret;
+		}
+	}
+
  	for (i = 0; i < data->num_phys; i++) {
  		struct sun4i_usb_phy *phy = data->phys + i;
  		char name[16];

@@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
  		phy_set_drvdata(phy->phy, &data->phys[i]);
  	}
 
+	data->id_det_irq = gpiod_to_irq(data->id_det_gpio);
+	data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
+	if (data->id_det_irq  < 0 || data->vbus_det_irq < 0)
+		data->phy0_poll = true;
+
+	if (data->id_det_irq >= 0) {
+		ret = devm_request_irq(dev, data->id_det_irq,
+				sun4i_usb_phy0_id_vbus_det_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				"usb0-id-det", data);
+		if (ret) {
+			dev_err(dev, "Err requesting id-det-irq: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (data->vbus_det_irq >= 0) {
+		ret = devm_request_irq(dev, data->vbus_det_irq,
+				sun4i_usb_phy0_id_vbus_det_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				"usb0-vbus-det", data);
+		if (ret) {
+			dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret);
+			return ret;
+		}
+	}
+
  	dev_set_drvdata(dev, data);
  	phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
 
  	return PTR_ERR_OR_ZERO(phy_provider);
  }
 
+static int sun4i_usb_phy_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+	if (data->id_det_irq >= 0)
+		devm_free_irq(dev, data->id_det_irq, data);
+	if (data->vbus_det_irq >= 0)
+		devm_free_irq(dev, data->vbus_det_irq, data);
+
+	cancel_delayed_work_sync(&data->detect);
+
+	return 0;
+}
+
  static const struct of_device_id sun4i_usb_phy_of_match[] = {
  	{ .compatible = "allwinner,sun4i-a10-usb-phy" },
  	{ .compatible = "allwinner,sun5i-a13-usb-phy" },

@@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
 
  static struct platform_driver sun4i_usb_phy_driver = {
  	.probe	= sun4i_usb_phy_probe,
+	.remove	= sun4i_usb_phy_remove,
  	.driver = {
  		.of_match_table	= sun4i_usb_phy_of_match,
-		.name  = "sun4i-usb-phy",
+		.name  = DRIVER_NAME,
  	}
  };
  module_platform_driver(sun4i_usb_phy_driver);

+Chanwoo

Hi,

Quoted 71 lines · Hans de Goede

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:

(quoted diff) +0 -2

-Do not call regulator_disable in an unbalanced manner when an external vbus
  is present
---
  .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
  drivers/phy/Kconfig                                |   1 +
  drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
  3 files changed, 281 insertions(+), 11 deletions(-)

Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt +16 -2

index 16528b9..557fa99 100644
@@ -23,6 +23,13 @@ Required properties:
    * "usb1_reset"
    * "usb2_reset" for sun4i, sun6i or sun7i
 
+Optional properties:
+- usb0_id_det-gpios : gpio phandle for reading the otg id pin value
+- usb0_vbus_det-gpios : gpio phandle for detecting the presence of usb0 vbus
+- usb0_vbus-supply : regulator phandle for controller usb0 vbus
+- usb1_vbus-supply : regulator phandle for controller usb1 vbus
+- usb2_vbus-supply : regulator phandle for controller usb2 vbus
+
  Example:
      usbphy: phy at 0x01c13400 {
          #phy-cells = <1>;

@@ -32,6 +39,13 @@ Example:
          reg-names = "phy_ctrl", "pmu1", "pmu2";
          clocks = <&usb_clk 8>;
          clock-names = "usb_phy";
-        resets = <&usb_clk 1>, <&usb_clk 2>;
-        reset-names = "usb1_reset", "usb2_reset";
+        resets = <&usb_clk 0>, <&usb_clk 1>, <&usb_clk 2>;
+        reset-names = "usb0_reset", "usb1_reset", "usb2_reset";
+        pinctrl-names = "default";
+        pinctrl-0 = <&usb0_id_detect_pin>, <&usb0_vbus_detect_pin>;
+        usb0_id_det-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>; /* PH19 */
+        usb0_vbus_det-gpios = <&pio 7 22 GPIO_ACTIVE_HIGH>; /* PH22 */
+        usb0_vbus-supply = <&reg_usb0_vbus>;
+        usb1_vbus-supply = <&reg_usb1_vbus>;
+        usb2_vbus-supply = <&reg_usb2_vbus>;
      };

drivers/phy/Kconfig +0 -1

index a53bd5b..4614fba 100644
--- a/drivers/phy/Kconfig

drivers/phy/Kconfig config PHY_SUN4I_USB · line 176

Quoted 6 lines

+++ b/drivers/phy/Kconfig

drivers/phy/Kconfig +1 -0

@@ -173,6 +173,7 @@ config PHY_SUN4I_USB
      tristate "Allwinner sunxi SoC USB PHY driver"
      depends on ARCH_SUNXI && HAS_IOMEM && OF
      depends on RESET_CONTROLLER
+    select EXTCON

Avoid using 'select' on visible Kconfig symbols.

Also please split the patch to make the reviewing a bit easier.

Thanks
Kishon

Quoted 411 lines

drivers/phy/Kconfig +0 -0

      select GENERIC_PHY
      help
        Enable this to support the transceiver that is part of Allwinner

drivers/phy/phy-sun4i-usb.c +264 -9

index 91c5be4..b45d707 100644
@@ -1,7 +1,7 @@
  /*
   * Allwinner sun4i USB phy driver
   *
- * Copyright (C) 2014 Hans de Goede <[email protected]>
+ * Copyright (C) 2014-2015 Hans de Goede <[email protected]>
   *
   * Based on code from
   * Allwinner Technology Co., Ltd. <www.allwinnertech.com>

@@ -23,17 +23,23 @@
 
  #include <linux/clk.h>
  #include <linux/err.h>
+#include <linux/extcon.h>
  #include <linux/io.h>
+#include <linux/interrupt.h>
  #include <linux/kernel.h>
  #include <linux/module.h>
  #include <linux/mutex.h>
  #include <linux/of.h>
  #include <linux/of_address.h>
+#include <linux/of_gpio.h>
  #include <linux/phy/phy.h>
  #include <linux/phy/phy-sun4i-usb.h>
  #include <linux/platform_device.h>
  #include <linux/regulator/consumer.h>
  #include <linux/reset.h>
+#include <linux/workqueue.h>
+
+#define DRIVER_NAME "sun4i-usb-phy"
 
  #define REG_ISCR            0x00
  #define REG_PHYCTL            0x04

@@ -47,6 +53,17 @@
  #define SUNXI_AHB_INCRX_ALIGN_EN    BIT(8)
  #define SUNXI_ULPI_BYPASS_EN        BIT(0)
 
+/* ISCR, Interface Status and Control bits */
+#define ISCR_ID_PULLUP_EN        (1 << 17)
+#define ISCR_DPDM_PULLUP_EN    (1 << 16)
+/* sunxi has the phy id/vbus pins not connected, so we use the force bits */
+#define ISCR_FORCE_ID_MASK    (3 << 14)
+#define ISCR_FORCE_ID_LOW        (2 << 14)
+#define ISCR_FORCE_ID_HIGH    (3 << 14)
+#define ISCR_FORCE_VBUS_MASK    (3 << 12)
+#define ISCR_FORCE_VBUS_LOW    (2 << 12)
+#define ISCR_FORCE_VBUS_HIGH    (3 << 12)
+
  /* Common Control Bits for Both PHYs */
  #define PHY_PLL_BW            0x03
  #define PHY_RES45_CAL_EN        0x0c

@@ -63,6 +80,13 @@
 
  #define MAX_PHYS            3
 
+/*
+ * Note do not raise the debounce time, we must report Vusb high within 100ms
+ * otherwise we get Vbus errors
+ */
+#define DEBOUNCE_TIME            msecs_to_jiffies(50)
+#define POLL_TIME            msecs_to_jiffies(250)
+
  struct sun4i_usb_phy_data {
      void __iomem *base;
      struct mutex mutex;

@@ -74,13 +98,58 @@ struct sun4i_usb_phy_data {
          struct regulator *vbus;
          struct reset_control *reset;
          struct clk *clk;
+        bool regulator_on;
          int index;
      } phys[MAX_PHYS];
+    /* phy0 / otg related variables */
+    struct extcon_dev extcon;
+    const char *extcon_cable_names[3];
+    bool phy0_init;
+    bool phy0_poll;
+    struct gpio_desc *id_det_gpio;
+    struct gpio_desc *vbus_det_gpio;
+    int id_det_irq;
+    int vbus_det_irq;
+    int id_det;
+    int vbus_det;
+    struct delayed_work detect;
  };
 
  #define to_sun4i_usb_phy_data(phy) \
      container_of((phy), struct sun4i_usb_phy_data, phys[(phy)->index])
 
+static void sun4i_usb_phy0_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    u32 iscr;
+
+    iscr = readl(data->base + REG_ISCR);
+    iscr &= ~clr;
+    iscr |= set;
+    writel(iscr, data->base + REG_ISCR);
+}
+
+static void sun4i_usb_phy0_set_id_detect(struct phy *phy, u32 val)
+{
+    if (val)
+        val = ISCR_FORCE_ID_HIGH;
+    else
+        val = ISCR_FORCE_ID_LOW;
+
+    sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_ID_MASK, val);
+}
+
+static void sun4i_usb_phy0_set_vbus_detect(struct phy *phy, u32 val)
+{
+    if (val)
+        val = ISCR_FORCE_VBUS_HIGH;
+    else
+        val = ISCR_FORCE_VBUS_LOW;
+
+    sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_VBUS_MASK, val);
+}
+
  static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
                  int len)
  {

@@ -171,12 +240,39 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 
      sun4i_usb_phy_passby(phy, 1);
 
+    if (phy->index == 0) {
+        data->phy0_init = true;
+
+        /* Enable pull-ups */
+        sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_DPDM_PULLUP_EN);
+        sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_ID_PULLUP_EN);
+
+        if (data->id_det_gpio) {
+            /* OTG mode, force ISCR and cable state updates */
+            data->id_det = -1;
+            data->vbus_det = -1;
+            queue_delayed_work(system_wq, &data->detect, 0);
+        } else {
+            /* Host only mode */
+            sun4i_usb_phy0_set_id_detect(_phy, 0);
+            sun4i_usb_phy0_set_vbus_detect(_phy, 1);
+        }
+    }
+
      return 0;
  }
 
  static int sun4i_usb_phy_exit(struct phy *_phy)
  {
      struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (phy->index == 0) {
+        /* Disable pull-ups */
+        sun4i_usb_phy0_update_iscr(_phy, ISCR_DPDM_PULLUP_EN, 0);
+        sun4i_usb_phy0_update_iscr(_phy, ISCR_ID_PULLUP_EN, 0);
+        data->phy0_init = false;
+    }
 
      sun4i_usb_phy_passby(phy, 0);
      reset_control_assert(phy->reset);

@@ -188,20 +284,46 @@ static int sun4i_usb_phy_exit(struct phy *_phy)
  static int sun4i_usb_phy_power_on(struct phy *_phy)
  {
      struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
-    int ret = 0;
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    int ret;
+
+    if (!phy->vbus || phy->regulator_on)
+        return 0;
+
+    /* For phy0 only turn on Vbus if we don't have an ext. Vbus */
+    if (phy->index == 0 && data->vbus_det)
+        return 0;
 
-    if (phy->vbus)
-        ret = regulator_enable(phy->vbus);
+    ret = regulator_enable(phy->vbus);
+    if (ret)
+        return ret;
 
-    return ret;
+    phy->regulator_on = true;
+
+    /* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
+    if (phy->index == 0 && data->phy0_poll)
+        mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+    return 0;
  }
 
  static int sun4i_usb_phy_power_off(struct phy *_phy)
  {
      struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (!phy->vbus || !phy->regulator_on)
+        return 0;
 
-    if (phy->vbus)
-        regulator_disable(phy->vbus);
+    regulator_disable(phy->vbus);
+    phy->regulator_on = false;
+
+    /*
+     * phy0 vbus typically slowly discharges, sometimes this causes the
+     * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan.
+     */
+    if (phy->index == 0 && !data->phy0_poll)
+        mod_delayed_work(system_wq, &data->detect, POLL_TIME);
 
      return 0;
  }

@@ -221,6 +343,61 @@ static struct phy_ops sun4i_usb_phy_ops = {
      .owner        = THIS_MODULE,
  };
 
+static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
+{
+    struct sun4i_usb_phy_data *data =
+        container_of(work, struct sun4i_usb_phy_data, detect.work);
+    struct phy *phy0 = data->phys[0].phy;
+    int id_det, vbus_det, id_notify = 0, vbus_notify = 0;
+
+    id_det = gpiod_get_value_cansleep(data->id_det_gpio);
+    vbus_det = gpiod_get_value_cansleep(data->vbus_det_gpio);
+
+    mutex_lock(&phy0->mutex);
+
+    if (!data->phy0_init) {
+        mutex_unlock(&phy0->mutex);
+        return;
+    }
+
+    if (id_det != data->id_det) {
+        sun4i_usb_phy0_set_id_detect(phy0, id_det);
+        data->id_det = id_det;
+        id_notify = 1;
+    }
+
+    if (vbus_det != data->vbus_det) {
+        sun4i_usb_phy0_set_vbus_detect(phy0, vbus_det);
+        data->vbus_det = vbus_det;
+        vbus_notify = 1;
+    }
+
+    mutex_unlock(&phy0->mutex);
+
+    if (id_notify)
+        extcon_set_cable_state(&data->extcon,
+                       extcon_cable_name[EXTCON_USB_HOST],
+                       !id_det);
+
+    if (vbus_notify)
+        extcon_set_cable_state(&data->extcon,
+                       extcon_cable_name[EXTCON_USB],
+                       vbus_det);
+
+    if (data->phy0_poll)
+        queue_delayed_work(system_wq, &data->detect, POLL_TIME);
+}
+
+static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
+{
+    struct sun4i_usb_phy_data *data = dev_id;
+
+    /* vbus or id changed, let the pins settle and then scan them */
+    mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+    return IRQ_HANDLED;
+}
+
  static struct phy *sun4i_usb_phy_xlate(struct device *dev,
                      struct of_phandle_args *args)
  {

@@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
      struct phy_provider *phy_provider;
      bool dedicated_clocks;
      struct resource *res;
-    int i;
+    int i, ret;
 
      data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
      if (!data)
          return -ENOMEM;
 
      mutex_init(&data->mutex);
+    INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
+    data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST];
+    data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB];
+    data->extcon_cable_names[2] = NULL;
+    data->extcon.name = DRIVER_NAME;
+    data->extcon.supported_cable = data->extcon_cable_names;
+    data->extcon.dev.parent = dev;
 
      if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
          data->num_phys = 2;

@@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
      if (IS_ERR(data->base))
          return PTR_ERR(data->base);
 
+    data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN);
+    if (IS_ERR(data->id_det_gpio)) {
+        if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->id_det_gpio = NULL;
+    }
+
+    data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN);
+    if (IS_ERR(data->vbus_det_gpio)) {
+        if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->vbus_det_gpio = NULL;
+    }
+
+    /* We either want both gpio pins or neither (when in host mode) */
+    if (!data->id_det_gpio != !data->vbus_det_gpio) {
+        dev_err(dev, "failed to get id or vbus detect pin\n");
+        return -ENODEV;
+    }
+
+    if (data->id_det_gpio) {
+        ret = devm_extcon_dev_register(dev, &data->extcon);
+        if (ret) {
+            dev_err(dev, "failed to register extcon: %d\n", ret);
+            return ret;
+        }
+    }
+
      for (i = 0; i < data->num_phys; i++) {
          struct sun4i_usb_phy *phy = data->phys + i;
          char name[16];

@@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
          phy_set_drvdata(phy->phy, &data->phys[i]);
      }
 
+    data->id_det_irq = gpiod_to_irq(data->id_det_gpio);
+    data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
+    if (data->id_det_irq  < 0 || data->vbus_det_irq < 0)
+        data->phy0_poll = true;
+
+    if (data->id_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->id_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-id-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting id-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
+    if (data->vbus_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->vbus_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-vbus-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
      dev_set_drvdata(dev, data);
      phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
 
      return PTR_ERR_OR_ZERO(phy_provider);
  }
 
+static int sun4i_usb_phy_remove(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (data->id_det_irq >= 0)
+        devm_free_irq(dev, data->id_det_irq, data);
+    if (data->vbus_det_irq >= 0)
+        devm_free_irq(dev, data->vbus_det_irq, data);
+
+    cancel_delayed_work_sync(&data->detect);
+
+    return 0;
+}
+
  static const struct of_device_id sun4i_usb_phy_of_match[] = {
      { .compatible = "allwinner,sun4i-a10-usb-phy" },
      { .compatible = "allwinner,sun5i-a13-usb-phy" },

@@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
 
  static struct platform_driver sun4i_usb_phy_driver = {
      .probe    = sun4i_usb_phy_probe,
+    .remove    = sun4i_usb_phy_remove,
      .driver = {
          .of_match_table    = sun4i_usb_phy_of_match,
-        .name  = "sun4i-usb-phy",
+        .name  = DRIVER_NAME,
      }
  };
  module_platform_driver(sun4i_usb_phy_driver);

Hi,

Quoted 76 lines · Kishon Vijay Abraham I

+Chanwoo

Hi,

Quoted 71 lines · Hans de Goede

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:

(quoted diff) +0 -2

-Do not call regulator_disable in an unbalanced manner when an external vbus
  is present
---
  .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
  drivers/phy/Kconfig                                |   1 +
  drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
  3 files changed, 281 insertions(+), 11 deletions(-)

Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt +16 -2

index 16528b9..557fa99 100644
@@ -23,6 +23,13 @@ Required properties:
    * "usb1_reset"
    * "usb2_reset" for sun4i, sun6i or sun7i
 
+Optional properties:
+- usb0_id_det-gpios : gpio phandle for reading the otg id pin value
+- usb0_vbus_det-gpios : gpio phandle for detecting the presence of usb0 vbus
+- usb0_vbus-supply : regulator phandle for controller usb0 vbus
+- usb1_vbus-supply : regulator phandle for controller usb1 vbus
+- usb2_vbus-supply : regulator phandle for controller usb2 vbus
+
  Example:
      usbphy: phy at 0x01c13400 {
          #phy-cells = <1>;

@@ -32,6 +39,13 @@ Example:
          reg-names = "phy_ctrl", "pmu1", "pmu2";
          clocks = <&usb_clk 8>;
          clock-names = "usb_phy";
-        resets = <&usb_clk 1>, <&usb_clk 2>;
-        reset-names = "usb1_reset", "usb2_reset";
+        resets = <&usb_clk 0>, <&usb_clk 1>, <&usb_clk 2>;
+        reset-names = "usb0_reset", "usb1_reset", "usb2_reset";
+        pinctrl-names = "default";
+        pinctrl-0 = <&usb0_id_detect_pin>, <&usb0_vbus_detect_pin>;
+        usb0_id_det-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>; /* PH19 */
+        usb0_vbus_det-gpios = <&pio 7 22 GPIO_ACTIVE_HIGH>; /* PH22 */
+        usb0_vbus-supply = <&reg_usb0_vbus>;
+        usb1_vbus-supply = <&reg_usb1_vbus>;
+        usb2_vbus-supply = <&reg_usb2_vbus>;
      };

drivers/phy/Kconfig +0 -1

index a53bd5b..4614fba 100644
--- a/drivers/phy/Kconfig

Quoted 8 lines

drivers/phy/Kconfig config PHY_SUN4I_USB · line 176

Quoted 6 lines

+++ b/drivers/phy/Kconfig

drivers/phy/Kconfig +1 -0

@@ -173,6 +173,7 @@ config PHY_SUN4I_USB
      tristate "Allwinner sunxi SoC USB PHY driver"
      depends on ARCH_SUNXI && HAS_IOMEM && OF
      depends on RESET_CONTROLLER
+    select EXTCON

Avoid using 'select' on visible Kconfig symbols.

Ok, I'll do a v5 changing this to a depends on.

Quoted 1 line

Also please split the patch to make the reviewing a bit easier.

There really is not much which can be split out, the iscr register updating is only done from the id / vbus det scanning, so the 2 are intertwined with each other.

Also I must say I'm unhappy with the fact that you request this now, after this patch has been posted in its current form a long time ago already. The changes in v3 and v4 were very minimal, the patch has existed in this for since v2, so you've had a long long time to request changes like this already.

In the mean time I've build a whole set of related changes (enabling the use of otg on newer versions of the sunxi soc), so sorry I'm not going to split this up now as that is going to be a big pain with all the extra patches which are sitting on top, and as said there really is not that much to split in the first place.

I'll send out a v5 of this patch together with a resend of all the patches adding support for new models which sit on top of this one (and which I've already send before).

+Chanwoo

Hi,

Quoted 73 lines · Hans de Goede

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:

(quoted diff) +0 -2

-Do not call regulator_disable in an unbalanced manner when an external vbus
  is present
---
  .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
  drivers/phy/Kconfig                                |   1 +
  drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
  3 files changed, 281 insertions(+), 11 deletions(-)

Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt +16 -2

index 16528b9..557fa99 100644
@@ -23,6 +23,13 @@ Required properties:
    * "usb1_reset"
    * "usb2_reset" for sun4i, sun6i or sun7i
 
+Optional properties:
+- usb0_id_det-gpios : gpio phandle for reading the otg id pin value
+- usb0_vbus_det-gpios : gpio phandle for detecting the presence of usb0 vbus
+- usb0_vbus-supply : regulator phandle for controller usb0 vbus
+- usb1_vbus-supply : regulator phandle for controller usb1 vbus
+- usb2_vbus-supply : regulator phandle for controller usb2 vbus
+
  Example:
      usbphy: phy at 0x01c13400 {
          #phy-cells = <1>;

@@ -32,6 +39,13 @@ Example:
          reg-names = "phy_ctrl", "pmu1", "pmu2";
          clocks = <&usb_clk 8>;
          clock-names = "usb_phy";
-        resets = <&usb_clk 1>, <&usb_clk 2>;
-        reset-names = "usb1_reset", "usb2_reset";
+        resets = <&usb_clk 0>, <&usb_clk 1>, <&usb_clk 2>;
+        reset-names = "usb0_reset", "usb1_reset", "usb2_reset";
+        pinctrl-names = "default";
+        pinctrl-0 = <&usb0_id_detect_pin>, <&usb0_vbus_detect_pin>;
+        usb0_id_det-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>; /* PH19 */
+        usb0_vbus_det-gpios = <&pio 7 22 GPIO_ACTIVE_HIGH>; /* PH22 */
+        usb0_vbus-supply = <&reg_usb0_vbus>;
+        usb1_vbus-supply = <&reg_usb1_vbus>;
+        usb2_vbus-supply = <&reg_usb2_vbus>;
      };

drivers/phy/Kconfig +0 -0

index a53bd5b..4614fba 100644

drivers/phy/Kconfig config PHY_SUN4I_USB · line 176

Quoted 4 lines

drivers/phy/Kconfig +1 -0

@@ -173,6 +173,7 @@ config PHY_SUN4I_USB
      tristate "Allwinner sunxi SoC USB PHY driver"
      depends on ARCH_SUNXI && HAS_IOMEM && OF
      depends on RESET_CONTROLLER
+    select EXTCON

Avoid using 'select' on visible Kconfig symbols.

Also please split the patch to make the reviewing a bit easier.

Thanks
Kishon

Quoted 74 lines

drivers/phy/Kconfig +0 -0

      select GENERIC_PHY
      help
        Enable this to support the transceiver that is part of Allwinner

drivers/phy/phy-sun4i-usb.c +25 -1

index 91c5be4..b45d707 100644
@@ -1,7 +1,7 @@
  /*
   * Allwinner sun4i USB phy driver
   *
- * Copyright (C) 2014 Hans de Goede <[email protected]>
+ * Copyright (C) 2014-2015 Hans de Goede <[email protected]>
   *
   * Based on code from
   * Allwinner Technology Co., Ltd. <www.allwinnertech.com>

@@ -23,17 +23,23 @@
 
  #include <linux/clk.h>
  #include <linux/err.h>
+#include <linux/extcon.h>
  #include <linux/io.h>
+#include <linux/interrupt.h>
  #include <linux/kernel.h>
  #include <linux/module.h>
  #include <linux/mutex.h>
  #include <linux/of.h>
  #include <linux/of_address.h>
+#include <linux/of_gpio.h>
  #include <linux/phy/phy.h>
  #include <linux/phy/phy-sun4i-usb.h>
  #include <linux/platform_device.h>
  #include <linux/regulator/consumer.h>
  #include <linux/reset.h>
+#include <linux/workqueue.h>
+
+#define DRIVER_NAME "sun4i-usb-phy"
 
  #define REG_ISCR            0x00
  #define REG_PHYCTL            0x04

@@ -47,6 +53,17 @@
  #define SUNXI_AHB_INCRX_ALIGN_EN    BIT(8)
  #define SUNXI_ULPI_BYPASS_EN        BIT(0)
 
+/* ISCR, Interface Status and Control bits */
+#define ISCR_ID_PULLUP_EN        (1 << 17)
+#define ISCR_DPDM_PULLUP_EN    (1 << 16)
+/* sunxi has the phy id/vbus pins not connected, so we use the force bits */
+#define ISCR_FORCE_ID_MASK    (3 << 14)
+#define ISCR_FORCE_ID_LOW        (2 << 14)
+#define ISCR_FORCE_ID_HIGH    (3 << 14)
+#define ISCR_FORCE_VBUS_MASK    (3 << 12)
+#define ISCR_FORCE_VBUS_LOW    (2 << 12)
+#define ISCR_FORCE_VBUS_HIGH    (3 << 12)
+
  /* Common Control Bits for Both PHYs */
  #define PHY_PLL_BW            0x03
  #define PHY_RES45_CAL_EN        0x0c

@@ -63,6 +80,13 @@
 
  #define MAX_PHYS            3
 
+/*
+ * Note do not raise the debounce time, we must report Vusb high within 100ms
+ * otherwise we get Vbus errors
+ */
+#define DEBOUNCE_TIME            msecs_to_jiffies(50)
+#define POLL_TIME            msecs_to_jiffies(250)
+
  struct sun4i_usb_phy_data {
      void __iomem *base;
      struct mutex mutex;

@@ -74,13 +98,58 @@ struct sun4i_usb_phy_data {
          struct regulator *vbus;

drivers/phy/phy-sun4i-usb.c struct sun4i_usb_phy_data { · line 106

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c +4 -0

          struct reset_control *reset;
          struct clk *clk;
+        bool regulator_on;
          int index;
      } phys[MAX_PHYS];
+    /* phy0 / otg related variables */
+    struct extcon_dev extcon;
+    const char *extcon_cable_names[3];

Quoted 178 lines

drivers/phy/phy-sun4i-usb.c +132 -6

+    bool phy0_init;
+    bool phy0_poll;
+    struct gpio_desc *id_det_gpio;
+    struct gpio_desc *vbus_det_gpio;
+    int id_det_irq;
+    int vbus_det_irq;
+    int id_det;
+    int vbus_det;
+    struct delayed_work detect;
  };
 
  #define to_sun4i_usb_phy_data(phy) \
      container_of((phy), struct sun4i_usb_phy_data, phys[(phy)->index])
 
+static void sun4i_usb_phy0_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    u32 iscr;
+
+    iscr = readl(data->base + REG_ISCR);
+    iscr &= ~clr;
+    iscr |= set;
+    writel(iscr, data->base + REG_ISCR);
+}
+
+static void sun4i_usb_phy0_set_id_detect(struct phy *phy, u32 val)
+{
+    if (val)
+        val = ISCR_FORCE_ID_HIGH;
+    else
+        val = ISCR_FORCE_ID_LOW;
+
+    sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_ID_MASK, val);
+}
+
+static void sun4i_usb_phy0_set_vbus_detect(struct phy *phy, u32 val)
+{
+    if (val)
+        val = ISCR_FORCE_VBUS_HIGH;
+    else
+        val = ISCR_FORCE_VBUS_LOW;
+
+    sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_VBUS_MASK, val);
+}
+
  static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
                  int len)
  {

@@ -171,12 +240,39 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 
      sun4i_usb_phy_passby(phy, 1);
 
+    if (phy->index == 0) {
+        data->phy0_init = true;
+
+        /* Enable pull-ups */
+        sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_DPDM_PULLUP_EN);
+        sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_ID_PULLUP_EN);
+
+        if (data->id_det_gpio) {
+            /* OTG mode, force ISCR and cable state updates */
+            data->id_det = -1;
+            data->vbus_det = -1;
+            queue_delayed_work(system_wq, &data->detect, 0);
+        } else {
+            /* Host only mode */
+            sun4i_usb_phy0_set_id_detect(_phy, 0);
+            sun4i_usb_phy0_set_vbus_detect(_phy, 1);
+        }
+    }
+
      return 0;
  }
 
  static int sun4i_usb_phy_exit(struct phy *_phy)
  {
      struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (phy->index == 0) {
+        /* Disable pull-ups */
+        sun4i_usb_phy0_update_iscr(_phy, ISCR_DPDM_PULLUP_EN, 0);
+        sun4i_usb_phy0_update_iscr(_phy, ISCR_ID_PULLUP_EN, 0);
+        data->phy0_init = false;
+    }
 
      sun4i_usb_phy_passby(phy, 0);
      reset_control_assert(phy->reset);

@@ -188,20 +284,46 @@ static int sun4i_usb_phy_exit(struct phy *_phy)
  static int sun4i_usb_phy_power_on(struct phy *_phy)
  {
      struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
-    int ret = 0;
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    int ret;
+
+    if (!phy->vbus || phy->regulator_on)
+        return 0;
+
+    /* For phy0 only turn on Vbus if we don't have an ext. Vbus */
+    if (phy->index == 0 && data->vbus_det)
+        return 0;
 
-    if (phy->vbus)
-        ret = regulator_enable(phy->vbus);
+    ret = regulator_enable(phy->vbus);
+    if (ret)
+        return ret;
 
-    return ret;
+    phy->regulator_on = true;
+
+    /* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
+    if (phy->index == 0 && data->phy0_poll)
+        mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+    return 0;
  }
 
  static int sun4i_usb_phy_power_off(struct phy *_phy)
  {
      struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (!phy->vbus || !phy->regulator_on)
+        return 0;
 
-    if (phy->vbus)
-        regulator_disable(phy->vbus);
+    regulator_disable(phy->vbus);
+    phy->regulator_on = false;
+
+    /*
+     * phy0 vbus typically slowly discharges, sometimes this causes the
+     * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan.
+     */
+    if (phy->index == 0 && !data->phy0_poll)
+        mod_delayed_work(system_wq, &data->detect, POLL_TIME);
 
      return 0;
  }

@@ -221,6 +343,61 @@ static struct phy_ops sun4i_usb_phy_ops = {
      .owner        = THIS_MODULE,
  };
 
+static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
+{
+    struct sun4i_usb_phy_data *data =
+        container_of(work, struct sun4i_usb_phy_data, detect.work);
+    struct phy *phy0 = data->phys[0].phy;
+    int id_det, vbus_det, id_notify = 0, vbus_notify = 0;
+
+    id_det = gpiod_get_value_cansleep(data->id_det_gpio);
+    vbus_det = gpiod_get_value_cansleep(data->vbus_det_gpio);
+
+    mutex_lock(&phy0->mutex);
+
+    if (!data->phy0_init) {
+        mutex_unlock(&phy0->mutex);
+        return;
+    }
+
+    if (id_det != data->id_det) {
+        sun4i_usb_phy0_set_id_detect(phy0, id_det);
+        data->id_det = id_det;
+        id_notify = 1;
+    }
+
+    if (vbus_det != data->vbus_det) {
+        sun4i_usb_phy0_set_vbus_detect(phy0, vbus_det);
+        data->vbus_det = vbus_det;
+        vbus_notify = 1;
+    }
+
+    mutex_unlock(&phy0->mutex);
+
+    if (id_notify)

drivers/phy/phy-sun4i-usb.c static struct phy_ops sun4i_usb_phy_ops = { · line 385

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c +8 -0

+        extcon_set_cable_state(&data->extcon,
+                       extcon_cable_name[EXTCON_USB_HOST],
+                       !id_det);
+
+    if (vbus_notify)
+        extcon_set_cable_state(&data->extcon,
+                       extcon_cable_name[EXTCON_USB],
+                       vbus_det);

Quoted 29 lines

drivers/phy/phy-sun4i-usb.c +16 -1

+
+    if (data->phy0_poll)
+        queue_delayed_work(system_wq, &data->detect, POLL_TIME);
+}
+
+static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
+{
+    struct sun4i_usb_phy_data *data = dev_id;
+
+    /* vbus or id changed, let the pins settle and then scan them */
+    mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+    return IRQ_HANDLED;
+}
+
  static struct phy *sun4i_usb_phy_xlate(struct device *dev,
                      struct of_phandle_args *args)
  {

@@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
      struct phy_provider *phy_provider;
      bool dedicated_clocks;
      struct resource *res;
-    int i;
+    int i, ret;
 
      data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
      if (!data)
          return -ENOMEM;
 

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 431

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c +5 -0

      mutex_init(&data->mutex);
+    INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
+    data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST];
+    data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB];
+    data->extcon_cable_names[2] = NULL;
+    data->extcon.name = DRIVER_NAME;

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 432

Quoted 1 line

drivers/phy/phy-sun4i-usb.c +1 -0

+    data->extcon.supported_cable = data->extcon_cable_names;

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 433

Quoted 1 line

drivers/phy/phy-sun4i-usb.c +1 -0

+    data->extcon.dev.parent = dev;

Quoted 95 lines

drivers/phy/phy-sun4i-usb.c +70 -0

      if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
          data->num_phys = 2;

@@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
      if (IS_ERR(data->base))
          return PTR_ERR(data->base);
 
+    data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN);
+    if (IS_ERR(data->id_det_gpio)) {
+        if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->id_det_gpio = NULL;
+    }
+
+    data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN);
+    if (IS_ERR(data->vbus_det_gpio)) {
+        if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->vbus_det_gpio = NULL;
+    }
+
+    /* We either want both gpio pins or neither (when in host mode) */
+    if (!data->id_det_gpio != !data->vbus_det_gpio) {
+        dev_err(dev, "failed to get id or vbus detect pin\n");
+        return -ENODEV;
+    }
+
+    if (data->id_det_gpio) {
+        ret = devm_extcon_dev_register(dev, &data->extcon);
+        if (ret) {
+            dev_err(dev, "failed to register extcon: %d\n", ret);
+            return ret;
+        }
+    }
+
      for (i = 0; i < data->num_phys; i++) {
          struct sun4i_usb_phy *phy = data->phys + i;
          char name[16];

@@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
          phy_set_drvdata(phy->phy, &data->phys[i]);
      }
 
+    data->id_det_irq = gpiod_to_irq(data->id_det_gpio);
+    data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
+    if (data->id_det_irq  < 0 || data->vbus_det_irq < 0)
+        data->phy0_poll = true;
+
+    if (data->id_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->id_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-id-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting id-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
+    if (data->vbus_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->vbus_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-vbus-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
      dev_set_drvdata(dev, data);
      phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
 
      return PTR_ERR_OR_ZERO(phy_provider);
  }
 
+static int sun4i_usb_phy_remove(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (data->id_det_irq >= 0)
+        devm_free_irq(dev, data->id_det_irq, data);
+    if (data->vbus_det_irq >= 0)
+        devm_free_irq(dev, data->vbus_det_irq, data);
+
+    cancel_delayed_work_sync(&data->detect);
+
+    return 0;
+}
+
  static const struct of_device_id sun4i_usb_phy_of_match[] = {
      { .compatible = "allwinner,sun4i-a10-usb-phy" },
      { .compatible = "allwinner,sun5i-a13-usb-phy" },

@@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
 

drivers/phy/phy-sun4i-usb.c MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match); · line 598

Quoted 11 lines

drivers/phy/phy-sun4i-usb.c +2 -1

  static struct platform_driver sun4i_usb_phy_driver = {
      .probe    = sun4i_usb_phy_probe,
+    .remove    = sun4i_usb_phy_remove,
      .driver = {
          .of_match_table    = sun4i_usb_phy_of_match,
-        .name  = "sun4i-usb-phy",
+        .name  = DRIVER_NAME,
      }
  };
  module_platform_driver(sun4i_usb_phy_driver);

Hi Hans,

I add the comment about extcon-related code.

Firstly,
I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD

because there are both extcon
provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory.

Quoted 163 lines

+Chanwoo

Hi,

Quoted 73 lines · Hans de Goede

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:

(quoted diff) +0 -2

-Do not call regulator_disable in an unbalanced manner when an external vbus
   is present
---
   .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
   drivers/phy/Kconfig                                |   1 +
   drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
   3 files changed, 281 insertions(+), 11 deletions(-)

Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt +16 -2

index 16528b9..557fa99 100644
@@ -23,6 +23,13 @@ Required properties:
     * "usb1_reset"
     * "usb2_reset" for sun4i, sun6i or sun7i
 
+Optional properties:
+- usb0_id_det-gpios : gpio phandle for reading the otg id pin value
+- usb0_vbus_det-gpios : gpio phandle for detecting the presence of usb0 vbus
+- usb0_vbus-supply : regulator phandle for controller usb0 vbus
+- usb1_vbus-supply : regulator phandle for controller usb1 vbus
+- usb2_vbus-supply : regulator phandle for controller usb2 vbus
+
   Example:
       usbphy: phy at 0x01c13400 {
           #phy-cells = <1>;

@@ -32,6 +39,13 @@ Example:
           reg-names = "phy_ctrl", "pmu1", "pmu2";
           clocks = <&usb_clk 8>;
           clock-names = "usb_phy";
-        resets = <&usb_clk 1>, <&usb_clk 2>;
-        reset-names = "usb1_reset", "usb2_reset";
+        resets = <&usb_clk 0>, <&usb_clk 1>, <&usb_clk 2>;
+        reset-names = "usb0_reset", "usb1_reset", "usb2_reset";
+        pinctrl-names = "default";
+        pinctrl-0 = <&usb0_id_detect_pin>, <&usb0_vbus_detect_pin>;
+        usb0_id_det-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>; /* PH19 */
+        usb0_vbus_det-gpios = <&pio 7 22 GPIO_ACTIVE_HIGH>; /* PH22 */
+        usb0_vbus-supply = <&reg_usb0_vbus>;
+        usb1_vbus-supply = <&reg_usb1_vbus>;
+        usb2_vbus-supply = <&reg_usb2_vbus>;
       };

drivers/phy/Kconfig +0 -0

index a53bd5b..4614fba 100644

drivers/phy/Kconfig config PHY_SUN4I_USB · line 176

Quoted 4 lines

drivers/phy/Kconfig +1 -0

@@ -173,6 +173,7 @@ config PHY_SUN4I_USB
       tristate "Allwinner sunxi SoC USB PHY driver"
       depends on ARCH_SUNXI && HAS_IOMEM && OF
       depends on RESET_CONTROLLER
+    select EXTCON

Avoid using 'select' on visible Kconfig symbols.

Also please split the patch to make the reviewing a bit easier.

Thanks
Kishon

Quoted 74 lines

drivers/phy/Kconfig +0 -0

       select GENERIC_PHY
       help
         Enable this to support the transceiver that is part of Allwinner

drivers/phy/phy-sun4i-usb.c +25 -1

index 91c5be4..b45d707 100644
@@ -1,7 +1,7 @@
   /*
    * Allwinner sun4i USB phy driver
    *
- * Copyright (C) 2014 Hans de Goede <[email protected]>
+ * Copyright (C) 2014-2015 Hans de Goede <[email protected]>
    *
    * Based on code from
    * Allwinner Technology Co., Ltd. <www.allwinnertech.com>

@@ -23,17 +23,23 @@
 
   #include <linux/clk.h>
   #include <linux/err.h>
+#include <linux/extcon.h>
   #include <linux/io.h>
+#include <linux/interrupt.h>
   #include <linux/kernel.h>
   #include <linux/module.h>
   #include <linux/mutex.h>
   #include <linux/of.h>
   #include <linux/of_address.h>
+#include <linux/of_gpio.h>
   #include <linux/phy/phy.h>
   #include <linux/phy/phy-sun4i-usb.h>
   #include <linux/platform_device.h>
   #include <linux/regulator/consumer.h>
   #include <linux/reset.h>
+#include <linux/workqueue.h>
+
+#define DRIVER_NAME "sun4i-usb-phy"
 
   #define REG_ISCR            0x00
   #define REG_PHYCTL            0x04

@@ -47,6 +53,17 @@
   #define SUNXI_AHB_INCRX_ALIGN_EN    BIT(8)
   #define SUNXI_ULPI_BYPASS_EN        BIT(0)
 
+/* ISCR, Interface Status and Control bits */
+#define ISCR_ID_PULLUP_EN        (1 << 17)
+#define ISCR_DPDM_PULLUP_EN    (1 << 16)
+/* sunxi has the phy id/vbus pins not connected, so we use the force bits */
+#define ISCR_FORCE_ID_MASK    (3 << 14)
+#define ISCR_FORCE_ID_LOW        (2 << 14)
+#define ISCR_FORCE_ID_HIGH    (3 << 14)
+#define ISCR_FORCE_VBUS_MASK    (3 << 12)
+#define ISCR_FORCE_VBUS_LOW    (2 << 12)
+#define ISCR_FORCE_VBUS_HIGH    (3 << 12)
+
   /* Common Control Bits for Both PHYs */
   #define PHY_PLL_BW            0x03
   #define PHY_RES45_CAL_EN        0x0c

@@ -63,6 +80,13 @@
 
   #define MAX_PHYS            3
 
+/*
+ * Note do not raise the debounce time, we must report Vusb high within 100ms
+ * otherwise we get Vbus errors
+ */
+#define DEBOUNCE_TIME            msecs_to_jiffies(50)
+#define POLL_TIME            msecs_to_jiffies(250)
+
   struct sun4i_usb_phy_data {
       void __iomem *base;
       struct mutex mutex;

@@ -74,13 +98,58 @@ struct sun4i_usb_phy_data {
           struct regulator *vbus;

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c struct sun4i_usb_phy_data { · line 106

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c +4 -0

           struct reset_control *reset;
           struct clk *clk;
+        bool regulator_on;
           int index;
       } phys[MAX_PHYS];
+    /* phy0 / otg related variables */
+    struct extcon_dev extcon;
+    const char *extcon_cable_names[3];

The latest extcon use the unique id to indicate the each external connector instead of legacy string. The related patch[1] and patch[2] will be merged on v4.2-rc1.

[1] extcon: Use the unique id for external connector instead of string
- http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=2a9de9c0f08d61fbe3764a21d22d0b72df97d6ae
[2] extcon: Redefine the unique id of supported external connectors without 'enum extcon' type
- http://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extcon-next-v4.3&id=bbd8d8b4244bb2799143c959fde57b1f034ec838

For exmaple in drivers/extcon/extcon-usb-gpio.c:
Maybe you can add the similiar array which include the supported external connector.

static const unsigned int usb_extcon_cable[] = {
EXTCON_USB,
EXTCON_USB_HOST,
EXTCON_NONE,
};

Quoted 179 lines

Quoted 179 lines

drivers/phy/phy-sun4i-usb.c +132 -6

+    bool phy0_init;
+    bool phy0_poll;
+    struct gpio_desc *id_det_gpio;
+    struct gpio_desc *vbus_det_gpio;
+    int id_det_irq;
+    int vbus_det_irq;
+    int id_det;
+    int vbus_det;
+    struct delayed_work detect;
   };
 
   #define to_sun4i_usb_phy_data(phy) \
       container_of((phy), struct sun4i_usb_phy_data, phys[(phy)->index])
 
+static void sun4i_usb_phy0_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    u32 iscr;
+
+    iscr = readl(data->base + REG_ISCR);
+    iscr &= ~clr;
+    iscr |= set;
+    writel(iscr, data->base + REG_ISCR);
+}
+
+static void sun4i_usb_phy0_set_id_detect(struct phy *phy, u32 val)
+{
+    if (val)
+        val = ISCR_FORCE_ID_HIGH;
+    else
+        val = ISCR_FORCE_ID_LOW;
+
+    sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_ID_MASK, val);
+}
+
+static void sun4i_usb_phy0_set_vbus_detect(struct phy *phy, u32 val)
+{
+    if (val)
+        val = ISCR_FORCE_VBUS_HIGH;
+    else
+        val = ISCR_FORCE_VBUS_LOW;
+
+    sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_VBUS_MASK, val);
+}
+
   static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
                   int len)
   {

@@ -171,12 +240,39 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 
       sun4i_usb_phy_passby(phy, 1);
 
+    if (phy->index == 0) {
+        data->phy0_init = true;
+
+        /* Enable pull-ups */
+        sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_DPDM_PULLUP_EN);
+        sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_ID_PULLUP_EN);
+
+        if (data->id_det_gpio) {
+            /* OTG mode, force ISCR and cable state updates */
+            data->id_det = -1;
+            data->vbus_det = -1;
+            queue_delayed_work(system_wq, &data->detect, 0);
+        } else {
+            /* Host only mode */
+            sun4i_usb_phy0_set_id_detect(_phy, 0);
+            sun4i_usb_phy0_set_vbus_detect(_phy, 1);
+        }
+    }
+
       return 0;
   }
 
   static int sun4i_usb_phy_exit(struct phy *_phy)
   {
       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (phy->index == 0) {
+        /* Disable pull-ups */
+        sun4i_usb_phy0_update_iscr(_phy, ISCR_DPDM_PULLUP_EN, 0);
+        sun4i_usb_phy0_update_iscr(_phy, ISCR_ID_PULLUP_EN, 0);
+        data->phy0_init = false;
+    }
 
       sun4i_usb_phy_passby(phy, 0);
       reset_control_assert(phy->reset);

@@ -188,20 +284,46 @@ static int sun4i_usb_phy_exit(struct phy *_phy)
   static int sun4i_usb_phy_power_on(struct phy *_phy)
   {
       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
-    int ret = 0;
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    int ret;
+
+    if (!phy->vbus || phy->regulator_on)
+        return 0;
+
+    /* For phy0 only turn on Vbus if we don't have an ext. Vbus */
+    if (phy->index == 0 && data->vbus_det)
+        return 0;
 
-    if (phy->vbus)
-        ret = regulator_enable(phy->vbus);
+    ret = regulator_enable(phy->vbus);
+    if (ret)
+        return ret;
 
-    return ret;
+    phy->regulator_on = true;
+
+    /* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
+    if (phy->index == 0 && data->phy0_poll)
+        mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+    return 0;
   }
 
   static int sun4i_usb_phy_power_off(struct phy *_phy)
   {
       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (!phy->vbus || !phy->regulator_on)
+        return 0;
 
-    if (phy->vbus)
-        regulator_disable(phy->vbus);
+    regulator_disable(phy->vbus);
+    phy->regulator_on = false;
+
+    /*
+     * phy0 vbus typically slowly discharges, sometimes this causes the
+     * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan.
+     */
+    if (phy->index == 0 && !data->phy0_poll)
+        mod_delayed_work(system_wq, &data->detect, POLL_TIME);
 
       return 0;
   }

@@ -221,6 +343,61 @@ static struct phy_ops sun4i_usb_phy_ops = {
       .owner        = THIS_MODULE,
   };
 
+static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
+{
+    struct sun4i_usb_phy_data *data =
+        container_of(work, struct sun4i_usb_phy_data, detect.work);
+    struct phy *phy0 = data->phys[0].phy;
+    int id_det, vbus_det, id_notify = 0, vbus_notify = 0;
+
+    id_det = gpiod_get_value_cansleep(data->id_det_gpio);
+    vbus_det = gpiod_get_value_cansleep(data->vbus_det_gpio);
+
+    mutex_lock(&phy0->mutex);
+
+    if (!data->phy0_init) {
+        mutex_unlock(&phy0->mutex);
+        return;
+    }
+
+    if (id_det != data->id_det) {
+        sun4i_usb_phy0_set_id_detect(phy0, id_det);
+        data->id_det = id_det;
+        id_notify = 1;
+    }
+
+    if (vbus_det != data->vbus_det) {
+        sun4i_usb_phy0_set_vbus_detect(phy0, vbus_det);
+        data->vbus_det = vbus_det;
+        vbus_notify = 1;
+    }
+
+    mutex_unlock(&phy0->mutex);
+
+    if (id_notify)

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c static struct phy_ops sun4i_usb_phy_ops = { · line 385

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c +8 -0

+        extcon_set_cable_state(&data->extcon,
+                       extcon_cable_name[EXTCON_USB_HOST],
+                       !id_det);
+
+    if (vbus_notify)
+        extcon_set_cable_state(&data->extcon,
+                       extcon_cable_name[EXTCON_USB],
+                       vbus_det);

I don't want to use the legacy extcon API with string name. Instead, you can use the recommeded extcon_set_cable_state_() API as following:

- Legacy extcon API
: extern int extcon_set_cable_state(struct extcon_dev *edev,

				  const char *cable_name, bool cable_state);
- Recommended extcon API
: extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
				   bool cable_state);

[1] extcon: Use the unique id for external connector instead of string
- http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=2a9de9c0f08d61fbe3764a21d22d0b72df97d6ae

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-sun4i-usb.c +16 -1

+
+    if (data->phy0_poll)
+        queue_delayed_work(system_wq, &data->detect, POLL_TIME);
+}
+
+static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
+{
+    struct sun4i_usb_phy_data *data = dev_id;
+
+    /* vbus or id changed, let the pins settle and then scan them */
+    mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+    return IRQ_HANDLED;
+}
+
   static struct phy *sun4i_usb_phy_xlate(struct device *dev,
                       struct of_phandle_args *args)
   {

@@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
       struct phy_provider *phy_provider;
       bool dedicated_clocks;
       struct resource *res;
-    int i;
+    int i, ret;
 
       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
       if (!data)
           return -ENOMEM;
 

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 431

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c +5 -0

       mutex_init(&data->mutex);
+    INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
+    data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST];
+    data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB];
+    data->extcon_cable_names[2] = NULL;
+    data->extcon.name = DRIVER_NAME;

Don't need it because extcon_dev_register() set the device name automatically.
You can check it on patch[3].
[3] http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=71c3ffa5d23af0554c27010cf12710da9bf85950

Quoted 1 line

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 432

Quoted 1 line

drivers/phy/phy-sun4i-usb.c +1 -0

+    data->extcon.supported_cable = data->extcon_cable_names;

You can use the devm_extcon_dev_allocate() to set the supported external connectors.

Quoted 1 line

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 433

Quoted 1 line

drivers/phy/phy-sun4i-usb.c +1 -0

+    data->extcon.dev.parent = dev;

Don't need it because extcon_dev_register() set the parent device automatically.

The field of 'struct extcon_dev' shold be filled with extcon API without direct allocation.

Quoted 95 lines

Quoted 95 lines

drivers/phy/phy-sun4i-usb.c +70 -0

       if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
           data->num_phys = 2;

@@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
       if (IS_ERR(data->base))
           return PTR_ERR(data->base);
 
+    data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN);
+    if (IS_ERR(data->id_det_gpio)) {
+        if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->id_det_gpio = NULL;
+    }
+
+    data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN);
+    if (IS_ERR(data->vbus_det_gpio)) {
+        if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->vbus_det_gpio = NULL;
+    }
+
+    /* We either want both gpio pins or neither (when in host mode) */
+    if (!data->id_det_gpio != !data->vbus_det_gpio) {
+        dev_err(dev, "failed to get id or vbus detect pin\n");
+        return -ENODEV;
+    }
+
+    if (data->id_det_gpio) {
+        ret = devm_extcon_dev_register(dev, &data->extcon);
+        if (ret) {
+            dev_err(dev, "failed to register extcon: %d\n", ret);
+            return ret;
+        }
+    }
+
       for (i = 0; i < data->num_phys; i++) {
           struct sun4i_usb_phy *phy = data->phys + i;
           char name[16];

@@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
           phy_set_drvdata(phy->phy, &data->phys[i]);
       }
 
+    data->id_det_irq = gpiod_to_irq(data->id_det_gpio);
+    data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
+    if (data->id_det_irq  < 0 || data->vbus_det_irq < 0)
+        data->phy0_poll = true;
+
+    if (data->id_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->id_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-id-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting id-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
+    if (data->vbus_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->vbus_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-vbus-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
       dev_set_drvdata(dev, data);
       phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
 
       return PTR_ERR_OR_ZERO(phy_provider);
   }
 
+static int sun4i_usb_phy_remove(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (data->id_det_irq >= 0)
+        devm_free_irq(dev, data->id_det_irq, data);
+    if (data->vbus_det_irq >= 0)
+        devm_free_irq(dev, data->vbus_det_irq, data);
+
+    cancel_delayed_work_sync(&data->detect);
+
+    return 0;
+}
+
   static const struct of_device_id sun4i_usb_phy_of_match[] = {
       { .compatible = "allwinner,sun4i-a10-usb-phy" },
       { .compatible = "allwinner,sun5i-a13-usb-phy" },

@@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
 

Quoted 12 lines

drivers/phy/phy-sun4i-usb.c MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match); · line 598

Quoted 11 lines

drivers/phy/phy-sun4i-usb.c +2 -1

   static struct platform_driver sun4i_usb_phy_driver = {
       .probe    = sun4i_usb_phy_probe,
+    .remove    = sun4i_usb_phy_remove,
       .driver = {
           .of_match_table    = sun4i_usb_phy_of_match,
-        .name  = "sun4i-usb-phy",
+        .name  = DRIVER_NAME,
       }
   };
   module_platform_driver(sun4i_usb_phy_driver);

Thanks,
Chanwoo Choi

Hi Chanwoo,

Thanks for the quick review.

Quoted 7 lines · Chanwoo Choi

Hi Hans,

I add the comment about extcon-related code.

Firstly,
I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD

No, just no, this is not what the MFD framework is for, the usb-phy in question here is not a multifunction device. The MFD framework is intended for true multi-function devices like i2c attached PMICs which have regulators, gpios, pwm, input (power button), chargers, power-supply, etc. That is NOT the case here.

Also moving this to the MFD framework would very likely requiring the devicetree binding for the usb-phy to change which we cannot do as that would break the devicetree ABI.

Quoted 5 lines

because there are both extcon
provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory.

I do not find this a compelling reason, there are plenty of subsystems where not all implementations of the subsystem class live in the subsystem directory, e.g. input and hwmon devices are often also found outside of the input and hwmon driver directories.

Quoted 183 lines · Kishon Vijay Abraham I

Quoted 163 lines

+Chanwoo

Hi,

Quoted 73 lines · Hans de Goede

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:

(quoted diff) +0 -2

-Do not call regulator_disable in an unbalanced manner when an external vbus
   is present
---
   .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
   drivers/phy/Kconfig                                |   1 +
   drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
   3 files changed, 281 insertions(+), 11 deletions(-)

Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt +16 -2

index 16528b9..557fa99 100644
@@ -23,6 +23,13 @@ Required properties:
     * "usb1_reset"
     * "usb2_reset" for sun4i, sun6i or sun7i
 
+Optional properties:
+- usb0_id_det-gpios : gpio phandle for reading the otg id pin value
+- usb0_vbus_det-gpios : gpio phandle for detecting the presence of usb0 vbus
+- usb0_vbus-supply : regulator phandle for controller usb0 vbus
+- usb1_vbus-supply : regulator phandle for controller usb1 vbus
+- usb2_vbus-supply : regulator phandle for controller usb2 vbus
+
   Example:
       usbphy: phy at 0x01c13400 {
           #phy-cells = <1>;

@@ -32,6 +39,13 @@ Example:
           reg-names = "phy_ctrl", "pmu1", "pmu2";
           clocks = <&usb_clk 8>;
           clock-names = "usb_phy";
-        resets = <&usb_clk 1>, <&usb_clk 2>;
-        reset-names = "usb1_reset", "usb2_reset";
+        resets = <&usb_clk 0>, <&usb_clk 1>, <&usb_clk 2>;
+        reset-names = "usb0_reset", "usb1_reset", "usb2_reset";
+        pinctrl-names = "default";
+        pinctrl-0 = <&usb0_id_detect_pin>, <&usb0_vbus_detect_pin>;
+        usb0_id_det-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>; /* PH19 */
+        usb0_vbus_det-gpios = <&pio 7 22 GPIO_ACTIVE_HIGH>; /* PH22 */
+        usb0_vbus-supply = <&reg_usb0_vbus>;
+        usb1_vbus-supply = <&reg_usb1_vbus>;
+        usb2_vbus-supply = <&reg_usb2_vbus>;
       };

drivers/phy/Kconfig +0 -0

index a53bd5b..4614fba 100644

drivers/phy/Kconfig config PHY_SUN4I_USB · line 176

Quoted 4 lines

drivers/phy/Kconfig +1 -0

@@ -173,6 +173,7 @@ config PHY_SUN4I_USB
       tristate "Allwinner sunxi SoC USB PHY driver"
       depends on ARCH_SUNXI && HAS_IOMEM && OF
       depends on RESET_CONTROLLER
+    select EXTCON

Avoid using 'select' on visible Kconfig symbols.

Also please split the patch to make the reviewing a bit easier.

Thanks
Kishon

Quoted 74 lines

drivers/phy/Kconfig +0 -0

       select GENERIC_PHY
       help
         Enable this to support the transceiver that is part of Allwinner

drivers/phy/phy-sun4i-usb.c +25 -1

index 91c5be4..b45d707 100644
@@ -1,7 +1,7 @@
   /*
    * Allwinner sun4i USB phy driver
    *
- * Copyright (C) 2014 Hans de Goede <[email protected]>
+ * Copyright (C) 2014-2015 Hans de Goede <[email protected]>
    *
    * Based on code from
    * Allwinner Technology Co., Ltd. <www.allwinnertech.com>

@@ -23,17 +23,23 @@
 
   #include <linux/clk.h>
   #include <linux/err.h>
+#include <linux/extcon.h>
   #include <linux/io.h>
+#include <linux/interrupt.h>
   #include <linux/kernel.h>
   #include <linux/module.h>
   #include <linux/mutex.h>
   #include <linux/of.h>
   #include <linux/of_address.h>
+#include <linux/of_gpio.h>
   #include <linux/phy/phy.h>
   #include <linux/phy/phy-sun4i-usb.h>
   #include <linux/platform_device.h>
   #include <linux/regulator/consumer.h>
   #include <linux/reset.h>
+#include <linux/workqueue.h>
+
+#define DRIVER_NAME "sun4i-usb-phy"
 
   #define REG_ISCR            0x00
   #define REG_PHYCTL            0x04

@@ -47,6 +53,17 @@
   #define SUNXI_AHB_INCRX_ALIGN_EN    BIT(8)
   #define SUNXI_ULPI_BYPASS_EN        BIT(0)
 
+/* ISCR, Interface Status and Control bits */
+#define ISCR_ID_PULLUP_EN        (1 << 17)
+#define ISCR_DPDM_PULLUP_EN    (1 << 16)
+/* sunxi has the phy id/vbus pins not connected, so we use the force bits */
+#define ISCR_FORCE_ID_MASK    (3 << 14)
+#define ISCR_FORCE_ID_LOW        (2 << 14)
+#define ISCR_FORCE_ID_HIGH    (3 << 14)
+#define ISCR_FORCE_VBUS_MASK    (3 << 12)
+#define ISCR_FORCE_VBUS_LOW    (2 << 12)
+#define ISCR_FORCE_VBUS_HIGH    (3 << 12)
+
   /* Common Control Bits for Both PHYs */
   #define PHY_PLL_BW            0x03
   #define PHY_RES45_CAL_EN        0x0c

@@ -63,6 +80,13 @@
 
   #define MAX_PHYS            3
 
+/*
+ * Note do not raise the debounce time, we must report Vusb high within 100ms
+ * otherwise we get Vbus errors
+ */
+#define DEBOUNCE_TIME            msecs_to_jiffies(50)
+#define POLL_TIME            msecs_to_jiffies(250)
+
   struct sun4i_usb_phy_data {
       void __iomem *base;
       struct mutex mutex;

@@ -74,13 +98,58 @@ struct sun4i_usb_phy_data {
           struct regulator *vbus;

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c struct sun4i_usb_phy_data { · line 106

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c +4 -0

           struct reset_control *reset;
           struct clk *clk;
+        bool regulator_on;
           int index;
       } phys[MAX_PHYS];
+    /* phy0 / otg related variables */
+    struct extcon_dev extcon;
+    const char *extcon_cable_names[3];

The latest extcon use the unique id to indicate the each external connector instead of legacy string. The related patch[1] and patch[2] will be merged on v4.2-rc1.

[1] extcon: Use the unique id for external connector instead of string
- http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=2a9de9c0f08d61fbe3764a21d22d0b72df97d6ae
[2] extcon: Redefine the unique id of supported external connectors without 'enum extcon' type
- http://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extcon-next-v4.3&id=bbd8d8b4244bb2799143c959fde57b1f034ec838

Quoted 8 lines

For exmaple in drivers/extcon/extcon-usb-gpio.c:
Maybe you can add the similiar array which include the supported external connector.

static const unsigned int usb_extcon_cable[] = {
EXTCON_USB,
EXTCON_USB_HOST,
EXTCON_NONE,
};

Ah, that is good to know, I'll port the extcon code in phy-sun4i-usb.c to this new API for the next version of the patch.

Thanks & Regards,

Hans

Quoted 362 lines

Quoted 179 lines

Quoted 179 lines

drivers/phy/phy-sun4i-usb.c +132 -6

+    bool phy0_init;
+    bool phy0_poll;
+    struct gpio_desc *id_det_gpio;
+    struct gpio_desc *vbus_det_gpio;
+    int id_det_irq;
+    int vbus_det_irq;
+    int id_det;
+    int vbus_det;
+    struct delayed_work detect;
   };
 
   #define to_sun4i_usb_phy_data(phy) \
       container_of((phy), struct sun4i_usb_phy_data, phys[(phy)->index])
 
+static void sun4i_usb_phy0_update_iscr(struct phy *_phy, u32 clr, u32 set)
+{
+    struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    u32 iscr;
+
+    iscr = readl(data->base + REG_ISCR);
+    iscr &= ~clr;
+    iscr |= set;
+    writel(iscr, data->base + REG_ISCR);
+}
+
+static void sun4i_usb_phy0_set_id_detect(struct phy *phy, u32 val)
+{
+    if (val)
+        val = ISCR_FORCE_ID_HIGH;
+    else
+        val = ISCR_FORCE_ID_LOW;
+
+    sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_ID_MASK, val);
+}
+
+static void sun4i_usb_phy0_set_vbus_detect(struct phy *phy, u32 val)
+{
+    if (val)
+        val = ISCR_FORCE_VBUS_HIGH;
+    else
+        val = ISCR_FORCE_VBUS_LOW;
+
+    sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_VBUS_MASK, val);
+}
+
   static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data,
                   int len)
   {

@@ -171,12 +240,39 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 
       sun4i_usb_phy_passby(phy, 1);
 
+    if (phy->index == 0) {
+        data->phy0_init = true;
+
+        /* Enable pull-ups */
+        sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_DPDM_PULLUP_EN);
+        sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_ID_PULLUP_EN);
+
+        if (data->id_det_gpio) {
+            /* OTG mode, force ISCR and cable state updates */
+            data->id_det = -1;
+            data->vbus_det = -1;
+            queue_delayed_work(system_wq, &data->detect, 0);
+        } else {
+            /* Host only mode */
+            sun4i_usb_phy0_set_id_detect(_phy, 0);
+            sun4i_usb_phy0_set_vbus_detect(_phy, 1);
+        }
+    }
+
       return 0;
   }
 
   static int sun4i_usb_phy_exit(struct phy *_phy)
   {
       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (phy->index == 0) {
+        /* Disable pull-ups */
+        sun4i_usb_phy0_update_iscr(_phy, ISCR_DPDM_PULLUP_EN, 0);
+        sun4i_usb_phy0_update_iscr(_phy, ISCR_ID_PULLUP_EN, 0);
+        data->phy0_init = false;
+    }
 
       sun4i_usb_phy_passby(phy, 0);
       reset_control_assert(phy->reset);

@@ -188,20 +284,46 @@ static int sun4i_usb_phy_exit(struct phy *_phy)
   static int sun4i_usb_phy_power_on(struct phy *_phy)
   {
       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
-    int ret = 0;
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+    int ret;
+
+    if (!phy->vbus || phy->regulator_on)
+        return 0;
+
+    /* For phy0 only turn on Vbus if we don't have an ext. Vbus */
+    if (phy->index == 0 && data->vbus_det)
+        return 0;
 
-    if (phy->vbus)
-        ret = regulator_enable(phy->vbus);
+    ret = regulator_enable(phy->vbus);
+    if (ret)
+        return ret;
 
-    return ret;
+    phy->regulator_on = true;
+
+    /* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */
+    if (phy->index == 0 && data->phy0_poll)
+        mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+    return 0;
   }
 
   static int sun4i_usb_phy_power_off(struct phy *_phy)
   {
       struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
+    struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
+
+    if (!phy->vbus || !phy->regulator_on)
+        return 0;
 
-    if (phy->vbus)
-        regulator_disable(phy->vbus);
+    regulator_disable(phy->vbus);
+    phy->regulator_on = false;
+
+    /*
+     * phy0 vbus typically slowly discharges, sometimes this causes the
+     * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan.
+     */
+    if (phy->index == 0 && !data->phy0_poll)
+        mod_delayed_work(system_wq, &data->detect, POLL_TIME);
 
       return 0;
   }

@@ -221,6 +343,61 @@ static struct phy_ops sun4i_usb_phy_ops = {
       .owner        = THIS_MODULE,
   };
 
+static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work)
+{
+    struct sun4i_usb_phy_data *data =
+        container_of(work, struct sun4i_usb_phy_data, detect.work);
+    struct phy *phy0 = data->phys[0].phy;
+    int id_det, vbus_det, id_notify = 0, vbus_notify = 0;
+
+    id_det = gpiod_get_value_cansleep(data->id_det_gpio);
+    vbus_det = gpiod_get_value_cansleep(data->vbus_det_gpio);
+
+    mutex_lock(&phy0->mutex);
+
+    if (!data->phy0_init) {
+        mutex_unlock(&phy0->mutex);
+        return;
+    }
+
+    if (id_det != data->id_det) {
+        sun4i_usb_phy0_set_id_detect(phy0, id_det);
+        data->id_det = id_det;
+        id_notify = 1;
+    }
+
+    if (vbus_det != data->vbus_det) {
+        sun4i_usb_phy0_set_vbus_detect(phy0, vbus_det);
+        data->vbus_det = vbus_det;
+        vbus_notify = 1;
+    }
+
+    mutex_unlock(&phy0->mutex);
+
+    if (id_notify)

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c static struct phy_ops sun4i_usb_phy_ops = { · line 385

Quoted 8 lines

drivers/phy/phy-sun4i-usb.c +8 -0

+        extcon_set_cable_state(&data->extcon,
+                       extcon_cable_name[EXTCON_USB_HOST],
+                       !id_det);
+
+    if (vbus_notify)
+        extcon_set_cable_state(&data->extcon,
+                       extcon_cable_name[EXTCON_USB],
+                       vbus_det);

I don't want to use the legacy extcon API with string name. Instead, you can use the recommeded extcon_set_cable_state_() API as following:

- Legacy extcon API
: extern int extcon_set_cable_state(struct extcon_dev *edev,

                  const char *cable_name, bool cable_state);
- Recommended extcon API
: extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id,
                   bool cable_state);

[1] extcon: Use the unique id for external connector instead of string
- http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=2a9de9c0f08d61fbe3764a21d22d0b72df97d6ae

Quoted 29 lines

Quoted 29 lines

drivers/phy/phy-sun4i-usb.c +16 -1

+
+    if (data->phy0_poll)
+        queue_delayed_work(system_wq, &data->detect, POLL_TIME);
+}
+
+static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id)
+{
+    struct sun4i_usb_phy_data *data = dev_id;
+
+    /* vbus or id changed, let the pins settle and then scan them */
+    mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME);
+
+    return IRQ_HANDLED;
+}
+
   static struct phy *sun4i_usb_phy_xlate(struct device *dev,
                       struct of_phandle_args *args)
   {

@@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
       struct phy_provider *phy_provider;
       bool dedicated_clocks;
       struct resource *res;
-    int i;
+    int i, ret;
 
       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
       if (!data)
           return -ENOMEM;
 

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 431

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c +5 -0

       mutex_init(&data->mutex);
+    INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
+    data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST];
+    data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB];
+    data->extcon_cable_names[2] = NULL;
+    data->extcon.name = DRIVER_NAME;

Don't need it because extcon_dev_register() set the device name automatically.
You can check it on patch[3].
[3] http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=71c3ffa5d23af0554c27010cf12710da9bf85950

Quoted 1 line

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 432

Quoted 1 line

drivers/phy/phy-sun4i-usb.c +1 -0

+    data->extcon.supported_cable = data->extcon_cable_names;

You can use the devm_extcon_dev_allocate() to set the supported external connectors.

Quoted 1 line

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 433

Quoted 1 line

drivers/phy/phy-sun4i-usb.c +1 -0

+    data->extcon.dev.parent = dev;

Don't need it because extcon_dev_register() set the parent device automatically.

The field of 'struct extcon_dev' shold be filled with extcon API without direct allocation.

Quoted 95 lines

Quoted 95 lines

drivers/phy/phy-sun4i-usb.c +70 -0

       if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
           data->num_phys = 2;

@@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
       if (IS_ERR(data->base))
           return PTR_ERR(data->base);
 
+    data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN);
+    if (IS_ERR(data->id_det_gpio)) {
+        if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->id_det_gpio = NULL;
+    }
+
+    data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN);
+    if (IS_ERR(data->vbus_det_gpio)) {
+        if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->vbus_det_gpio = NULL;
+    }
+
+    /* We either want both gpio pins or neither (when in host mode) */
+    if (!data->id_det_gpio != !data->vbus_det_gpio) {
+        dev_err(dev, "failed to get id or vbus detect pin\n");
+        return -ENODEV;
+    }
+
+    if (data->id_det_gpio) {
+        ret = devm_extcon_dev_register(dev, &data->extcon);
+        if (ret) {
+            dev_err(dev, "failed to register extcon: %d\n", ret);
+            return ret;
+        }
+    }
+
       for (i = 0; i < data->num_phys; i++) {
           struct sun4i_usb_phy *phy = data->phys + i;
           char name[16];

@@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
           phy_set_drvdata(phy->phy, &data->phys[i]);
       }
 
+    data->id_det_irq = gpiod_to_irq(data->id_det_gpio);
+    data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
+    if (data->id_det_irq  < 0 || data->vbus_det_irq < 0)
+        data->phy0_poll = true;
+
+    if (data->id_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->id_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-id-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting id-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
+    if (data->vbus_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->vbus_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-vbus-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
       dev_set_drvdata(dev, data);
       phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
 
       return PTR_ERR_OR_ZERO(phy_provider);
   }
 
+static int sun4i_usb_phy_remove(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (data->id_det_irq >= 0)
+        devm_free_irq(dev, data->id_det_irq, data);
+    if (data->vbus_det_irq >= 0)
+        devm_free_irq(dev, data->vbus_det_irq, data);
+
+    cancel_delayed_work_sync(&data->detect);
+
+    return 0;
+}
+
   static const struct of_device_id sun4i_usb_phy_of_match[] = {
       { .compatible = "allwinner,sun4i-a10-usb-phy" },
       { .compatible = "allwinner,sun5i-a13-usb-phy" },

@@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
 

Quoted 12 lines

drivers/phy/phy-sun4i-usb.c MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match); · line 598

Quoted 11 lines

drivers/phy/phy-sun4i-usb.c +2 -1

   static struct platform_driver sun4i_usb_phy_driver = {
       .probe    = sun4i_usb_phy_probe,
+    .remove    = sun4i_usb_phy_remove,
       .driver = {
           .of_match_table    = sun4i_usb_phy_of_match,
-        .name  = "sun4i-usb-phy",
+        .name  = DRIVER_NAME,
       }
   };
   module_platform_driver(sun4i_usb_phy_driver);

Thanks,
Chanwoo Choi

Hi Hans,

Quoted 29 lines · Hans de Goede

Hi Chanwoo,

Thanks for the quick review.

Quoted 7 lines · Chanwoo Choi

Hi Hans,

I add the comment about extcon-related code.

Firstly,
I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD

No, just no, this is not what the MFD framework is for, the usb-phy in question here is not a multifunction device. The MFD framework is intended for true multi-function devices like i2c attached PMICs which have regulators, gpios, pwm, input (power button), chargers, power-supply, etc. That is NOT the case here.

Also moving this to the MFD framework would very likely requiring the devicetree binding for the usb-phy to change which we cannot do as that would break the devicetree ABI.

Quoted 5 lines

because there are both extcon
provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory.

Quoted 4 lines

I do not find this a compelling reason, there are plenty of subsystems where not all implementations of the subsystem class live in the subsystem directory, e.g. input and hwmon devices are often also found outside of the input and hwmon driver directories.

There are difference on between input/hwmon and extcon.

Because input and hwmon driver implement the only one type driver as provider driver. But, extcon implement the two type driver of both extcon provider and extcon client driver. The extcon is similiar with regulator and clock framework as resource.

extcon provider driver to provider the event when the state of external connector is changed.
- devm_extcon_dev_register()
- e.g., almost extcon provider driver are included in 'drivers/extcon/' directory.

Hi,

Quoted 39 lines · Chanwoo Choi

Hi Hans,

Quoted 29 lines · Hans de Goede

Hi Chanwoo,

Thanks for the quick review.

Quoted 7 lines · Chanwoo Choi

Hi Hans,

I add the comment about extcon-related code.

Firstly,
I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD

No, just no, this is not what the MFD framework is for, the usb-phy in question here is not a multifunction device. The MFD framework is intended for true multi-function devices like i2c attached PMICs which have regulators, gpios, pwm, input (power button), chargers, power-supply, etc. That is NOT the case here.

Also moving this to the MFD framework would very likely requiring the devicetree binding for the usb-phy to change which we cannot do as that would break the devicetree ABI.

Quoted 5 lines

because there are both extcon
provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory.

Quoted 4 lines

I do not find this a compelling reason, there are plenty of subsystems where not all implementations of the subsystem class live in the subsystem directory, e.g. input and hwmon devices are often also found outside of the input and hwmon driver directories.

There are difference on between input/hwmon and extcon.

Quoted 7 lines

Because input and hwmon driver implement the only one type driver as provider driver. But, extcon implement the two type driver of both extcon provider and extcon client driver. The extcon is similiar with regulator and clock framework as resource.

extcon provider driver to provider the event when the state of external connector is changed.
- devm_extcon_dev_register()
- e.g., almost extcon provider driver are included in 'drivers/extcon/' directory.

I understand, but that does not change my first argument, that the usb-phy is not a MFD device. And although it may be desirable to keep extcon provider drivers in the drivers/extcon, there are no technical reasons to do so.

The whole reason why Kishon asked me to start using the extcon framework is to avoid adding a private API to the phy-sun4i-usb code for notifying the musb-sunxi code about otg-id-pin status changes. Adding a separate driver for just the extcon bits means re-adding a private api to the phy-sun4i-usb code but this time for the extcon code, at which point we might just as well skip extcon and have the musb-sunxi glue code call directly into the phy-sun4i-usb code...

Needing a private API for a separate extcn driver actually is a good argument to NOT have a separate extcon driver and keep the extcon code in the phy-sun4i-usb code, where as I see no technical arguments in favor of a separate extcon driver.

Hi,

Quoted 61 lines · Hans de Goede

Hi,

Quoted 39 lines · Chanwoo Choi

Hi Hans,

Quoted 29 lines · Hans de Goede

Hi Chanwoo,

Thanks for the quick review.

Quoted 7 lines · Chanwoo Choi

Hi Hans,

I add the comment about extcon-related code.

Firstly,
I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD

No, just no, this is not what the MFD framework is for, the usb-phy in question here is not a multifunction device. The MFD framework is intended for true multi-function devices like i2c attached PMICs which have regulators, gpios, pwm, input (power button), chargers, power-supply, etc. That is NOT the case here.

Also moving this to the MFD framework would very likely requiring the devicetree binding for the usb-phy to change which we cannot do as that would break the devicetree ABI.

Quoted 5 lines

because there are both extcon
provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory.

Quoted 4 lines

I do not find this a compelling reason, there are plenty of subsystems where not all implementations of the subsystem class live in the subsystem directory, e.g. input and hwmon devices are often also found outside of the input and hwmon driver directories.

There are difference on between input/hwmon and extcon.

Quoted 7 lines

Because input and hwmon driver implement the only one type driver as provider driver. But, extcon implement the two type driver of both extcon provider and extcon client driver. The extcon is similiar with regulator and clock framework as resource.

extcon provider driver to provider the event when the state of external connector is changed.
- devm_extcon_dev_register()
- e.g., almost extcon provider driver are included in 'drivers/extcon/' directory.

I understand, but that does not change my first argument, that the usb-phy is not a MFD device. And although it may be desirable to keep extcon provider drivers in the drivers/extcon, there are no technical reasons to do so.

The whole reason why Kishon asked me to start using the extcon framework is to avoid adding a private API to the phy-sun4i-usb code for notifying the musb-sunxi code about otg-id-pin status changes. Adding a separate driver for just the extcon bits means re-adding a private api to the phy-sun4i-usb code but this time for the extcon code, at which point we might just as well skip extcon and have the musb-sunxi glue code call directly into the phy-sun4i-usb code...

Quoted 3 lines

Needing a private API for a separate extcn driver actually is a good argument to NOT have a separate extcon driver and keep the extcon code in the phy-sun4i-usb code, where as I see no technical arguments in favor of a separate extcon driver.

There is one technical issue.

The extcon_set_cable_state() should be handled by extcon provider driver. because extcon_set_cable_state() inform the extcon client driver of the event when detecting the change of h/w line (gpio line) or register of peripheral device.

But, extcon client driver can now get the instance of extcon_dev structure
by extcon_get_edev_by_phandle() and then can change the cable state by using the extcon_set_cable_state().

I think that these issue have to be protected by framework level.

Hi,

Quoted 61 lines · Hans de Goede

Hi,

Quoted 39 lines · Chanwoo Choi

Hi Hans,

Quoted 29 lines · Hans de Goede

Hi Chanwoo,

Thanks for the quick review.

Quoted 7 lines · Chanwoo Choi

Hi Hans,

I add the comment about extcon-related code.

Firstly,
I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD

No, just no, this is not what the MFD framework is for, the usb-phy in question here is not a multifunction device. The MFD framework is intended for true multi-function devices like i2c attached PMICs which have regulators, gpios, pwm, input (power button), chargers, power-supply, etc. That is NOT the case here.

Also moving this to the MFD framework would very likely requiring the devicetree binding for the usb-phy to change which we cannot do as that would break the devicetree ABI.

Quoted 5 lines

because there are both extcon
provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory.

Quoted 4 lines

I do not find this a compelling reason, there are plenty of subsystems where not all implementations of the subsystem class live in the subsystem directory, e.g. input and hwmon devices are often also found outside of the input and hwmon driver directories.

There are difference on between input/hwmon and extcon.

Quoted 7 lines

Because input and hwmon driver implement the only one type driver as provider driver. But, extcon implement the two type driver of both extcon provider and extcon client driver. The extcon is similiar with regulator and clock framework as resource.

extcon provider driver to provider the event when the state of external connector is changed.
- devm_extcon_dev_register()
- e.g., almost extcon provider driver are included in 'drivers/extcon/' directory.

I understand, but that does not change my first argument, that the usb-phy is not a MFD device. And although it may be desirable to keep extcon provider drivers in the drivers/extcon, there are no technical reasons to do so.

The whole reason why Kishon asked me to start using the extcon framework is to avoid adding a private API to the phy-sun4i-usb code for notifying the musb-sunxi code about otg-id-pin status changes. Adding a separate driver for just the extcon bits means re-adding a private api to the phy-sun4i-usb code but this time for the extcon code, at which point we might just as well skip extcon and have the musb-sunxi glue code call directly into the phy-sun4i-usb code...

Quoted 3 lines

Needing a private API for a separate extcn driver actually is a good argument to NOT have a separate extcon driver and keep the extcon code in the phy-sun4i-usb code, where as I see no technical arguments in favor of a separate extcon driver.

There is one technical issue.

The extcon_set_cable_state() should be handled by extcon provider driver.

because extcon_set_cable_state() inform the extcon client driver of the event when detecting the change of h/w line (gpio line) or register of peripheral device.

But, extcon client driver can now get the instance of extcon_dev structure
by extcon_get_edev_by_phandle() and then can change the cable state by using the extcon_set_cable_state().

I think that these issue have to be protected by framework level.

Hi,

Quoted 64 lines · Chanwoo Choi

Hi,

Quoted 61 lines · Hans de Goede

Hi,

Quoted 39 lines · Chanwoo Choi

Hi Hans,

Quoted 29 lines · Hans de Goede

Hi Chanwoo,

Thanks for the quick review.

Quoted 7 lines · Chanwoo Choi

Hi Hans,

I add the comment about extcon-related code.

Firstly,
I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD

No, just no, this is not what the MFD framework is for, the usb-phy in question here is not a multifunction device. The MFD framework is intended for true multi-function devices like i2c attached PMICs which have regulators, gpios, pwm, input (power button), chargers, power-supply, etc. That is NOT the case here.

Also moving this to the MFD framework would very likely requiring the devicetree binding for the usb-phy to change which we cannot do as that would break the devicetree ABI.

Quoted 5 lines

because there are both extcon
provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory.

Quoted 4 lines

I do not find this a compelling reason, there are plenty of subsystems where not all implementations of the subsystem class live in the subsystem directory, e.g. input and hwmon devices are often also found outside of the input and hwmon driver directories.

There are difference on between input/hwmon and extcon.

Quoted 7 lines

Because input and hwmon driver implement the only one type driver as provider driver. But, extcon implement the two type driver of both extcon provider and extcon client driver. The extcon is similiar with regulator and clock framework as resource.

extcon provider driver to provider the event when the state of external connector is changed.
- devm_extcon_dev_register()
- e.g., almost extcon provider driver are included in 'drivers/extcon/' directory.

I understand, but that does not change my first argument, that the usb-phy is not a MFD device. And although it may be desirable to keep extcon provider drivers in the drivers/extcon, there are no technical reasons to do so.

The whole reason why Kishon asked me to start using the extcon framework is to avoid adding a private API to the phy-sun4i-usb code for notifying the musb-sunxi code about otg-id-pin status changes. Adding a separate driver for just the extcon bits means re-adding a private api to the phy-sun4i-usb code but this time for the extcon code, at which point we might just as well skip extcon and have the musb-sunxi glue code call directly into the phy-sun4i-usb code...

Quoted 7 lines

Quoted 3 lines

Needing a private API for a separate extcn driver actually is a good argument to NOT have a separate extcon driver and keep the extcon code in the phy-sun4i-usb code, where as I see no technical arguments in favor of a separate extcon driver.

There is one technical issue.

The extcon_set_cable_state() should be handled by extcon provider driver.

That is something which can be done regardless of where the extcon provider driver code is located in the kernel tree...

Quoted 6 lines

because extcon_set_cable_state() inform the extcon client driver of the event when detecting the change of h/w line (gpio line) or register of peripheral device.

But, extcon client driver can now get the instance of extcon_dev structure
by extcon_get_edev_by_phandle() and then can change the cable state by using the extcon_set_cable_state().

Quoted 1 line

I think that these issue have to be protected by framework level.

Protecting this at the framework level would mean protecting it with code in drivers/extcon/extcon.c, that code will be used (and thus can protect things) regardless of where the extcon provider code lives.

I really still see no technical reasons why all extcon provider code MUST be under drivers/extcon.

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:
-Do not call regulator_disable in an unbalanced manner when an external vbus
is present

---
  .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
  drivers/phy/Kconfig                                |   1 +
  drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
  3 files changed, 281 insertions(+), 11 deletions(-)

drivers/phy/phy-sun4i-usb.c +36 -1

  static struct phy *sun4i_usb_phy_xlate(struct device *dev,
  					struct of_phandle_args *args)
  {

@@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
  	struct phy_provider *phy_provider;
  	bool dedicated_clocks;
  	struct resource *res;
-	int i;
+	int i, ret;
 
  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
  	if (!data)
  		return -ENOMEM;
 
  	mutex_init(&data->mutex);
+	INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
+	data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST];
+	data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB];
+	data->extcon_cable_names[2] = NULL;
+	data->extcon.name = DRIVER_NAME;
+	data->extcon.supported_cable = data->extcon_cable_names;
+	data->extcon.dev.parent = dev;
 
  	if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
  		data->num_phys = 2;

@@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
  	if (IS_ERR(data->base))
  		return PTR_ERR(data->base);
 
+	data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN);
+	if (IS_ERR(data->id_det_gpio)) {
+		if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		data->id_det_gpio = NULL;
+	}
+
+	data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN);
+	if (IS_ERR(data->vbus_det_gpio)) {
+		if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		data->vbus_det_gpio = NULL;
+	}
+
+	/* We either want both gpio pins or neither (when in host mode) */
+	if (!data->id_det_gpio != !data->vbus_det_gpio) {
+		dev_err(dev, "failed to get id or vbus detect pin\n");
+		return -ENODEV;
+	}
+
+	if (data->id_det_gpio) {
+		ret = devm_extcon_dev_register(dev, &data->extcon);
+		if (ret) {
+			dev_err(dev, "failed to register extcon: %d\n", ret);
+			return ret;
+		}
+	}
+
  	for (i = 0; i < data->num_phys; i++) {
  		struct sun4i_usb_phy *phy = data->phys + i;
  		char name[16];

drivers/phy/phy-sun4i-usb.c +23 -0

+
+	if (data->id_det_irq >= 0) {
+		ret = devm_request_irq(dev, data->id_det_irq,
+				sun4i_usb_phy0_id_vbus_det_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				"usb0-id-det", data);
+		if (ret) {
+			dev_err(dev, "Err requesting id-det-irq: %d\n", ret);
+			return ret;
+		}
+	}
+
+	if (data->vbus_det_irq >= 0) {
+		ret = devm_request_irq(dev, data->vbus_det_irq,
+				sun4i_usb_phy0_id_vbus_det_irq,
+				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+				"usb0-vbus-det", data);
+		if (ret) {
+			dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret);
+			return ret;
+		}
+	}
+
  	dev_set_drvdata(dev, data);
  	phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
 
  	return PTR_ERR_OR_ZERO(phy_provider);
  }

drivers/phy/phy-sun4i-usb.c +8 -1

+
+	cancel_delayed_work_sync(&data->detect);
+
+	return 0;
+}
+
  static const struct of_device_id sun4i_usb_phy_of_match[] = {
  	{ .compatible = "allwinner,sun4i-a10-usb-phy" },
  	{ .compatible = "allwinner,sun5i-a13-usb-phy" },

@@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
 
  static struct platform_driver sun4i_usb_phy_driver = {
  	.probe	= sun4i_usb_phy_probe,
+	.remove	= sun4i_usb_phy_remove,
  	.driver = {
  		.of_match_table	= sun4i_usb_phy_of_match,
-		.name  = "sun4i-usb-phy",
+		.name  = DRIVER_NAME,
  	}
  };
  module_platform_driver(sun4i_usb_phy_driver);

Hi,

Quoted 28 lines · Hans de Goede

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:
-Do not call regulator_disable in an unbalanced manner when an external vbus
is present

Quoted 6 lines

---
  .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
  drivers/phy/Kconfig                                |   1 +
  drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
  3 files changed, 281 insertions(+), 11 deletions(-)

.
.
<snip>
.
.

drivers/phy/phy-sun4i-usb.c

Quoted 5 lines

drivers/phy/phy-sun4i-usb.c +0 -0

index 91c5be4..b45d707 100644
@@ -1,7 +1,7 @@

.
.
<snip>
.
.

Quoted 64 lines

drivers/phy/phy-sun4i-usb.c +36 -1

  static struct phy *sun4i_usb_phy_xlate(struct device *dev,
                      struct of_phandle_args *args)
  {

@@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
      struct phy_provider *phy_provider;
      bool dedicated_clocks;
      struct resource *res;
-    int i;
+    int i, ret;
 
      data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
      if (!data)
          return -ENOMEM;
 
      mutex_init(&data->mutex);
+    INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
+    data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST];
+    data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB];
+    data->extcon_cable_names[2] = NULL;
+    data->extcon.name = DRIVER_NAME;
+    data->extcon.supported_cable = data->extcon_cable_names;
+    data->extcon.dev.parent = dev;
 
      if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
          data->num_phys = 2;

@@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
      if (IS_ERR(data->base))
          return PTR_ERR(data->base);
 
+    data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN);
+    if (IS_ERR(data->id_det_gpio)) {
+        if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->id_det_gpio = NULL;
+    }
+
+    data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN);
+    if (IS_ERR(data->vbus_det_gpio)) {
+        if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->vbus_det_gpio = NULL;
+    }
+
+    /* We either want both gpio pins or neither (when in host mode) */
+    if (!data->id_det_gpio != !data->vbus_det_gpio) {
+        dev_err(dev, "failed to get id or vbus detect pin\n");
+        return -ENODEV;
+    }
+
+    if (data->id_det_gpio) {
+        ret = devm_extcon_dev_register(dev, &data->extcon);
+        if (ret) {
+            dev_err(dev, "failed to register extcon: %d\n", ret);
+            return ret;
+        }
+    }
+
      for (i = 0; i < data->num_phys; i++) {
          struct sun4i_usb_phy *phy = data->phys + i;
          char name[16];

@@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
          phy_set_drvdata(phy->phy, &data->phys[i]);
      }
 

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 536

Quoted 4 lines

drivers/phy/phy-sun4i-usb.c +4 -0

+    data->id_det_irq = gpiod_to_irq(data->id_det_gpio);
+    data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
+    if (data->id_det_irq  < 0 || data->vbus_det_irq < 0)
+        data->phy0_poll = true;

if polling is enabled, we shouldn't enable irq at all?

Quoted 29 lines

drivers/phy/phy-sun4i-usb.c +23 -0

+
+    if (data->id_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->id_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-id-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting id-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
+    if (data->vbus_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->vbus_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-vbus-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
      dev_set_drvdata(dev, data);
      phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
 
      return PTR_ERR_OR_ZERO(phy_provider);
  }
 

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 574

Quoted 9 lines

drivers/phy/phy-sun4i-usb.c +9 -0

+static int sun4i_usb_phy_remove(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (data->id_det_irq >= 0)
+        devm_free_irq(dev, data->id_det_irq, data);
+    if (data->vbus_det_irq >= 0)
+        devm_free_irq(dev, data->vbus_det_irq, data);

This shouldn't be needed since you already use devm_* in probe.

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 576

Quoted 1 line

drivers/phy/phy-sun4i-usb.c +1 -0

+    cancel_delayed_work_sync(&data->detect);

Quoted 9 lines

drivers/phy/phy-sun4i-usb.c +4 -0

+
+    return 0;
+}
+
  static const struct of_device_id sun4i_usb_phy_of_match[] = {
      { .compatible = "allwinner,sun4i-a10-usb-phy" },
      { .compatible = "allwinner,sun5i-a13-usb-phy" },

@@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match);
 

drivers/phy/phy-sun4i-usb.c MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match); · line 598

Quoted 11 lines

drivers/phy/phy-sun4i-usb.c +2 -1

  static struct platform_driver sun4i_usb_phy_driver = {
      .probe    = sun4i_usb_phy_probe,
+    .remove    = sun4i_usb_phy_remove,
      .driver = {
          .of_match_table    = sun4i_usb_phy_of_match,
-        .name  = "sun4i-usb-phy",
+        .name  = DRIVER_NAME,
      }
  };
  module_platform_driver(sun4i_usb_phy_driver);

Hi,

Quoted 117 lines · Kishon Vijay Abraham I

Hi,

Quoted 28 lines · Hans de Goede

The usb0 phy is connected to an OTG controller, and as such needs some special handling:

1) It allows explicit control over the pullups, enable these on phy_init and disable them on phy_exit.

2) It has bits to signal id and vbus detect to the musb-core, add support for for monitoring id and vbus detect gpio-s for use in dual role mode, and set these bits to the correct values for operating in host only mode when no gpios are specified in the devicetree.

3) When in dual role mode the musb sunxi glue needs to know if the a host or device cable is plugged in, so when in dual role mode register an extcon.

While updating the devicetree binding documentation also add documentation for the sofar undocumented usage of regulators for vbus for all 3 phys.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Removed the sunxi specific phy functions, instead the id / vbus gpio polling
has been moved to the phy-sun4i-usb driver and their status is exported through extcon for the sunxi-musb glue
Changes in v3:
-No changes
Changes in v4:
-Do not call regulator_disable in an unbalanced manner when an external vbus
is present

Quoted 6 lines

---
   .../devicetree/bindings/phy/sun4i-usb-phy.txt      |  18 +-
   drivers/phy/Kconfig                                |   1 +
   drivers/phy/phy-sun4i-usb.c                        | 273 ++++++++++++++++++++-
   3 files changed, 281 insertions(+), 11 deletions(-)

.
.
<snip>
.
.

drivers/phy/phy-sun4i-usb.c

Quoted 5 lines

drivers/phy/phy-sun4i-usb.c +0 -0

index 91c5be4..b45d707 100644
@@ -1,7 +1,7 @@

.
.
<snip>
.
.

Quoted 64 lines

drivers/phy/phy-sun4i-usb.c +36 -1

   static struct phy *sun4i_usb_phy_xlate(struct device *dev,
                       struct of_phandle_args *args)
   {

@@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
       struct phy_provider *phy_provider;
       bool dedicated_clocks;
       struct resource *res;
-    int i;
+    int i, ret;
 
       data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
       if (!data)
           return -ENOMEM;
 
       mutex_init(&data->mutex);
+    INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan);
+    data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST];
+    data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB];
+    data->extcon_cable_names[2] = NULL;
+    data->extcon.name = DRIVER_NAME;
+    data->extcon.supported_cable = data->extcon_cable_names;
+    data->extcon.dev.parent = dev;
 
       if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy"))
           data->num_phys = 2;

@@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
       if (IS_ERR(data->base))
           return PTR_ERR(data->base);
 
+    data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN);
+    if (IS_ERR(data->id_det_gpio)) {
+        if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->id_det_gpio = NULL;
+    }
+
+    data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN);
+    if (IS_ERR(data->vbus_det_gpio)) {
+        if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER)
+            return -EPROBE_DEFER;
+        data->vbus_det_gpio = NULL;
+    }
+
+    /* We either want both gpio pins or neither (when in host mode) */
+    if (!data->id_det_gpio != !data->vbus_det_gpio) {
+        dev_err(dev, "failed to get id or vbus detect pin\n");
+        return -ENODEV;
+    }
+
+    if (data->id_det_gpio) {
+        ret = devm_extcon_dev_register(dev, &data->extcon);
+        if (ret) {
+            dev_err(dev, "failed to register extcon: %d\n", ret);
+            return ret;
+        }
+    }
+
       for (i = 0; i < data->num_phys; i++) {
           struct sun4i_usb_phy *phy = data->phys + i;
           char name[16];

@@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
           phy_set_drvdata(phy->phy, &data->phys[i]);
       }
 

Quoted 6 lines

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 536

Quoted 4 lines

drivers/phy/phy-sun4i-usb.c +4 -0

+    data->id_det_irq = gpiod_to_irq(data->id_det_gpio);
+    data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio);
+    if (data->id_det_irq  < 0 || data->vbus_det_irq < 0)
+        data->phy0_poll = true;

if polling is enabled, we shouldn't enable irq at all?

Thanks for the review.

One some boards one of the gpio-s is irq capable and the other is not, in which case the current code indeed enables both irq handling for the one gpio which is irq capable and enables polling. This is done this way deliberately as the irq path has much better latency then polling and the 2 can co-exist.

Quoted 30 lines

Quoted 30 lines

drivers/phy/phy-sun4i-usb.c +23 -0

+
+    if (data->id_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->id_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-id-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting id-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
+    if (data->vbus_det_irq >= 0) {
+        ret = devm_request_irq(dev, data->vbus_det_irq,
+                sun4i_usb_phy0_id_vbus_det_irq,
+                IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
+                "usb0-vbus-det", data);
+        if (ret) {
+            dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret);
+            return ret;
+        }
+    }
+
       dev_set_drvdata(dev, data);
       phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate);
 
       return PTR_ERR_OR_ZERO(phy_provider);
   }
 

Quoted 11 lines

drivers/phy/phy-sun4i-usb.c static int sun4i_usb_phy_probe(struct platform_device *pdev) · line 574

Quoted 9 lines

drivers/phy/phy-sun4i-usb.c +9 -0

+static int sun4i_usb_phy_remove(struct platform_device *pdev)
+{
+    struct device *dev = &pdev->dev;
+    struct sun4i_usb_phy_data *data = dev_get_drvdata(dev);
+
+    if (data->id_det_irq >= 0)
+        devm_free_irq(dev, data->id_det_irq, data);
+    if (data->vbus_det_irq >= 0)
+        devm_free_irq(dev, data->vbus_det_irq, data);

This shouldn't be needed since you already use devm_* in probe.