From: kishon@ti•com (Kishon Vijay Abraham I)
To: linux-arm-kernel@lists•infradead.org
Subject: [linux-sunxi] Re: [PATCH v4 1/2] phy-sun4i-usb: Add full support for usb0 phy / OTG
Date: Thu, 11 Jun 2015 18:29:38 +0530 [thread overview]
Message-ID: <5579863A.6000402@ti.com> (raw)
In-Reply-To: <5579808C.2080208@redhat.com>
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@redhat•com>
>>>>> ---
>>>>> 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
next prev parent reply other threads:[~2015-06-11 12:59 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-31 16:10 [PATCH v4 0/2] Remaining sunxi musb patches Hans de Goede
2015-05-31 16:10 ` [PATCH v4 1/2] phy-sun4i-usb: Add full support for usb0 phy / OTG Hans de Goede
2015-06-01 9:22 ` Maxime Ripard
2015-06-01 9:28 ` Hans de Goede
2015-06-02 15:22 ` Maxime Ripard
2015-06-11 5:48 ` Kishon Vijay Abraham I
2015-06-11 7:56 ` [linux-sunxi] " Hans de Goede
2015-06-11 8:28 ` Hans de Goede
2015-06-11 8:07 ` Chanwoo Choi
2015-06-11 8:21 ` [linux-sunxi] " Hans de Goede
2015-06-11 8:29 ` Chanwoo Choi
2015-06-11 8:59 ` Hans de Goede
2015-06-11 9:33 ` Chanwoo Choi
2015-06-11 9:37 ` Chanwoo Choi
2015-06-11 9:38 ` Hans de Goede
2015-06-11 10:30 ` Chanwoo Choi
2015-06-11 9:42 ` Kishon Vijay Abraham I
2015-06-11 9:53 ` Hans de Goede
2015-06-11 11:16 ` Kishon Vijay Abraham I
2015-06-11 12:35 ` [linux-sunxi] " Hans de Goede
2015-06-11 12:59 ` Kishon Vijay Abraham I [this message]
2015-07-15 10:55 ` Kishon Vijay Abraham I
2015-07-15 10:58 ` Kishon Vijay Abraham I
2015-05-31 16:10 ` [PATCH v4 2/2] musb: Add support for the Allwinner sunxi musb controller Hans de Goede
2015-06-02 15:02 ` Felipe Balbi
[not found] ` <79fc4306-a621-43b8-b347-e42a27beb134@googlegroups.com>
2015-06-01 18:34 ` [PATCH v4 0/2] Remaining sunxi musb patches Felipe Balbi
2015-06-02 7:14 ` Hans de Goede
2015-06-02 14:45 ` Felipe Balbi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5579863A.6000402@ti.com \
--to=kishon@ti$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox