On Thursday 11 June 2015 06:05 PM, Hans de Goede wrote:
Hi,
On 11-06-15 13:16, Kishon Vijay Abraham I wrote:
Hi,
On Thursday 11 June 2015 03:23 PM, Hans de Goede wrote:
Hi,
On 11-06-15 11:42, Kishon Vijay Abraham I wrote:
Hi,
On Sunday 31 May 2015 09:40 PM, Hans de Goede wrote:
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 <hdegoede@xxxxxxxxxx>
---
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(-)
.
.
<snip>
.
.
diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index 91c5be4..b45d707 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -1,7 +1,7 @@
.
.
<snip>
.
.
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 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.
okay. Would be good to have this as a comment.
+
+ 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);
This shouldn't be needed since you already use devm_* in probe.
The use of devm_* in probe helps to keep the error handling in
probe sane, but we must explicitly free the irqs here, otherwise
they may trigger after this:
From whatever I know free irq will be invoked in remove if devm_request_irq is used in probe. I have to check though.
Yes it will be invoked, but after we've stopped our workqueue function,
and in the mean time the irq handler can trigger re-scheduling the
workqueue function.
All drivers using a workqueue explicitly free irqs on remove before
stopping the workqueue because of this reason. The auto freeing
of the irq happens after the remove function has been called, and
thus after the workqueue has been stopped/cancelled which is too
late.
Fine then. Thanks for that detail.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
