[PATCH v2 1/3] usb: common: usb-conn-gpio: fix NULL pointer dereference of charger

[PATCH v2 1/3] usb: common: usb-conn-gpio: fix NULL pointer dereference of charger

[Chunfeng Yun]

When power on system with OTG cable, IDDIG's interrupt arises before
the charger registration, it will cause a NULL pointer dereference,
fix the issue by registering the power supply before requesting
IDDIG/VBUS irq.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: no changes
---
 drivers/usb/common/usb-conn-gpio.c | 44 ++++++++++++++++++------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index 6c4e3a19f42c..c9545a4eff66 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -149,14 +149,32 @@ static int usb_charger_get_property(struct power_supply *psy,
 	return 0;
 }
 
-static int usb_conn_probe(struct platform_device *pdev)
+static int usb_conn_psy_register(struct usb_conn_info *info)
 {
-	struct device *dev = &pdev->dev;
-	struct power_supply_desc *desc;
-	struct usb_conn_info *info;
+	struct device *dev = info->dev;
+	struct power_supply_desc *desc = &info->desc;
 	struct power_supply_config cfg = {
 		.of_node = dev->of_node,
 	};
+
+	desc->name = "usb-charger";
+	desc->properties = usb_charger_properties;
+	desc->num_properties = ARRAY_SIZE(usb_charger_properties);
+	desc->get_property = usb_charger_get_property;
+	desc->type = POWER_SUPPLY_TYPE_USB;
+	cfg.drv_data = info;
+
+	info->charger = devm_power_supply_register(dev, desc, &cfg);
+	if (IS_ERR(info->charger))
+		dev_err(dev, "Unable to register charger\n");
+
+	return PTR_ERR_OR_ZERO(info->charger);
+}
+
+static int usb_conn_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_conn_info *info;
 	bool need_vbus = true;
 	int ret = 0;
 
@@ -218,6 +236,10 @@ static int usb_conn_probe(struct platform_device *pdev)
 		return PTR_ERR(info->role_sw);
 	}
 
+	ret = usb_conn_psy_register(info);
+	if (ret)
+		goto put_role_sw;
+
 	if (info->id_gpiod) {
 		info->id_irq = gpiod_to_irq(info->id_gpiod);
 		if (info->id_irq < 0) {
@@ -252,20 +274,6 @@ static int usb_conn_probe(struct platform_device *pdev)
 		}
 	}
 
-	desc = &info->desc;
-	desc->name = "usb-charger";
-	desc->properties = usb_charger_properties;
-	desc->num_properties = ARRAY_SIZE(usb_charger_properties);
-	desc->get_property = usb_charger_get_property;
-	desc->type = POWER_SUPPLY_TYPE_USB;
-	cfg.drv_data = info;
-
-	info->charger = devm_power_supply_register(dev, desc, &cfg);
-	if (IS_ERR(info->charger)) {
-		dev_err(dev, "Unable to register charger\n");
-		return PTR_ERR(info->charger);
-	}
-
 	platform_set_drvdata(pdev, info);
 
 	/* Perform initial detection */
-- 
2.18.0

[PATCH v2 1/3] usb: common: usb-conn-gpio: fix NULL pointer dereference of charger

[Chunfeng Yun]

When power on system with OTG cable, IDDIG's interrupt arises before
the charger registration, it will cause a NULL pointer dereference,
fix the issue by registering the power supply before requesting
IDDIG/VBUS irq.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: no changes
---
 drivers/usb/common/usb-conn-gpio.c | 44 ++++++++++++++++++------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index 6c4e3a19f42c..c9545a4eff66 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -149,14 +149,32 @@ static int usb_charger_get_property(struct power_supply *psy,
 	return 0;
 }
 
-static int usb_conn_probe(struct platform_device *pdev)
+static int usb_conn_psy_register(struct usb_conn_info *info)
 {
-	struct device *dev = &pdev->dev;
-	struct power_supply_desc *desc;
-	struct usb_conn_info *info;
+	struct device *dev = info->dev;
+	struct power_supply_desc *desc = &info->desc;
 	struct power_supply_config cfg = {
 		.of_node = dev->of_node,
 	};
+
+	desc->name = "usb-charger";
+	desc->properties = usb_charger_properties;
+	desc->num_properties = ARRAY_SIZE(usb_charger_properties);
+	desc->get_property = usb_charger_get_property;
+	desc->type = POWER_SUPPLY_TYPE_USB;
+	cfg.drv_data = info;
+
+	info->charger = devm_power_supply_register(dev, desc, &cfg);
+	if (IS_ERR(info->charger))
+		dev_err(dev, "Unable to register charger\n");
+
+	return PTR_ERR_OR_ZERO(info->charger);
+}
+
+static int usb_conn_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_conn_info *info;
 	bool need_vbus = true;
 	int ret = 0;
 
@@ -218,6 +236,10 @@ static int usb_conn_probe(struct platform_device *pdev)
 		return PTR_ERR(info->role_sw);
 	}
 
+	ret = usb_conn_psy_register(info);
+	if (ret)
+		goto put_role_sw;
+
 	if (info->id_gpiod) {
 		info->id_irq = gpiod_to_irq(info->id_gpiod);
 		if (info->id_irq < 0) {
@@ -252,20 +274,6 @@ static int usb_conn_probe(struct platform_device *pdev)
 		}
 	}
 
-	desc = &info->desc;
-	desc->name = "usb-charger";
-	desc->properties = usb_charger_properties;
-	desc->num_properties = ARRAY_SIZE(usb_charger_properties);
-	desc->get_property = usb_charger_get_property;
-	desc->type = POWER_SUPPLY_TYPE_USB;
-	cfg.drv_data = info;
-
-	info->charger = devm_power_supply_register(dev, desc, &cfg);
-	if (IS_ERR(info->charger)) {
-		dev_err(dev, "Unable to register charger\n");
-		return PTR_ERR(info->charger);
-	}
-
 	platform_set_drvdata(pdev, info);
 
 	/* Perform initial detection */
-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/3] usb: common: usb-conn-gpio: fix NULL pointer dereference of charger

[Chunfeng Yun]

When power on system with OTG cable, IDDIG's interrupt arises before
the charger registration, it will cause a NULL pointer dereference,
fix the issue by registering the power supply before requesting
IDDIG/VBUS irq.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: no changes
---
 drivers/usb/common/usb-conn-gpio.c | 44 ++++++++++++++++++------------
 1 file changed, 26 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index 6c4e3a19f42c..c9545a4eff66 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -149,14 +149,32 @@ static int usb_charger_get_property(struct power_supply *psy,
 	return 0;
 }
 
-static int usb_conn_probe(struct platform_device *pdev)
+static int usb_conn_psy_register(struct usb_conn_info *info)
 {
-	struct device *dev = &pdev->dev;
-	struct power_supply_desc *desc;
-	struct usb_conn_info *info;
+	struct device *dev = info->dev;
+	struct power_supply_desc *desc = &info->desc;
 	struct power_supply_config cfg = {
 		.of_node = dev->of_node,
 	};
+
+	desc->name = "usb-charger";
+	desc->properties = usb_charger_properties;
+	desc->num_properties = ARRAY_SIZE(usb_charger_properties);
+	desc->get_property = usb_charger_get_property;
+	desc->type = POWER_SUPPLY_TYPE_USB;
+	cfg.drv_data = info;
+
+	info->charger = devm_power_supply_register(dev, desc, &cfg);
+	if (IS_ERR(info->charger))
+		dev_err(dev, "Unable to register charger\n");
+
+	return PTR_ERR_OR_ZERO(info->charger);
+}
+
+static int usb_conn_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct usb_conn_info *info;
 	bool need_vbus = true;
 	int ret = 0;
 
@@ -218,6 +236,10 @@ static int usb_conn_probe(struct platform_device *pdev)
 		return PTR_ERR(info->role_sw);
 	}
 
+	ret = usb_conn_psy_register(info);
+	if (ret)
+		goto put_role_sw;
+
 	if (info->id_gpiod) {
 		info->id_irq = gpiod_to_irq(info->id_gpiod);
 		if (info->id_irq < 0) {
@@ -252,20 +274,6 @@ static int usb_conn_probe(struct platform_device *pdev)
 		}
 	}
 
-	desc = &info->desc;
-	desc->name = "usb-charger";
-	desc->properties = usb_charger_properties;
-	desc->num_properties = ARRAY_SIZE(usb_charger_properties);
-	desc->get_property = usb_charger_get_property;
-	desc->type = POWER_SUPPLY_TYPE_USB;
-	cfg.drv_data = info;
-
-	info->charger = devm_power_supply_register(dev, desc, &cfg);
-	if (IS_ERR(info->charger)) {
-		dev_err(dev, "Unable to register charger\n");
-		return PTR_ERR(info->charger);
-	}
-
 	platform_set_drvdata(pdev, info);
 
 	/* Perform initial detection */
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 2/3] usb: common: usb-conn-gpio: use dev_err_probe() to print log

[Chunfeng Yun]

Use dev_err_probe() to print debug or error message depending on
whether the error value is -DPROBE_DEFER or not.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: no changes
---
 drivers/usb/common/usb-conn-gpio.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index c9545a4eff66..dfbbc4f51ed6 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -223,18 +223,14 @@ static int usb_conn_probe(struct platform_device *pdev)
 	}
 
 	if (IS_ERR(info->vbus)) {
-		if (PTR_ERR(info->vbus) != -EPROBE_DEFER)
-			dev_err(dev, "failed to get vbus: %ld\n", PTR_ERR(info->vbus));
-		return PTR_ERR(info->vbus);
+		ret = PTR_ERR(info->vbus);
+		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
 	}
 
 	info->role_sw = usb_role_switch_get(dev);
-	if (IS_ERR(info->role_sw)) {
-		if (PTR_ERR(info->role_sw) != -EPROBE_DEFER)
-			dev_err(dev, "failed to get role switch\n");
-
-		return PTR_ERR(info->role_sw);
-	}
+	if (IS_ERR(info->role_sw))
+		return dev_err_probe(dev, PTR_ERR(info->role_sw),
+				     "failed to get role switch\n");
 
 	ret = usb_conn_psy_register(info);
 	if (ret)
-- 
2.18.0

[PATCH v2 2/3] usb: common: usb-conn-gpio: use dev_err_probe() to print log

[Chunfeng Yun]

Use dev_err_probe() to print debug or error message depending on
whether the error value is -DPROBE_DEFER or not.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: no changes
---
 drivers/usb/common/usb-conn-gpio.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index c9545a4eff66..dfbbc4f51ed6 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -223,18 +223,14 @@ static int usb_conn_probe(struct platform_device *pdev)
 	}
 
 	if (IS_ERR(info->vbus)) {
-		if (PTR_ERR(info->vbus) != -EPROBE_DEFER)
-			dev_err(dev, "failed to get vbus: %ld\n", PTR_ERR(info->vbus));
-		return PTR_ERR(info->vbus);
+		ret = PTR_ERR(info->vbus);
+		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
 	}
 
 	info->role_sw = usb_role_switch_get(dev);
-	if (IS_ERR(info->role_sw)) {
-		if (PTR_ERR(info->role_sw) != -EPROBE_DEFER)
-			dev_err(dev, "failed to get role switch\n");
-
-		return PTR_ERR(info->role_sw);
-	}
+	if (IS_ERR(info->role_sw))
+		return dev_err_probe(dev, PTR_ERR(info->role_sw),
+				     "failed to get role switch\n");
 
 	ret = usb_conn_psy_register(info);
 	if (ret)
-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] usb: common: usb-conn-gpio: use dev_err_probe() to print log

[Chunfeng Yun]

Use dev_err_probe() to print debug or error message depending on
whether the error value is -DPROBE_DEFER or not.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: no changes
---
 drivers/usb/common/usb-conn-gpio.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index c9545a4eff66..dfbbc4f51ed6 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -223,18 +223,14 @@ static int usb_conn_probe(struct platform_device *pdev)
 	}
 
 	if (IS_ERR(info->vbus)) {
-		if (PTR_ERR(info->vbus) != -EPROBE_DEFER)
-			dev_err(dev, "failed to get vbus: %ld\n", PTR_ERR(info->vbus));
-		return PTR_ERR(info->vbus);
+		ret = PTR_ERR(info->vbus);
+		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
 	}
 
 	info->role_sw = usb_role_switch_get(dev);
-	if (IS_ERR(info->role_sw)) {
-		if (PTR_ERR(info->role_sw) != -EPROBE_DEFER)
-			dev_err(dev, "failed to get role switch\n");
-
-		return PTR_ERR(info->role_sw);
-	}
+	if (IS_ERR(info->role_sw))
+		return dev_err_probe(dev, PTR_ERR(info->role_sw),
+				     "failed to get role switch\n");
 
 	ret = usb_conn_psy_register(info);
 	if (ret)
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

Vbus is already an optional supply, if the vbus-supply is not
provided in DTS, will use a dummy regulator,
the warning log is as below:
"supply vbus not found, using dummy regulator"

This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.

Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: remove unused variable "need_vbus"
---
 drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index dfbbc4f51ed6..65d89140cd19 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
 		return;
 	}
 
-	if (info->last_role == USB_ROLE_HOST && info->vbus)
+	if (info->last_role == USB_ROLE_HOST)
 		regulator_disable(info->vbus);
 
 	ret = usb_role_switch_set_role(info->role_sw, role);
 	if (ret)
 		dev_err(info->dev, "failed to set role: %d\n", ret);
 
-	if (role == USB_ROLE_HOST && info->vbus) {
+	if (role == USB_ROLE_HOST) {
 		ret = regulator_enable(info->vbus);
 		if (ret)
 			dev_err(info->dev, "enable vbus regulator failed\n");
@@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
 
 	info->last_role = role;
 
-	if (info->vbus)
-		dev_dbg(info->dev, "vbus regulator is %s\n",
-			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
+	dev_dbg(info->dev, "vbus regulator is %s\n",
+		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
 
 	power_supply_changed(info->charger);
 }
@@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct usb_conn_info *info;
-	bool need_vbus = true;
 	int ret = 0;
 
 	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
@@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
 
-	/*
-	 * If the USB connector is a child of a USB port and that port already provides the VBUS
-	 * supply, there's no need for the USB connector to provide it again.
-	 */
-	if (dev->parent && dev->parent->of_node) {
-		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
-			need_vbus = false;
-	}
-
-	if (!need_vbus) {
-		info->vbus = devm_regulator_get_optional(dev, "vbus");
-		if (PTR_ERR(info->vbus) == -ENODEV)
-			info->vbus = NULL;
-	} else {
-		info->vbus = devm_regulator_get(dev, "vbus");
-	}
-
+	info->vbus = devm_regulator_get(dev, "vbus");
 	if (IS_ERR(info->vbus)) {
 		ret = PTR_ERR(info->vbus);
 		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
@@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
 
 	cancel_delayed_work_sync(&info->dw_det);
 
-	if (info->last_role == USB_ROLE_HOST && info->vbus)
+	if (info->last_role == USB_ROLE_HOST)
 		regulator_disable(info->vbus);
 
 	usb_role_switch_put(info->role_sw);
-- 
2.18.0

[PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

Vbus is already an optional supply, if the vbus-supply is not
provided in DTS, will use a dummy regulator,
the warning log is as below:
"supply vbus not found, using dummy regulator"

This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.

Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: remove unused variable "need_vbus"
---
 drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index dfbbc4f51ed6..65d89140cd19 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
 		return;
 	}
 
-	if (info->last_role == USB_ROLE_HOST && info->vbus)
+	if (info->last_role == USB_ROLE_HOST)
 		regulator_disable(info->vbus);
 
 	ret = usb_role_switch_set_role(info->role_sw, role);
 	if (ret)
 		dev_err(info->dev, "failed to set role: %d\n", ret);
 
-	if (role == USB_ROLE_HOST && info->vbus) {
+	if (role == USB_ROLE_HOST) {
 		ret = regulator_enable(info->vbus);
 		if (ret)
 			dev_err(info->dev, "enable vbus regulator failed\n");
@@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
 
 	info->last_role = role;
 
-	if (info->vbus)
-		dev_dbg(info->dev, "vbus regulator is %s\n",
-			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
+	dev_dbg(info->dev, "vbus regulator is %s\n",
+		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
 
 	power_supply_changed(info->charger);
 }
@@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct usb_conn_info *info;
-	bool need_vbus = true;
 	int ret = 0;
 
 	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
@@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
 
-	/*
-	 * If the USB connector is a child of a USB port and that port already provides the VBUS
-	 * supply, there's no need for the USB connector to provide it again.
-	 */
-	if (dev->parent && dev->parent->of_node) {
-		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
-			need_vbus = false;
-	}
-
-	if (!need_vbus) {
-		info->vbus = devm_regulator_get_optional(dev, "vbus");
-		if (PTR_ERR(info->vbus) == -ENODEV)
-			info->vbus = NULL;
-	} else {
-		info->vbus = devm_regulator_get(dev, "vbus");
-	}
-
+	info->vbus = devm_regulator_get(dev, "vbus");
 	if (IS_ERR(info->vbus)) {
 		ret = PTR_ERR(info->vbus);
 		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
@@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
 
 	cancel_delayed_work_sync(&info->dw_det);
 
-	if (info->last_role == USB_ROLE_HOST && info->vbus)
+	if (info->last_role == USB_ROLE_HOST)
 		regulator_disable(info->vbus);
 
 	usb_role_switch_put(info->role_sw);
-- 
2.18.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

Vbus is already an optional supply, if the vbus-supply is not
provided in DTS, will use a dummy regulator,
the warning log is as below:
"supply vbus not found, using dummy regulator"

This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.

Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
v2: remove unused variable "need_vbus"
---
 drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
index dfbbc4f51ed6..65d89140cd19 100644
--- a/drivers/usb/common/usb-conn-gpio.c
+++ b/drivers/usb/common/usb-conn-gpio.c
@@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
 		return;
 	}
 
-	if (info->last_role == USB_ROLE_HOST && info->vbus)
+	if (info->last_role == USB_ROLE_HOST)
 		regulator_disable(info->vbus);
 
 	ret = usb_role_switch_set_role(info->role_sw, role);
 	if (ret)
 		dev_err(info->dev, "failed to set role: %d\n", ret);
 
-	if (role == USB_ROLE_HOST && info->vbus) {
+	if (role == USB_ROLE_HOST) {
 		ret = regulator_enable(info->vbus);
 		if (ret)
 			dev_err(info->dev, "enable vbus regulator failed\n");
@@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
 
 	info->last_role = role;
 
-	if (info->vbus)
-		dev_dbg(info->dev, "vbus regulator is %s\n",
-			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
+	dev_dbg(info->dev, "vbus regulator is %s\n",
+		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
 
 	power_supply_changed(info->charger);
 }
@@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct usb_conn_info *info;
-	bool need_vbus = true;
 	int ret = 0;
 
 	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
@@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
 
 	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
 
-	/*
-	 * If the USB connector is a child of a USB port and that port already provides the VBUS
-	 * supply, there's no need for the USB connector to provide it again.
-	 */
-	if (dev->parent && dev->parent->of_node) {
-		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
-			need_vbus = false;
-	}
-
-	if (!need_vbus) {
-		info->vbus = devm_regulator_get_optional(dev, "vbus");
-		if (PTR_ERR(info->vbus) == -ENODEV)
-			info->vbus = NULL;
-	} else {
-		info->vbus = devm_regulator_get(dev, "vbus");
-	}
-
+	info->vbus = devm_regulator_get(dev, "vbus");
 	if (IS_ERR(info->vbus)) {
 		ret = PTR_ERR(info->vbus);
 		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
@@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
 
 	cancel_delayed_work_sync(&info->dw_det);
 
-	if (info->last_role == USB_ROLE_HOST && info->vbus)
+	if (info->last_role == USB_ROLE_HOST)
 		regulator_disable(info->vbus);
 
 	usb_role_switch_put(info->role_sw);
-- 
2.18.0
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4618 bytes --]

On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> Vbus is already an optional supply, if the vbus-supply is not
> provided in DTS, will use a dummy regulator,

That statement is not entirely correct. The dummy regulator is
substituted only if the supply is in fact not optional. The idea behind
that is to allow DTS files that don't specify all required regulators to
get away with it, based on the assumption that the supply is one of
those always-on supplies that are often not described in DTS.

> the warning log is as below:
> "supply vbus not found, using dummy regulator"

And the reason why we get that warning is to point out that the DTS has
a bug and that it should be fixed (by adding a proper regulator to take
the place of the dummy).

> This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.

But if you read the description of that commit, the purpose of that
patch was in fact to make the supply completely optional in the case
where we already have the VBUS supply specified for the USB port that
the connector is parented to.

So in that case the DTS doesn't have the bug because the VBUS supply is
already specified for the USB port and therefore it doesn't have to be
specified in the USB connector again. In fact, specifying it twice can
lead to a situation where the USB port may not be able to switch the
VBUS supply on or off because the setting might conflict with that of
the USB connector.

So unless there's a real reason why this is needed, I don't think this
should be applied.

Thierry

> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2: remove unused variable "need_vbus"
> ---
>  drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
> index dfbbc4f51ed6..65d89140cd19 100644
> --- a/drivers/usb/common/usb-conn-gpio.c
> +++ b/drivers/usb/common/usb-conn-gpio.c
> @@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
>  		return;
>  	}
>  
> -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> +	if (info->last_role == USB_ROLE_HOST)
>  		regulator_disable(info->vbus);
>  
>  	ret = usb_role_switch_set_role(info->role_sw, role);
>  	if (ret)
>  		dev_err(info->dev, "failed to set role: %d\n", ret);
>  
> -	if (role == USB_ROLE_HOST && info->vbus) {
> +	if (role == USB_ROLE_HOST) {
>  		ret = regulator_enable(info->vbus);
>  		if (ret)
>  			dev_err(info->dev, "enable vbus regulator failed\n");
> @@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
>  
>  	info->last_role = role;
>  
> -	if (info->vbus)
> -		dev_dbg(info->dev, "vbus regulator is %s\n",
> -			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> +	dev_dbg(info->dev, "vbus regulator is %s\n",
> +		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
>  
>  	power_supply_changed(info->charger);
>  }
> @@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct usb_conn_info *info;
> -	bool need_vbus = true;
>  	int ret = 0;
>  
>  	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> @@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
>  
> -	/*
> -	 * If the USB connector is a child of a USB port and that port already provides the VBUS
> -	 * supply, there's no need for the USB connector to provide it again.
> -	 */
> -	if (dev->parent && dev->parent->of_node) {
> -		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
> -			need_vbus = false;
> -	}
> -
> -	if (!need_vbus) {
> -		info->vbus = devm_regulator_get_optional(dev, "vbus");
> -		if (PTR_ERR(info->vbus) == -ENODEV)
> -			info->vbus = NULL;
> -	} else {
> -		info->vbus = devm_regulator_get(dev, "vbus");
> -	}
> -
> +	info->vbus = devm_regulator_get(dev, "vbus");
>  	if (IS_ERR(info->vbus)) {
>  		ret = PTR_ERR(info->vbus);
>  		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
> @@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
>  
>  	cancel_delayed_work_sync(&info->dw_det);
>  
> -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> +	if (info->last_role == USB_ROLE_HOST)
>  		regulator_disable(info->vbus);
>  
>  	usb_role_switch_put(info->role_sw);
> -- 
> 2.18.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4618 bytes --]

On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> Vbus is already an optional supply, if the vbus-supply is not
> provided in DTS, will use a dummy regulator,

That statement is not entirely correct. The dummy regulator is
substituted only if the supply is in fact not optional. The idea behind
that is to allow DTS files that don't specify all required regulators to
get away with it, based on the assumption that the supply is one of
those always-on supplies that are often not described in DTS.

> the warning log is as below:
> "supply vbus not found, using dummy regulator"

And the reason why we get that warning is to point out that the DTS has
a bug and that it should be fixed (by adding a proper regulator to take
the place of the dummy).

> This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.

But if you read the description of that commit, the purpose of that
patch was in fact to make the supply completely optional in the case
where we already have the VBUS supply specified for the USB port that
the connector is parented to.

So in that case the DTS doesn't have the bug because the VBUS supply is
already specified for the USB port and therefore it doesn't have to be
specified in the USB connector again. In fact, specifying it twice can
lead to a situation where the USB port may not be able to switch the
VBUS supply on or off because the setting might conflict with that of
the USB connector.

So unless there's a real reason why this is needed, I don't think this
should be applied.

Thierry

> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2: remove unused variable "need_vbus"
> ---
>  drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
> index dfbbc4f51ed6..65d89140cd19 100644
> --- a/drivers/usb/common/usb-conn-gpio.c
> +++ b/drivers/usb/common/usb-conn-gpio.c
> @@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
>  		return;
>  	}
>  
> -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> +	if (info->last_role == USB_ROLE_HOST)
>  		regulator_disable(info->vbus);
>  
>  	ret = usb_role_switch_set_role(info->role_sw, role);
>  	if (ret)
>  		dev_err(info->dev, "failed to set role: %d\n", ret);
>  
> -	if (role == USB_ROLE_HOST && info->vbus) {
> +	if (role == USB_ROLE_HOST) {
>  		ret = regulator_enable(info->vbus);
>  		if (ret)
>  			dev_err(info->dev, "enable vbus regulator failed\n");
> @@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
>  
>  	info->last_role = role;
>  
> -	if (info->vbus)
> -		dev_dbg(info->dev, "vbus regulator is %s\n",
> -			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> +	dev_dbg(info->dev, "vbus regulator is %s\n",
> +		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
>  
>  	power_supply_changed(info->charger);
>  }
> @@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct usb_conn_info *info;
> -	bool need_vbus = true;
>  	int ret = 0;
>  
>  	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> @@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
>  
> -	/*
> -	 * If the USB connector is a child of a USB port and that port already provides the VBUS
> -	 * supply, there's no need for the USB connector to provide it again.
> -	 */
> -	if (dev->parent && dev->parent->of_node) {
> -		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
> -			need_vbus = false;
> -	}
> -
> -	if (!need_vbus) {
> -		info->vbus = devm_regulator_get_optional(dev, "vbus");
> -		if (PTR_ERR(info->vbus) == -ENODEV)
> -			info->vbus = NULL;
> -	} else {
> -		info->vbus = devm_regulator_get(dev, "vbus");
> -	}
> -
> +	info->vbus = devm_regulator_get(dev, "vbus");
>  	if (IS_ERR(info->vbus)) {
>  		ret = PTR_ERR(info->vbus);
>  		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
> @@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
>  
>  	cancel_delayed_work_sync(&info->dw_det);
>  
> -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> +	if (info->last_role == USB_ROLE_HOST)
>  		regulator_disable(info->vbus);
>  
>  	usb_role_switch_put(info->role_sw);
> -- 
> 2.18.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4618 bytes --]

On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> Vbus is already an optional supply, if the vbus-supply is not
> provided in DTS, will use a dummy regulator,

That statement is not entirely correct. The dummy regulator is
substituted only if the supply is in fact not optional. The idea behind
that is to allow DTS files that don't specify all required regulators to
get away with it, based on the assumption that the supply is one of
those always-on supplies that are often not described in DTS.

> the warning log is as below:
> "supply vbus not found, using dummy regulator"

And the reason why we get that warning is to point out that the DTS has
a bug and that it should be fixed (by adding a proper regulator to take
the place of the dummy).

> This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.

But if you read the description of that commit, the purpose of that
patch was in fact to make the supply completely optional in the case
where we already have the VBUS supply specified for the USB port that
the connector is parented to.

So in that case the DTS doesn't have the bug because the VBUS supply is
already specified for the USB port and therefore it doesn't have to be
specified in the USB connector again. In fact, specifying it twice can
lead to a situation where the USB port may not be able to switch the
VBUS supply on or off because the setting might conflict with that of
the USB connector.

So unless there's a real reason why this is needed, I don't think this
should be applied.

Thierry

> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
> v2: remove unused variable "need_vbus"
> ---
>  drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
> index dfbbc4f51ed6..65d89140cd19 100644
> --- a/drivers/usb/common/usb-conn-gpio.c
> +++ b/drivers/usb/common/usb-conn-gpio.c
> @@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
>  		return;
>  	}
>  
> -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> +	if (info->last_role == USB_ROLE_HOST)
>  		regulator_disable(info->vbus);
>  
>  	ret = usb_role_switch_set_role(info->role_sw, role);
>  	if (ret)
>  		dev_err(info->dev, "failed to set role: %d\n", ret);
>  
> -	if (role == USB_ROLE_HOST && info->vbus) {
> +	if (role == USB_ROLE_HOST) {
>  		ret = regulator_enable(info->vbus);
>  		if (ret)
>  			dev_err(info->dev, "enable vbus regulator failed\n");
> @@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
>  
>  	info->last_role = role;
>  
> -	if (info->vbus)
> -		dev_dbg(info->dev, "vbus regulator is %s\n",
> -			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> +	dev_dbg(info->dev, "vbus regulator is %s\n",
> +		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
>  
>  	power_supply_changed(info->charger);
>  }
> @@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
>  	struct usb_conn_info *info;
> -	bool need_vbus = true;
>  	int ret = 0;
>  
>  	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> @@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
>  
>  	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
>  
> -	/*
> -	 * If the USB connector is a child of a USB port and that port already provides the VBUS
> -	 * supply, there's no need for the USB connector to provide it again.
> -	 */
> -	if (dev->parent && dev->parent->of_node) {
> -		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
> -			need_vbus = false;
> -	}
> -
> -	if (!need_vbus) {
> -		info->vbus = devm_regulator_get_optional(dev, "vbus");
> -		if (PTR_ERR(info->vbus) == -ENODEV)
> -			info->vbus = NULL;
> -	} else {
> -		info->vbus = devm_regulator_get(dev, "vbus");
> -	}
> -
> +	info->vbus = devm_regulator_get(dev, "vbus");
>  	if (IS_ERR(info->vbus)) {
>  		ret = PTR_ERR(info->vbus);
>  		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
> @@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
>  
>  	cancel_delayed_work_sync(&info->dw_det);
>  
> -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> +	if (info->last_role == USB_ROLE_HOST)
>  		regulator_disable(info->vbus);
>  
>  	usb_role_switch_put(info->role_sw);
> -- 
> 2.18.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Greg Kroah-Hartman]

On Fri, May 21, 2021 at 03:20:23PM +0200, Thierry Reding wrote:
> On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > Vbus is already an optional supply, if the vbus-supply is not
> > provided in DTS, will use a dummy regulator,
> 
> That statement is not entirely correct. The dummy regulator is
> substituted only if the supply is in fact not optional. The idea behind
> that is to allow DTS files that don't specify all required regulators to
> get away with it, based on the assumption that the supply is one of
> those always-on supplies that are often not described in DTS.
> 
> > the warning log is as below:
> > "supply vbus not found, using dummy regulator"
> 
> And the reason why we get that warning is to point out that the DTS has
> a bug and that it should be fixed (by adding a proper regulator to take
> the place of the dummy).
> 
> > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> 
> But if you read the description of that commit, the purpose of that
> patch was in fact to make the supply completely optional in the case
> where we already have the VBUS supply specified for the USB port that
> the connector is parented to.
> 
> So in that case the DTS doesn't have the bug because the VBUS supply is
> already specified for the USB port and therefore it doesn't have to be
> specified in the USB connector again. In fact, specifying it twice can
> lead to a situation where the USB port may not be able to switch the
> VBUS supply on or off because the setting might conflict with that of
> the USB connector.
> 
> So unless there's a real reason why this is needed, I don't think this
> should be applied.

I've dropped this from my tree now, thanks.

greg k-h

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Greg Kroah-Hartman]

On Fri, May 21, 2021 at 03:20:23PM +0200, Thierry Reding wrote:
> On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > Vbus is already an optional supply, if the vbus-supply is not
> > provided in DTS, will use a dummy regulator,
> 
> That statement is not entirely correct. The dummy regulator is
> substituted only if the supply is in fact not optional. The idea behind
> that is to allow DTS files that don't specify all required regulators to
> get away with it, based on the assumption that the supply is one of
> those always-on supplies that are often not described in DTS.
> 
> > the warning log is as below:
> > "supply vbus not found, using dummy regulator"
> 
> And the reason why we get that warning is to point out that the DTS has
> a bug and that it should be fixed (by adding a proper regulator to take
> the place of the dummy).
> 
> > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> 
> But if you read the description of that commit, the purpose of that
> patch was in fact to make the supply completely optional in the case
> where we already have the VBUS supply specified for the USB port that
> the connector is parented to.
> 
> So in that case the DTS doesn't have the bug because the VBUS supply is
> already specified for the USB port and therefore it doesn't have to be
> specified in the USB connector again. In fact, specifying it twice can
> lead to a situation where the USB port may not be able to switch the
> VBUS supply on or off because the setting might conflict with that of
> the USB connector.
> 
> So unless there's a real reason why this is needed, I don't think this
> should be applied.

I've dropped this from my tree now, thanks.

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Greg Kroah-Hartman]

On Fri, May 21, 2021 at 03:20:23PM +0200, Thierry Reding wrote:
> On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > Vbus is already an optional supply, if the vbus-supply is not
> > provided in DTS, will use a dummy regulator,
> 
> That statement is not entirely correct. The dummy regulator is
> substituted only if the supply is in fact not optional. The idea behind
> that is to allow DTS files that don't specify all required regulators to
> get away with it, based on the assumption that the supply is one of
> those always-on supplies that are often not described in DTS.
> 
> > the warning log is as below:
> > "supply vbus not found, using dummy regulator"
> 
> And the reason why we get that warning is to point out that the DTS has
> a bug and that it should be fixed (by adding a proper regulator to take
> the place of the dummy).
> 
> > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> 
> But if you read the description of that commit, the purpose of that
> patch was in fact to make the supply completely optional in the case
> where we already have the VBUS supply specified for the USB port that
> the connector is parented to.
> 
> So in that case the DTS doesn't have the bug because the VBUS supply is
> already specified for the USB port and therefore it doesn't have to be
> specified in the USB connector again. In fact, specifying it twice can
> lead to a situation where the USB port may not be able to switch the
> VBUS supply on or off because the setting might conflict with that of
> the USB connector.
> 
> So unless there's a real reason why this is needed, I don't think this
> should be applied.

I've dropped this from my tree now, thanks.

greg k-h

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > Vbus is already an optional supply, if the vbus-supply is not
> > provided in DTS, will use a dummy regulator,
> 
> That statement is not entirely correct. The dummy regulator is
> substituted only if the supply is in fact not optional. The idea behind
> that is to allow DTS files that don't specify all required regulators to
> get away with it, based on the assumption that the supply is one of
> those always-on supplies that are often not described in DTS.
Yes, you are right.
But from the point of result, it indeed can help to handle the absent
regulator.
> 
> > the warning log is as below:
> > "supply vbus not found, using dummy regulator"
> 
> And the reason why we get that warning is to point out that the DTS has
> a bug and that it should be fixed (by adding a proper regulator to take
> the place of the dummy).
> 
> > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> 
> But if you read the description of that commit, the purpose of that
> patch was in fact to make the supply completely optional in the case
> where we already have the VBUS supply specified for the USB port that
> the connector is parented to.
Could you please give an example you mentioned?

It seems prefer to provide vbus supply in connector instead of port
according to dt-binding

Thanks

> 
> So in that case the DTS doesn't have the bug because the VBUS supply is
> already specified for the USB port and therefore it doesn't have to be
> specified in the USB connector again. In fact, specifying it twice can
> lead to a situation where the USB port may not be able to switch the
> VBUS supply on or off because the setting might conflict with that of
> the USB connector.

> 
> So unless there's a real reason why this is needed, I don't think this
> should be applied.
> 
> Thierry
> 
> > Cc: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v2: remove unused variable "need_vbus"
> > ---
> >  drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
> >  1 file changed, 6 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
> > index dfbbc4f51ed6..65d89140cd19 100644
> > --- a/drivers/usb/common/usb-conn-gpio.c
> > +++ b/drivers/usb/common/usb-conn-gpio.c
> > @@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
> >  		return;
> >  	}
> >  
> > -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> > +	if (info->last_role == USB_ROLE_HOST)
> >  		regulator_disable(info->vbus);
> >  
> >  	ret = usb_role_switch_set_role(info->role_sw, role);
> >  	if (ret)
> >  		dev_err(info->dev, "failed to set role: %d\n", ret);
> >  
> > -	if (role == USB_ROLE_HOST && info->vbus) {
> > +	if (role == USB_ROLE_HOST) {
> >  		ret = regulator_enable(info->vbus);
> >  		if (ret)
> >  			dev_err(info->dev, "enable vbus regulator failed\n");
> > @@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
> >  
> >  	info->last_role = role;
> >  
> > -	if (info->vbus)
> > -		dev_dbg(info->dev, "vbus regulator is %s\n",
> > -			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> > +	dev_dbg(info->dev, "vbus regulator is %s\n",
> > +		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> >  
> >  	power_supply_changed(info->charger);
> >  }
> > @@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct usb_conn_info *info;
> > -	bool need_vbus = true;
> >  	int ret = 0;
> >  
> >  	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > @@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
> >  
> >  	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
> >  
> > -	/*
> > -	 * If the USB connector is a child of a USB port and that port already provides the VBUS
> > -	 * supply, there's no need for the USB connector to provide it again.
> > -	 */
> > -	if (dev->parent && dev->parent->of_node) {
> > -		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
> > -			need_vbus = false;
> > -	}
> > -
> > -	if (!need_vbus) {
> > -		info->vbus = devm_regulator_get_optional(dev, "vbus");
> > -		if (PTR_ERR(info->vbus) == -ENODEV)
> > -			info->vbus = NULL;
> > -	} else {
> > -		info->vbus = devm_regulator_get(dev, "vbus");
> > -	}
> > -
> > +	info->vbus = devm_regulator_get(dev, "vbus");
> >  	if (IS_ERR(info->vbus)) {
> >  		ret = PTR_ERR(info->vbus);
> >  		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
> > @@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
> >  
> >  	cancel_delayed_work_sync(&info->dw_det);
> >  
> > -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> > +	if (info->last_role == USB_ROLE_HOST)
> >  		regulator_disable(info->vbus);
> >  
> >  	usb_role_switch_put(info->role_sw);
> > -- 
> > 2.18.0
> > 

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > Vbus is already an optional supply, if the vbus-supply is not
> > provided in DTS, will use a dummy regulator,
> 
> That statement is not entirely correct. The dummy regulator is
> substituted only if the supply is in fact not optional. The idea behind
> that is to allow DTS files that don't specify all required regulators to
> get away with it, based on the assumption that the supply is one of
> those always-on supplies that are often not described in DTS.
Yes, you are right.
But from the point of result, it indeed can help to handle the absent
regulator.
> 
> > the warning log is as below:
> > "supply vbus not found, using dummy regulator"
> 
> And the reason why we get that warning is to point out that the DTS has
> a bug and that it should be fixed (by adding a proper regulator to take
> the place of the dummy).
> 
> > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> 
> But if you read the description of that commit, the purpose of that
> patch was in fact to make the supply completely optional in the case
> where we already have the VBUS supply specified for the USB port that
> the connector is parented to.
Could you please give an example you mentioned?

It seems prefer to provide vbus supply in connector instead of port
according to dt-binding

Thanks

> 
> So in that case the DTS doesn't have the bug because the VBUS supply is
> already specified for the USB port and therefore it doesn't have to be
> specified in the USB connector again. In fact, specifying it twice can
> lead to a situation where the USB port may not be able to switch the
> VBUS supply on or off because the setting might conflict with that of
> the USB connector.

> 
> So unless there's a real reason why this is needed, I don't think this
> should be applied.
> 
> Thierry
> 
> > Cc: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v2: remove unused variable "need_vbus"
> > ---
> >  drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
> >  1 file changed, 6 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
> > index dfbbc4f51ed6..65d89140cd19 100644
> > --- a/drivers/usb/common/usb-conn-gpio.c
> > +++ b/drivers/usb/common/usb-conn-gpio.c
> > @@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
> >  		return;
> >  	}
> >  
> > -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> > +	if (info->last_role == USB_ROLE_HOST)
> >  		regulator_disable(info->vbus);
> >  
> >  	ret = usb_role_switch_set_role(info->role_sw, role);
> >  	if (ret)
> >  		dev_err(info->dev, "failed to set role: %d\n", ret);
> >  
> > -	if (role == USB_ROLE_HOST && info->vbus) {
> > +	if (role == USB_ROLE_HOST) {
> >  		ret = regulator_enable(info->vbus);
> >  		if (ret)
> >  			dev_err(info->dev, "enable vbus regulator failed\n");
> > @@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
> >  
> >  	info->last_role = role;
> >  
> > -	if (info->vbus)
> > -		dev_dbg(info->dev, "vbus regulator is %s\n",
> > -			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> > +	dev_dbg(info->dev, "vbus regulator is %s\n",
> > +		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> >  
> >  	power_supply_changed(info->charger);
> >  }
> > @@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct usb_conn_info *info;
> > -	bool need_vbus = true;
> >  	int ret = 0;
> >  
> >  	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > @@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
> >  
> >  	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
> >  
> > -	/*
> > -	 * If the USB connector is a child of a USB port and that port already provides the VBUS
> > -	 * supply, there's no need for the USB connector to provide it again.
> > -	 */
> > -	if (dev->parent && dev->parent->of_node) {
> > -		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
> > -			need_vbus = false;
> > -	}
> > -
> > -	if (!need_vbus) {
> > -		info->vbus = devm_regulator_get_optional(dev, "vbus");
> > -		if (PTR_ERR(info->vbus) == -ENODEV)
> > -			info->vbus = NULL;
> > -	} else {
> > -		info->vbus = devm_regulator_get(dev, "vbus");
> > -	}
> > -
> > +	info->vbus = devm_regulator_get(dev, "vbus");
> >  	if (IS_ERR(info->vbus)) {
> >  		ret = PTR_ERR(info->vbus);
> >  		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
> > @@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
> >  
> >  	cancel_delayed_work_sync(&info->dw_det);
> >  
> > -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> > +	if (info->last_role == USB_ROLE_HOST)
> >  		regulator_disable(info->vbus);
> >  
> >  	usb_role_switch_put(info->role_sw);
> > -- 
> > 2.18.0
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > Vbus is already an optional supply, if the vbus-supply is not
> > provided in DTS, will use a dummy regulator,
> 
> That statement is not entirely correct. The dummy regulator is
> substituted only if the supply is in fact not optional. The idea behind
> that is to allow DTS files that don't specify all required regulators to
> get away with it, based on the assumption that the supply is one of
> those always-on supplies that are often not described in DTS.
Yes, you are right.
But from the point of result, it indeed can help to handle the absent
regulator.
> 
> > the warning log is as below:
> > "supply vbus not found, using dummy regulator"
> 
> And the reason why we get that warning is to point out that the DTS has
> a bug and that it should be fixed (by adding a proper regulator to take
> the place of the dummy).
> 
> > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> 
> But if you read the description of that commit, the purpose of that
> patch was in fact to make the supply completely optional in the case
> where we already have the VBUS supply specified for the USB port that
> the connector is parented to.
Could you please give an example you mentioned?

It seems prefer to provide vbus supply in connector instead of port
according to dt-binding

Thanks

> 
> So in that case the DTS doesn't have the bug because the VBUS supply is
> already specified for the USB port and therefore it doesn't have to be
> specified in the USB connector again. In fact, specifying it twice can
> lead to a situation where the USB port may not be able to switch the
> VBUS supply on or off because the setting might conflict with that of
> the USB connector.

> 
> So unless there's a real reason why this is needed, I don't think this
> should be applied.
> 
> Thierry
> 
> > Cc: Thierry Reding <treding@nvidia.com>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > ---
> > v2: remove unused variable "need_vbus"
> > ---
> >  drivers/usb/common/usb-conn-gpio.c | 30 ++++++------------------------
> >  1 file changed, 6 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/usb/common/usb-conn-gpio.c b/drivers/usb/common/usb-conn-gpio.c
> > index dfbbc4f51ed6..65d89140cd19 100644
> > --- a/drivers/usb/common/usb-conn-gpio.c
> > +++ b/drivers/usb/common/usb-conn-gpio.c
> > @@ -91,14 +91,14 @@ static void usb_conn_detect_cable(struct work_struct *work)
> >  		return;
> >  	}
> >  
> > -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> > +	if (info->last_role == USB_ROLE_HOST)
> >  		regulator_disable(info->vbus);
> >  
> >  	ret = usb_role_switch_set_role(info->role_sw, role);
> >  	if (ret)
> >  		dev_err(info->dev, "failed to set role: %d\n", ret);
> >  
> > -	if (role == USB_ROLE_HOST && info->vbus) {
> > +	if (role == USB_ROLE_HOST) {
> >  		ret = regulator_enable(info->vbus);
> >  		if (ret)
> >  			dev_err(info->dev, "enable vbus regulator failed\n");
> > @@ -106,9 +106,8 @@ static void usb_conn_detect_cable(struct work_struct *work)
> >  
> >  	info->last_role = role;
> >  
> > -	if (info->vbus)
> > -		dev_dbg(info->dev, "vbus regulator is %s\n",
> > -			regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> > +	dev_dbg(info->dev, "vbus regulator is %s\n",
> > +		regulator_is_enabled(info->vbus) ? "enabled" : "disabled");
> >  
> >  	power_supply_changed(info->charger);
> >  }
> > @@ -175,7 +174,6 @@ static int usb_conn_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> >  	struct usb_conn_info *info;
> > -	bool need_vbus = true;
> >  	int ret = 0;
> >  
> >  	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > @@ -205,23 +203,7 @@ static int usb_conn_probe(struct platform_device *pdev)
> >  
> >  	INIT_DELAYED_WORK(&info->dw_det, usb_conn_detect_cable);
> >  
> > -	/*
> > -	 * If the USB connector is a child of a USB port and that port already provides the VBUS
> > -	 * supply, there's no need for the USB connector to provide it again.
> > -	 */
> > -	if (dev->parent && dev->parent->of_node) {
> > -		if (of_find_property(dev->parent->of_node, "vbus-supply", NULL))
> > -			need_vbus = false;
> > -	}
> > -
> > -	if (!need_vbus) {
> > -		info->vbus = devm_regulator_get_optional(dev, "vbus");
> > -		if (PTR_ERR(info->vbus) == -ENODEV)
> > -			info->vbus = NULL;
> > -	} else {
> > -		info->vbus = devm_regulator_get(dev, "vbus");
> > -	}
> > -
> > +	info->vbus = devm_regulator_get(dev, "vbus");
> >  	if (IS_ERR(info->vbus)) {
> >  		ret = PTR_ERR(info->vbus);
> >  		return dev_err_probe(dev, ret, "failed to get vbus :%d\n", ret);
> > @@ -288,7 +270,7 @@ static int usb_conn_remove(struct platform_device *pdev)
> >  
> >  	cancel_delayed_work_sync(&info->dw_det);
> >  
> > -	if (info->last_role == USB_ROLE_HOST && info->vbus)
> > +	if (info->last_role == USB_ROLE_HOST)
> >  		regulator_disable(info->vbus);
> >  
> >  	usb_role_switch_put(info->role_sw);
> > -- 
> > 2.18.0
> > 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2105 bytes --]

On Mon, May 24, 2021 at 01:51:51PM +0800, Chunfeng Yun wrote:
> On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> > On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > > Vbus is already an optional supply, if the vbus-supply is not
> > > provided in DTS, will use a dummy regulator,
> > 
> > That statement is not entirely correct. The dummy regulator is
> > substituted only if the supply is in fact not optional. The idea behind
> > that is to allow DTS files that don't specify all required regulators to
> > get away with it, based on the assumption that the supply is one of
> > those always-on supplies that are often not described in DTS.
> Yes, you are right.
> But from the point of result, it indeed can help to handle the absent
> regulator.
> > 
> > > the warning log is as below:
> > > "supply vbus not found, using dummy regulator"
> > 
> > And the reason why we get that warning is to point out that the DTS has
> > a bug and that it should be fixed (by adding a proper regulator to take
> > the place of the dummy).
> > 
> > > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> > 
> > But if you read the description of that commit, the purpose of that
> > patch was in fact to make the supply completely optional in the case
> > where we already have the VBUS supply specified for the USB port that
> > the connector is parented to.
> Could you please give an example you mentioned?

You can find examples of this in these:

	arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
	arch/arm64/boot/dts/nvidia/tegra210-p3450-0000.dts
	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts

> It seems prefer to provide vbus supply in connector instead of port
> according to dt-binding

My recollection is that the above (or at least some of them) predate USB
connectors.

It's possible that we could convert the above to have the VBUS supply
listed in the connector instead of the port. However, since we have to
preserve backwards compatibility with older device trees, we can't
revert the commit anyway.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2105 bytes --]

On Mon, May 24, 2021 at 01:51:51PM +0800, Chunfeng Yun wrote:
> On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> > On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > > Vbus is already an optional supply, if the vbus-supply is not
> > > provided in DTS, will use a dummy regulator,
> > 
> > That statement is not entirely correct. The dummy regulator is
> > substituted only if the supply is in fact not optional. The idea behind
> > that is to allow DTS files that don't specify all required regulators to
> > get away with it, based on the assumption that the supply is one of
> > those always-on supplies that are often not described in DTS.
> Yes, you are right.
> But from the point of result, it indeed can help to handle the absent
> regulator.
> > 
> > > the warning log is as below:
> > > "supply vbus not found, using dummy regulator"
> > 
> > And the reason why we get that warning is to point out that the DTS has
> > a bug and that it should be fixed (by adding a proper regulator to take
> > the place of the dummy).
> > 
> > > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> > 
> > But if you read the description of that commit, the purpose of that
> > patch was in fact to make the supply completely optional in the case
> > where we already have the VBUS supply specified for the USB port that
> > the connector is parented to.
> Could you please give an example you mentioned?

You can find examples of this in these:

	arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
	arch/arm64/boot/dts/nvidia/tegra210-p3450-0000.dts
	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts

> It seems prefer to provide vbus supply in connector instead of port
> according to dt-binding

My recollection is that the above (or at least some of them) predate USB
connectors.

It's possible that we could convert the above to have the VBUS supply
listed in the connector instead of the port. However, since we have to
preserve backwards compatibility with older device trees, we can't
revert the commit anyway.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2105 bytes --]

On Mon, May 24, 2021 at 01:51:51PM +0800, Chunfeng Yun wrote:
> On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> > On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > > Vbus is already an optional supply, if the vbus-supply is not
> > > provided in DTS, will use a dummy regulator,
> > 
> > That statement is not entirely correct. The dummy regulator is
> > substituted only if the supply is in fact not optional. The idea behind
> > that is to allow DTS files that don't specify all required regulators to
> > get away with it, based on the assumption that the supply is one of
> > those always-on supplies that are often not described in DTS.
> Yes, you are right.
> But from the point of result, it indeed can help to handle the absent
> regulator.
> > 
> > > the warning log is as below:
> > > "supply vbus not found, using dummy regulator"
> > 
> > And the reason why we get that warning is to point out that the DTS has
> > a bug and that it should be fixed (by adding a proper regulator to take
> > the place of the dummy).
> > 
> > > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> > 
> > But if you read the description of that commit, the purpose of that
> > patch was in fact to make the supply completely optional in the case
> > where we already have the VBUS supply specified for the USB port that
> > the connector is parented to.
> Could you please give an example you mentioned?

You can find examples of this in these:

	arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
	arch/arm64/boot/dts/nvidia/tegra210-p3450-0000.dts
	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts

> It seems prefer to provide vbus supply in connector instead of port
> according to dt-binding

My recollection is that the above (or at least some of them) predate USB
connectors.

It's possible that we could convert the above to have the VBUS supply
listed in the connector instead of the port. However, since we have to
preserve backwards compatibility with older device trees, we can't
revert the commit anyway.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

On Tue, 2021-05-25 at 13:36 +0200, Thierry Reding wrote:
> On Mon, May 24, 2021 at 01:51:51PM +0800, Chunfeng Yun wrote:
> > On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> > > On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > > > Vbus is already an optional supply, if the vbus-supply is not
> > > > provided in DTS, will use a dummy regulator,
> > > 
> > > That statement is not entirely correct. The dummy regulator is
> > > substituted only if the supply is in fact not optional. The idea behind
> > > that is to allow DTS files that don't specify all required regulators to
> > > get away with it, based on the assumption that the supply is one of
> > > those always-on supplies that are often not described in DTS.
> > Yes, you are right.
> > But from the point of result, it indeed can help to handle the absent
> > regulator.
> > > 
> > > > the warning log is as below:
> > > > "supply vbus not found, using dummy regulator"
> > > 
> > > And the reason why we get that warning is to point out that the DTS has
> > > a bug and that it should be fixed (by adding a proper regulator to take
> > > the place of the dummy).
> > > 
> > > > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> > > 
> > > But if you read the description of that commit, the purpose of that
> > > patch was in fact to make the supply completely optional in the case
> > > where we already have the VBUS supply specified for the USB port that
> > > the connector is parented to.
> > Could you please give an example you mentioned?
> 
> You can find examples of this in these:
> 
> 	arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> 	arch/arm64/boot/dts/nvidia/tegra210-p3450-0000.dts
> 	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts
> 
> > It seems prefer to provide vbus supply in connector instead of port
> > according to dt-binding
> 
> My recollection is that the above (or at least some of them) predate USB
> connectors.
> 
> It's possible that we could convert the above to have the VBUS supply
> listed in the connector instead of the port. However, since we have to
> preserve backwards compatibility with older device trees, we can't
> revert the commit anyway.
Got it, thanks a lot

> 
> Thierry

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

On Tue, 2021-05-25 at 13:36 +0200, Thierry Reding wrote:
> On Mon, May 24, 2021 at 01:51:51PM +0800, Chunfeng Yun wrote:
> > On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> > > On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > > > Vbus is already an optional supply, if the vbus-supply is not
> > > > provided in DTS, will use a dummy regulator,
> > > 
> > > That statement is not entirely correct. The dummy regulator is
> > > substituted only if the supply is in fact not optional. The idea behind
> > > that is to allow DTS files that don't specify all required regulators to
> > > get away with it, based on the assumption that the supply is one of
> > > those always-on supplies that are often not described in DTS.
> > Yes, you are right.
> > But from the point of result, it indeed can help to handle the absent
> > regulator.
> > > 
> > > > the warning log is as below:
> > > > "supply vbus not found, using dummy regulator"
> > > 
> > > And the reason why we get that warning is to point out that the DTS has
> > > a bug and that it should be fixed (by adding a proper regulator to take
> > > the place of the dummy).
> > > 
> > > > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> > > 
> > > But if you read the description of that commit, the purpose of that
> > > patch was in fact to make the supply completely optional in the case
> > > where we already have the VBUS supply specified for the USB port that
> > > the connector is parented to.
> > Could you please give an example you mentioned?
> 
> You can find examples of this in these:
> 
> 	arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> 	arch/arm64/boot/dts/nvidia/tegra210-p3450-0000.dts
> 	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts
> 
> > It seems prefer to provide vbus supply in connector instead of port
> > according to dt-binding
> 
> My recollection is that the above (or at least some of them) predate USB
> connectors.
> 
> It's possible that we could convert the above to have the VBUS supply
> listed in the connector instead of the port. However, since we have to
> preserve backwards compatibility with older device trees, we can't
> revert the commit anyway.
Got it, thanks a lot

> 
> Thierry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] Revert "usb: common: usb-conn-gpio: Make VBUS supply optional"

[Chunfeng Yun]

On Tue, 2021-05-25 at 13:36 +0200, Thierry Reding wrote:
> On Mon, May 24, 2021 at 01:51:51PM +0800, Chunfeng Yun wrote:
> > On Fri, 2021-05-21 at 15:20 +0200, Thierry Reding wrote:
> > > On Wed, May 19, 2021 at 02:39:46PM +0800, Chunfeng Yun wrote:
> > > > Vbus is already an optional supply, if the vbus-supply is not
> > > > provided in DTS, will use a dummy regulator,
> > > 
> > > That statement is not entirely correct. The dummy regulator is
> > > substituted only if the supply is in fact not optional. The idea behind
> > > that is to allow DTS files that don't specify all required regulators to
> > > get away with it, based on the assumption that the supply is one of
> > > those always-on supplies that are often not described in DTS.
> > Yes, you are right.
> > But from the point of result, it indeed can help to handle the absent
> > regulator.
> > > 
> > > > the warning log is as below:
> > > > "supply vbus not found, using dummy regulator"
> > > 
> > > And the reason why we get that warning is to point out that the DTS has
> > > a bug and that it should be fixed (by adding a proper regulator to take
> > > the place of the dummy).
> > > 
> > > > This reverts commit 4ddf1ac79e5f082451cd549283d2eb7559ab6ca9.
> > > 
> > > But if you read the description of that commit, the purpose of that
> > > patch was in fact to make the supply completely optional in the case
> > > where we already have the VBUS supply specified for the USB port that
> > > the connector is parented to.
> > Could you please give an example you mentioned?
> 
> You can find examples of this in these:
> 
> 	arch/arm64/boot/dts/nvidia/tegra210-p2597.dtsi
> 	arch/arm64/boot/dts/nvidia/tegra210-p3450-0000.dts
> 	arch/arm64/boot/dts/nvidia/tegra186-p2771-0000.dts
> 
> > It seems prefer to provide vbus supply in connector instead of port
> > according to dt-binding
> 
> My recollection is that the above (or at least some of them) predate USB
> connectors.
> 
> It's possible that we could convert the above to have the VBUS supply
> listed in the connector instead of the port. However, since we have to
> preserve backwards compatibility with older device trees, we can't
> revert the commit anyway.
Got it, thanks a lot

> 
> Thierry

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek