From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kishon Vijay Abraham I Subject: Re: Re: [PATCH v4 1/2] phy-sun4i-usb: Add full support for usb0 phy / OTG Date: Thu, 11 Jun 2015 18:29:38 +0530 Message-ID: <5579863A.6000402@ti.com> References: <1433088626-8858-1-git-send-email-hdegoede@redhat.com> <1433088626-8858-2-git-send-email-hdegoede@redhat.com> <55795800.4070405@ti.com> <55795A98.8090303@redhat.com> <55796E23.90509@ti.com> <5579808C.2080208@redhat.com> Reply-To: kishon-l0cyMroinI0@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Return-path: In-Reply-To: <5579808C.2080208-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , To: Hans de Goede , Felipe Balbi Cc: Maxime Ripard , Chen-Yu Tsai , Roman Byshko , linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Id: devicetree@vger.kernel.org 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 >>>>> --- >>>>> 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(-) >>>>> >>>> . >>>> . >>>> >>>> . >>>> . >>>>> 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 @@ >>>> >>>> . >>>> . >>>> >>>> . >>>> . >>>>> 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