linux-kernel - Re: [PATCH v4 4/4] phy: add phy-hi6220-usb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150220160610.GB6430@saruman.tx.rr.com>
Date:	Fri, 20 Feb 2015 10:06:10 -0600
From:	Felipe Balbi <balbi@...com>
To:	zhangfei <zhangfei.gao@...aro.org>
CC:	<balbi@...com>, Kishon Vijay Abraham I <kishon@...com>,
	<mark.rutland@....com>, Peter Chen <peter.chen@...escale.com>,
	Sergei Shtylyov <sergei.shtylyov@...entembedded.com>,
	"dan . zhao" <dan.zhao@...ilicon.com>,
	Wangbinghui <wangbinghui@...ilicon.com>,
	<linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<devicetree@...r.kernel.org>, <linux-usb@...r.kernel.org>
Subject: Re: [PATCH v4 4/4] phy: add phy-hi6220-usb

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.

> >>+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

> >>+	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() ?

> 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.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)