Message-ID: <54E89E29.4010807@linaro.org>
Date: Sat, 21 Feb 2015 23:03:05 +0800
From: zhangfei <zhangfei.gao@linaro.org>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0
MIME-Version: 1.0
To: balbi@ti.com
CC: Kishon Vijay Abraham I <kishon@ti.com>, mark.rutland@arm.com,
        Peter Chen <peter.chen@freescale.com>,
        Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>,
        "dan . zhao" <dan.zhao@hisilicon.com>,
        Wangbinghui <wangbinghui@hisilicon.com>, linux-kernel@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
        linux-usb@vger.kernel.org
Subject: Re: [PATCH v4 4/4] phy: add phy-hi6220-usb
References: <1423726646-30336-1-git-send-email-zhangfei.gao@linaro.org> <1423726646-30336-5-git-send-email-zhangfei.gao@linaro.org> <20150220144119.GB1707@saruman.tx.rr.com> <54E75665.6050205@linaro.org> <20150220160610.GB6430@saruman.tx.rr.com>
In-Reply-To: <20150220160610.GB6430@saruman.tx.rr.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4016
Lines: 131

Hi, Balbi

On 02/21/2015 12:06 AM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 20, 2015 at 11:44:37PM +0800, zhangfei wrote:
>> Hi, Balbi
>>
>> On 02/20/2015 10:41 PM, Felipe Balbi wrote:
>>
>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>> +{
>>>> +	struct usb_otg *otg = priv->phy.otg;
>>>> +
>>>> +	if (!otg->gadget)
>>>> +		return;
>>>> +
>>>> +	if (on)
>>>> +		usb_gadget_connect(otg->gadget);
>>>> +	else
>>>> +		usb_gadget_disconnect(otg->gadget);
>>>
>>> why is the PHY fiddling with pullups ?
>>
>> We use this to enable/disable otg gadget mode.
>
> I got that, but the pullups don't belong to the PHY, they belong to the
> gadget.
>
>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>> host mode.
>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>
> all of that I understood clearly :-)
>
>> So when micro usb attached, we enable gadget mode; while micro usb
>> detached, we disable gadget mode, and dwc2 will automatically set to
>> host mode.
>
> that's all fine, I'm concerned about letting the PHY fiddle with
> something it doesn't own. If I am to change pullups rules in udc-core,
> this is likely to break down miserably and I don't want to have to go
> through that.

Thanks for the clarifying.

How about using usb_gadget_vbus_connect/disconnect, which are used in 
many files under drivers/usb/phy.
There is no vbus_session in dwc2/gadget.c, I thought it would be same as 
pullup.

However, usb_gadget_vbus_connect still need para gadget, where should we 
put this file, drivers/usb/phy or drivers/phy

>
>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>> +{
>>>> +	struct hi6220_priv *priv =
>>>> +		container_of(work, struct hi6220_priv, work.work);
>>>> +	int gpio_id, gpio_vbus;
>>>> +	enum usb_otg_state state;
>>>> +
>>>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>> +		return;
>>>> +
>>>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>
>>> looks like this should be using extcon
>> Not used extcon before.
>> However, we need gpio_vbus interrupt.
>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>> interrupt.
>> Will investigate tomorrow.
>
> drivers/extcon/extcon-gpio.c
I think there is no need to use extcon, gpio is clear enough.
extcon-gpio.c even do not support dt.

>
>>>> +	if (gpio_vbus == 0) {
>>>> +		if (gpio_id == 1)
>>>> +			state = OTG_STATE_B_PERIPHERAL;
>>>> +		else
>>>> +			state = OTG_STATE_A_HOST;
>>>> +	} else {
>>>> +		state = OTG_STATE_A_HOST;
>>>> +	}
>>>> +
>>>> +	if (priv->state != state) {
>>>> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
>>>> +		priv->state = state;
>>>> +	}
>>>> +}
>>>> +
>>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
>>>> +{
>>>> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
>>>> +
>>>> +	/* add debounce time */
>>>> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>>>
>>> this is really bad. We have threaded interrupt support, right ?
>>
>> Since we use two gpio to distinguish gadget mode or host mode.
>> Debounce time can introduce more accuracy.
>
> gpio_set_debounce() ?
Not all gpio.c support set_debounce, including gpio-pl061.c.

>
>> I think threaded interrupt can not be used for adding debounce time.
>> Here add debounce is just for safety.
>
> add the debounce to the gpio itself.

Here the debounce added only for safety.
gpio_id may mis-report when unplug usb, but it is correct for plug usb & 
otg device.
So debounce can be omitted.
If you think using delayed work for debounce is ugly, it is fine switch 
to threaded_irq.

Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/