Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1387078 > unrolled thread

RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Jun Li <jun.li@nxp.com> - 2016-04-26 04:10 +0200
    Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-26 05:50 +0200
      RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Jun Li <jun.li@nxp.com> - 2016-04-26 07:20 +0200
        Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-26 08:30 +0200
          RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Jun Li <jun.li@nxp.com> - 2016-04-26 09:10 +0200
            Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-26 10:30 +0200
              Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-27 05:30 +0200
                Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-27 13:10 +0200
                  Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Peter Chen <hzpeterchen@gmail.com> - 2016-04-28 04:10 +0200
                    Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-28 10:10 +0200
    Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core Roger Quadros <rogerq@ti.com> - 2016-04-27 13:20 +0200

#1387078 — RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

Hi Roger

> -----Original Message-----
> From: Roger Quadros [mailto:rogerq@ti.com]
> Sent: Tuesday, April 05, 2016 10:05 PM
> To: stern@rowland.harvard.edu; balbi@kernel.org;
> gregkh@linuxfoundation.org; peter.chen@freescale.com
> Cc: dan.j.williams@intel.com; jun.li@freescale.com;
> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org; Roger Quadros
> <rogerq@ti.com>
> Subject: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> 
> It provides APIs for the following tasks
> 
> - Registering an OTG/dual-role capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine
> 
> Provide a dual-role device (DRD) state machine.
> DRD mode is a reduced functionality OTG mode. In this mode we don't
> support SRP, HNP and dynamic role-swap.
> 
> In DRD operation, the controller mode (Host or Peripheral) is decided
> based on the ID pin status. Once a cable plug (Type-A or Type-B) is
> attached the controller selects the state and doesn't change till the
> cable in unplugged and a different cable type is inserted.
> 
> As we don't need most of the complex OTG states and OTG timers we
> implement a lean DRD state machine in usb-otg.c.
> The DRD state machine is only interested in 2 hardware inputs 'id' and
> 'b_sess_vld'.
> 
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---

...

> +/**
> + * Register pending host/gadget and remove entry from wait list  */
> +static void usb_otg_flush_wait(struct device *otg_dev) {
> +	struct otg_wait_data *wait;
> +	struct otg_hcd *host;
> +	struct otg_gcd *gadget;
> +
> +	mutex_lock(&wait_list_mutex);
> +
> +	wait = usb_otg_get_wait(otg_dev);
> +	if (!wait)
> +		goto done;
> +
> +	dev_dbg(otg_dev, "otg: registering pending host/gadget\n");
> +	gadget = &wait->gcd;
> +	if (gadget)

If (gadget->gadget)

> +		usb_otg_register_gadget(gadget->gadget, gadget->ops);
> +
> +	host = &wait->primary_hcd;
> +	if (host->hcd)
> +		usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
> +				     host->ops);
> +
> +	host = &wait->shared_hcd;
> +	if (host->hcd)
> +		usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
> +				     host->ops);
> +
> +	list_del(&wait->list);
> +	kfree(wait);
> +
> +done:
> +	mutex_unlock(&wait_list_mutex);
> +}
> +
> +/**
> + * Check if the OTG device is in our OTG list and return
> + * usb_otg data, else NULL.
> + *
> + * otg_list_mutex must be held.
> + */
> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev) {
> +	struct usb_otg *otg;
> +
> +	if (!otg_dev)
> +		return NULL;
> +
> +	list_for_each_entry(otg, &otg_list, list) {
> +		if (otg->dev == otg_dev)
> +			return otg;
> +	}
> +
> +	return NULL;
> +}

Could you export it to be a public API, we may need access usb_otg
in common host driver for handling of enumeration of otg test device.

...

> +/**
> + * Called when entering a DRD state.
> + * fsm->lock must be held.
> + */
> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state
> +new_state) {
> +	struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
> +
> +	if (otg->state == new_state)
> +		return;
> +
> +	fsm->state_changed = 1;
> +	dev_dbg(otg->dev, "otg: set state: %s\n",
> +		usb_otg_state_string(new_state));
> +	switch (new_state) {
> +	case OTG_STATE_B_IDLE:
> +		drd_set_protocol(fsm, PROTO_UNDEF);
> +		otg_drv_vbus(otg, 0);
> +		break;
> +	case OTG_STATE_B_PERIPHERAL:
> +		drd_set_protocol(fsm, PROTO_GADGET);
> +		otg_drv_vbus(otg, 0);
> +		break;
> +	case OTG_STATE_A_HOST:
> +		drd_set_protocol(fsm, PROTO_HOST);
> +		otg_drv_vbus(otg, 1);
> +		break;
> +	case OTG_STATE_UNDEFINED:
> +	case OTG_STATE_B_SRP_INIT:
> +	case OTG_STATE_B_WAIT_ACON:
> +	case OTG_STATE_B_HOST:
> +	case OTG_STATE_A_IDLE:
> +	case OTG_STATE_A_WAIT_VRISE:
> +	case OTG_STATE_A_WAIT_BCON:
> +	case OTG_STATE_A_SUSPEND:
> +	case OTG_STATE_A_PERIPHERAL:
> +	case OTG_STATE_A_WAIT_VFALL:
> +	case OTG_STATE_A_VBUS_ERR:

Remove above unused states.

> +	default:
> +		dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
> +			 __func__, usb_otg_state_string(new_state));
> +		break;
> +	}
> +
> +	otg->state = new_state;
> +}
> +
> +/**
> + * DRD state change judgement
> + *
> + * For DRD we're only interested in some of the OTG states
> + * i.e. OTG_STATE_B_IDLE: both peripheral and host are stopped
> + *	OTG_STATE_B_PERIPHERAL: peripheral active
> + *	OTG_STATE_A_HOST: host active
> + * we're only interested in the following inputs
> + *	fsm->id, fsm->b_sess_vld
> + */
> +int drd_statemachine(struct usb_otg *otg) {
> +	struct otg_fsm *fsm = &otg->fsm;
> +	enum usb_otg_state state;
> +	int ret;
> +
> +	mutex_lock(&fsm->lock);
> +
> +	fsm->state_changed = 0;
> +	state = otg->state;
> +
> +	switch (state) {
> +	case OTG_STATE_UNDEFINED:
> +		if (!fsm->id)
> +			drd_set_state(fsm, OTG_STATE_A_HOST);
> +		else if (fsm->id && fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
> +		else
> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
> +		break;
> +	case OTG_STATE_B_IDLE:
> +		if (!fsm->id)
> +			drd_set_state(fsm, OTG_STATE_A_HOST);
> +		else if (fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
> +		break;
> +	case OTG_STATE_B_PERIPHERAL:
> +		if (!fsm->id)
> +			drd_set_state(fsm, OTG_STATE_A_HOST);
> +		else if (!fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
> +		break;
> +	case OTG_STATE_A_HOST:
> +		if (fsm->id && fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
> +		else if (fsm->id && !fsm->b_sess_vld)
> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
> +		break;
> +
> +	/* invalid states for DRD */
> +	case OTG_STATE_B_SRP_INIT:
> +	case OTG_STATE_B_WAIT_ACON:
> +	case OTG_STATE_B_HOST:
> +	case OTG_STATE_A_IDLE:
> +	case OTG_STATE_A_WAIT_VRISE:
> +	case OTG_STATE_A_WAIT_BCON:
> +	case OTG_STATE_A_SUSPEND:
> +	case OTG_STATE_A_PERIPHERAL:
> +	case OTG_STATE_A_WAIT_VFALL:
> +	case OTG_STATE_A_VBUS_ERR:

Remove above unused states and add a default:

> +		dev_err(otg->dev, "%s: otg: invalid usb-drd state: %s\n",
> +			__func__, usb_otg_state_string(state));
> +		drd_set_state(fsm, OTG_STATE_UNDEFINED);
> +	break;
> +	}
> +
> +	ret = fsm->state_changed;
> +	mutex_unlock(&fsm->lock);
> +	dev_dbg(otg->dev, "otg: quit statemachine, changed %d\n",
> +		fsm->state_changed);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(drd_statemachine);
> +
> +/**
> + * OTG FSM/DRD work function

DRD work function

> + */
> +static void usb_otg_work(struct work_struct *work) {

usb_drd_work() name is better as it's only for drd.

> +	struct usb_otg *otg = container_of(work, struct usb_otg, work);
> +
> +	pm_runtime_get_sync(otg->dev);
> +	drd_statemachine(otg);
> +	pm_runtime_put_sync(otg->dev);
> +}
> +
> +/**
> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> + * @dev: OTG/dual-role controller device.
> + * @config: OTG configuration.
> + *
> + * Registers the OTG/dual-role controller device with the USB OTG core.
> + *
> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> + */
> +struct usb_otg *usb_otg_register(struct device *dev,
> +				 struct usb_otg_config *config)
> +{
> +	struct usb_otg *otg;
> +	struct otg_wait_data *wait;
> +	int ret = 0;
> +
> +	if (!dev || !config || !config->fsm_ops)
> +		return ERR_PTR(-EINVAL);
> +
> +	/* already in list? */
> +	mutex_lock(&otg_list_mutex);
> +	if (usb_otg_get_data(dev)) {
> +		dev_err(dev, "otg: %s: device already in otg list\n",
> +			__func__);
> +		ret = -EINVAL;
> +		goto unlock;
> +	}
> +
> +	/* allocate and add to list */
> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> +	if (!otg) {
> +		ret = -ENOMEM;
> +		goto unlock;
> +	}
> +
> +	otg->dev = dev;
> +	otg->caps = config->otg_caps;
> +
> +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> +	     otg->caps->adp_support) && !config->otg_work)
> +		dev_info(dev, "otg: limiting to dual-role\n");

dev_err, this should be an error.

> +
> +	if (config->otg_work)	/* custom otg_work ? */
> +		INIT_WORK(&otg->work, config->otg_work);
> +	else
> +		INIT_WORK(&otg->work, usb_otg_work);
> +
> +	otg->wq = create_singlethread_workqueue("usb_otg");
> +	if (!otg->wq) {
> +		dev_err(dev, "otg: %s: can't create workqueue\n",
> +			__func__);
> +		ret = -ENOMEM;
> +		goto err_wq;
> +	}
> +
> +	/* set otg ops */
> +	otg->fsm.ops = config->fsm_ops;
> +
> +	mutex_init(&otg->fsm.lock);
> +
> +	list_add_tail(&otg->list, &otg_list);
> +	mutex_unlock(&otg_list_mutex);
> +
> +	/* were we in wait list? */
> +	mutex_lock(&wait_list_mutex);
> +	wait = usb_otg_get_wait(dev);
> +	mutex_unlock(&wait_list_mutex);
> +	if (wait) {
> +		/* register pending host/gadget and flush from list */
> +		usb_otg_flush_wait(dev);
> +	}
> +
> +	return otg;
> +
> +err_wq:
> +	kfree(otg);
> +unlock:
> +	mutex_unlock(&otg_list_mutex);
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register);
> +

[toc] | [next] | [standalone]

#1387126

On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> > +struct usb_otg *usb_otg_register(struct device *dev,
> > +				 struct usb_otg_config *config)
> > +{
> > +	struct usb_otg *otg;
> > +	struct otg_wait_data *wait;
> > +	int ret = 0;
> > +
> > +	if (!dev || !config || !config->fsm_ops)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	/* already in list? */
> > +	mutex_lock(&otg_list_mutex);
> > +	if (usb_otg_get_data(dev)) {
> > +		dev_err(dev, "otg: %s: device already in otg list\n",
> > +			__func__);
> > +		ret = -EINVAL;
> > +		goto unlock;
> > +	}
> > +
> > +	/* allocate and add to list */
> > +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > +	if (!otg) {
> > +		ret = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +
> > +	otg->dev = dev;
> > +	otg->caps = config->otg_caps;
> > +
> > +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > +	     otg->caps->adp_support) && !config->otg_work)
> > +		dev_info(dev, "otg: limiting to dual-role\n");
> 
> dev_err, this should be an error.

The condition may be wrong, but it is an information to show
that current OTG is dual-role.

Peter
> 
> > +
> > +	if (config->otg_work)	/* custom otg_work ? */
> > +		INIT_WORK(&otg->work, config->otg_work);
> > +	else
> > +		INIT_WORK(&otg->work, usb_otg_work);
> > +
> > +	otg->wq = create_singlethread_workqueue("usb_otg");
> > +	if (!otg->wq) {
> > +		dev_err(dev, "otg: %s: can't create workqueue\n",
> > +			__func__);
> > +		ret = -ENOMEM;
> > +		goto err_wq;
> > +	}
> > +
> > +	/* set otg ops */
> > +	otg->fsm.ops = config->fsm_ops;
> > +
> > +	mutex_init(&otg->fsm.lock);
> > +
> > +	list_add_tail(&otg->list, &otg_list);
> > +	mutex_unlock(&otg_list_mutex);
> > +
> > +	/* were we in wait list? */
> > +	mutex_lock(&wait_list_mutex);
> > +	wait = usb_otg_get_wait(dev);
> > +	mutex_unlock(&wait_list_mutex);
> > +	if (wait) {
> > +		/* register pending host/gadget and flush from list */
> > +		usb_otg_flush_wait(dev);
> > +	}
> > +
> > +	return otg;
> > +
> > +err_wq:
> > +	kfree(otg);
> > +unlock:
> > +	mutex_unlock(&otg_list_mutex);
> > +	return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(usb_otg_register);
> > +
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1387137

Hi

> -----Original Message-----
> From: Peter Chen [mailto:hzpeterchen@gmail.com]
> Sent: Tuesday, April 26, 2016 11:47 AM
> To: Jun Li <jun.li@nxp.com>
> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
> dan.j.williams@intel.com; jun.li@freescale.com;
> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> 
> On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > +				 struct usb_otg_config *config) {
> > > +	struct usb_otg *otg;
> > > +	struct otg_wait_data *wait;
> > > +	int ret = 0;
> > > +
> > > +	if (!dev || !config || !config->fsm_ops)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	/* already in list? */
> > > +	mutex_lock(&otg_list_mutex);
> > > +	if (usb_otg_get_data(dev)) {
> > > +		dev_err(dev, "otg: %s: device already in otg list\n",
> > > +			__func__);
> > > +		ret = -EINVAL;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	/* allocate and add to list */
> > > +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > +	if (!otg) {
> > > +		ret = -ENOMEM;
> > > +		goto unlock;
> > > +	}
> > > +
> > > +	otg->dev = dev;
> > > +	otg->caps = config->otg_caps;
> > > +
> > > +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > +	     otg->caps->adp_support) && !config->otg_work)
> > > +		dev_info(dev, "otg: limiting to dual-role\n");
> >
> > dev_err, this should be an error.
> 
> The condition may be wrong, but it is an information to show that current
> OTG is dual-role.

This should not happen in any correct design, I even doubt if we
should try to continue by "downgrade" it to be duel role, currently
the only example user is dual role, so doing like this can't be
tested by real case, this downgrade is not so easy like we image,
at least for chipidea otg driver, simply replace a queue worker may
not work, as we have much more difference between the 2 configs.

Li Jun
 
> 
> Peter
> >
> > > +
> > > +	if (config->otg_work)	/* custom otg_work ? */
> > > +		INIT_WORK(&otg->work, config->otg_work);
> > > +	else
> > > +		INIT_WORK(&otg->work, usb_otg_work);
> > > +
> > > +	otg->wq = create_singlethread_workqueue("usb_otg");
> > > +	if (!otg->wq) {
> > > +		dev_err(dev, "otg: %s: can't create workqueue\n",
> > > +			__func__);
> > > +		ret = -ENOMEM;
> > > +		goto err_wq;
> > > +	}
> > > +
> > > +	/* set otg ops */
> > > +	otg->fsm.ops = config->fsm_ops;
> > > +
> > > +	mutex_init(&otg->fsm.lock);
> > > +
> > > +	list_add_tail(&otg->list, &otg_list);
> > > +	mutex_unlock(&otg_list_mutex);
> > > +
> > > +	/* were we in wait list? */
> > > +	mutex_lock(&wait_list_mutex);
> > > +	wait = usb_otg_get_wait(dev);
> > > +	mutex_unlock(&wait_list_mutex);
> > > +	if (wait) {
> > > +		/* register pending host/gadget and flush from list */
> > > +		usb_otg_flush_wait(dev);
> > > +	}
> > > +
> > > +	return otg;
> > > +
> > > +err_wq:
> > > +	kfree(otg);
> > > +unlock:
> > > +	mutex_unlock(&otg_list_mutex);
> > > +	return ERR_PTR(ret);
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_otg_register);
> > > +
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> > in the body of a message to majordomo@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> Best Regards,
> Peter Chen

[toc] | [prev] | [next] | [standalone]

#1387158

On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
> Hi
> 
> > -----Original Message-----
> > From: Peter Chen [mailto:hzpeterchen@gmail.com]
> > Sent: Tuesday, April 26, 2016 11:47 AM
> > To: Jun Li <jun.li@nxp.com>
> > Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> > balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
> > dan.j.williams@intel.com; jun.li@freescale.com;
> > mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
> > abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > 
> > On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > +				 struct usb_otg_config *config) {
> > > > +	struct usb_otg *otg;
> > > > +	struct otg_wait_data *wait;
> > > > +	int ret = 0;
> > > > +
> > > > +	if (!dev || !config || !config->fsm_ops)
> > > > +		return ERR_PTR(-EINVAL);
> > > > +
> > > > +	/* already in list? */
> > > > +	mutex_lock(&otg_list_mutex);
> > > > +	if (usb_otg_get_data(dev)) {
> > > > +		dev_err(dev, "otg: %s: device already in otg list\n",
> > > > +			__func__);
> > > > +		ret = -EINVAL;
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	/* allocate and add to list */
> > > > +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > +	if (!otg) {
> > > > +		ret = -ENOMEM;
> > > > +		goto unlock;
> > > > +	}
> > > > +
> > > > +	otg->dev = dev;
> > > > +	otg->caps = config->otg_caps;
> > > > +
> > > > +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > +	     otg->caps->adp_support) && !config->otg_work)
> > > > +		dev_info(dev, "otg: limiting to dual-role\n");
> > >
> > > dev_err, this should be an error.
> > 
> > The condition may be wrong, but it is an information to show that current
> > OTG is dual-role.
> 
> This should not happen in any correct design, I even doubt if we
> should try to continue by "downgrade" it to be duel role, currently
> the only example user is dual role, so doing like this can't be
> tested by real case, this downgrade is not so easy like we image,
> at least for chipidea otg driver, simply replace a queue worker may
> not work, as we have much more difference between the 2 configs.
> 

Would you show more why chipidea can't work just replace the work item,
and see if anything we still can improve for this framework?

-- 
Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1387179

Hi

> -----Original Message-----
> From: Peter Chen [mailto:hzpeterchen@gmail.com]
> Sent: Tuesday, April 26, 2016 2:28 PM
> To: Jun Li <jun.li@nxp.com>
> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
> dan.j.williams@intel.com; jun.li@freescale.com;
> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> 
> On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
> > Hi
> >
> > > -----Original Message-----
> > > From: Peter Chen [mailto:hzpeterchen@gmail.com]
> > > Sent: Tuesday, April 26, 2016 11:47 AM
> > > To: Jun Li <jun.li@nxp.com>
> > > Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> > > balbi@kernel.org; gregkh@linuxfoundation.org;
> > > peter.chen@freescale.com; dan.j.williams@intel.com;
> > > jun.li@freescale.com; mathias.nyman@linux.intel.com;
> > > tony@atomide.com; Joao.Pinto@synopsys.com; abrestic@chromium.org;
> > > r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > >
> > > On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> > > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > > +				 struct usb_otg_config *config) {
> > > > > +	struct usb_otg *otg;
> > > > > +	struct otg_wait_data *wait;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	if (!dev || !config || !config->fsm_ops)
> > > > > +		return ERR_PTR(-EINVAL);
> > > > > +
> > > > > +	/* already in list? */
> > > > > +	mutex_lock(&otg_list_mutex);
> > > > > +	if (usb_otg_get_data(dev)) {
> > > > > +		dev_err(dev, "otg: %s: device already in otg list\n",
> > > > > +			__func__);
> > > > > +		ret = -EINVAL;
> > > > > +		goto unlock;
> > > > > +	}
> > > > > +
> > > > > +	/* allocate and add to list */
> > > > > +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > > +	if (!otg) {
> > > > > +		ret = -ENOMEM;
> > > > > +		goto unlock;
> > > > > +	}
> > > > > +
> > > > > +	otg->dev = dev;
> > > > > +	otg->caps = config->otg_caps;
> > > > > +
> > > > > +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > > +	     otg->caps->adp_support) && !config->otg_work)
> > > > > +		dev_info(dev, "otg: limiting to dual-role\n");
> > > >
> > > > dev_err, this should be an error.
> > >
> > > The condition may be wrong, but it is an information to show that
> > > current OTG is dual-role.
> >
> > This should not happen in any correct design, I even doubt if we
> > should try to continue by "downgrade" it to be duel role, currently
> > the only example user is dual role, so doing like this can't be tested
> > by real case, this downgrade is not so easy like we image, at least
> > for chipidea otg driver, simply replace a queue worker may not work,
> > as we have much more difference between the 2 configs.
> >
> 
> Would you show more why chipidea can't work just replace the work item,
> and see if anything we still can improve for this framework?

In real OTG, we need enable AVV irq, but for duel role, nobody care/handle,
there are much more resource required for OTG: timers, hnp polling,
otg test device handling... 

with current design, chipidea driver can support real OTG with its own
queue worker, or DRD with Roger's drd work item if config is correct.

But improve something to work on a *wrong* config will make it complicated
and does not make much sense IMO.

Li Jun
> 
> --
> Best Regards,
> Peter Chen

[toc] | [prev] | [next] | [standalone]

#1387223

On Tue, Apr 26, 2016 at 07:00:22AM +0000, Jun Li wrote:
> Hi
> 
> > -----Original Message-----
> > From: Peter Chen [mailto:hzpeterchen@gmail.com]
> > Sent: Tuesday, April 26, 2016 2:28 PM
> > To: Jun Li <jun.li@nxp.com>
> > Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> > balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
> > dan.j.williams@intel.com; jun.li@freescale.com;
> > mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
> > abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > 
> > On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
> > > Hi
> > >
> > > > -----Original Message-----
> > > > From: Peter Chen [mailto:hzpeterchen@gmail.com]
> > > > Sent: Tuesday, April 26, 2016 11:47 AM
> > > > To: Jun Li <jun.li@nxp.com>
> > > > Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> > > > balbi@kernel.org; gregkh@linuxfoundation.org;
> > > > peter.chen@freescale.com; dan.j.williams@intel.com;
> > > > jun.li@freescale.com; mathias.nyman@linux.intel.com;
> > > > tony@atomide.com; Joao.Pinto@synopsys.com; abrestic@chromium.org;
> > > > r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> > > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > > >
> > > > On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> > > > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > > > +				 struct usb_otg_config *config) {
> > > > > > +	struct usb_otg *otg;
> > > > > > +	struct otg_wait_data *wait;
> > > > > > +	int ret = 0;
> > > > > > +
> > > > > > +	if (!dev || !config || !config->fsm_ops)
> > > > > > +		return ERR_PTR(-EINVAL);
> > > > > > +
> > > > > > +	/* already in list? */
> > > > > > +	mutex_lock(&otg_list_mutex);
> > > > > > +	if (usb_otg_get_data(dev)) {
> > > > > > +		dev_err(dev, "otg: %s: device already in otg list\n",
> > > > > > +			__func__);
> > > > > > +		ret = -EINVAL;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	/* allocate and add to list */
> > > > > > +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > > > +	if (!otg) {
> > > > > > +		ret = -ENOMEM;
> > > > > > +		goto unlock;
> > > > > > +	}
> > > > > > +
> > > > > > +	otg->dev = dev;
> > > > > > +	otg->caps = config->otg_caps;
> > > > > > +
> > > > > > +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > > > +	     otg->caps->adp_support) && !config->otg_work)
> > > > > > +		dev_info(dev, "otg: limiting to dual-role\n");
> > > > >
> > > > > dev_err, this should be an error.
> > > >
> > > > The condition may be wrong, but it is an information to show that
> > > > current OTG is dual-role.
> > >
> > > This should not happen in any correct design, I even doubt if we
> > > should try to continue by "downgrade" it to be duel role, currently
> > > the only example user is dual role, so doing like this can't be tested
> > > by real case, this downgrade is not so easy like we image, at least
> > > for chipidea otg driver, simply replace a queue worker may not work,
> > > as we have much more difference between the 2 configs.
> > >
> > 
> > Would you show more why chipidea can't work just replace the work item,
> > and see if anything we still can improve for this framework?
> 
> In real OTG, we need enable AVV irq,

Enable and Handling AVV is platform stuff. In this framework, we are
focus on how otg device manages host and gadget together, and the state
machine when the related otg event occurs.

> but for duel role, nobody care/handle,
> there are much more resource required for OTG: timers, hnp polling,
> otg test device handling... 

They are common things for fully OTG fsm, you can move them
to common code (In fact, hnp polling handling is already common code).

> 
> with current design, chipidea driver can support real OTG with its own
> queue worker, or DRD with Roger's drd work item if config is correct.
> 
> But improve something to work on a *wrong* config will make it complicated
> and does not make much sense IMO.
> 

What does above "config" you mean?

If the configure is fully OTG, you can choose different state machine,
eg otg_statemachine, if you find it is hard for chipidea to use this
framework, just list the reason, and see if we can improve.

-- 
Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1388631

On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
> On Tue, Apr 26, 2016 at 07:00:22AM +0000, Jun Li wrote:
> > Hi
> > 
> > > -----Original Message-----
> > > From: Peter Chen [mailto:hzpeterchen@gmail.com]
> > > Sent: Tuesday, April 26, 2016 2:28 PM
> > > To: Jun Li <jun.li@nxp.com>
> > > Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> > > balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
> > > dan.j.williams@intel.com; jun.li@freescale.com;
> > > mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
> > > abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > > 
> > > On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
> > > > Hi
> > > >
> > > > > -----Original Message-----
> > > > > From: Peter Chen [mailto:hzpeterchen@gmail.com]
> > > > > Sent: Tuesday, April 26, 2016 11:47 AM
> > > > > To: Jun Li <jun.li@nxp.com>
> > > > > Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> > > > > balbi@kernel.org; gregkh@linuxfoundation.org;
> > > > > peter.chen@freescale.com; dan.j.williams@intel.com;
> > > > > jun.li@freescale.com; mathias.nyman@linux.intel.com;
> > > > > tony@atomide.com; Joao.Pinto@synopsys.com; abrestic@chromium.org;
> > > > > r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> > > > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > > > >
> > > > > On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> > > > > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > > > > +				 struct usb_otg_config *config) {
> > > > > > > +	struct usb_otg *otg;
> > > > > > > +	struct otg_wait_data *wait;
> > > > > > > +	int ret = 0;
> > > > > > > +
> > > > > > > +	if (!dev || !config || !config->fsm_ops)
> > > > > > > +		return ERR_PTR(-EINVAL);
> > > > > > > +
> > > > > > > +	/* already in list? */
> > > > > > > +	mutex_lock(&otg_list_mutex);
> > > > > > > +	if (usb_otg_get_data(dev)) {
> > > > > > > +		dev_err(dev, "otg: %s: device already in otg list\n",
> > > > > > > +			__func__);
> > > > > > > +		ret = -EINVAL;
> > > > > > > +		goto unlock;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	/* allocate and add to list */
> > > > > > > +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > > > > +	if (!otg) {
> > > > > > > +		ret = -ENOMEM;
> > > > > > > +		goto unlock;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	otg->dev = dev;
> > > > > > > +	otg->caps = config->otg_caps;
> > > > > > > +
> > > > > > > +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > > > > +	     otg->caps->adp_support) && !config->otg_work)
> > > > > > > +		dev_info(dev, "otg: limiting to dual-role\n");
> > > > > >
> > > > > > dev_err, this should be an error.
> > > > >
> > > > > The condition may be wrong, but it is an information to show that
> > > > > current OTG is dual-role.
> > > >
> > > > This should not happen in any correct design, I even doubt if we
> > > > should try to continue by "downgrade" it to be duel role, currently
> > > > the only example user is dual role, so doing like this can't be tested
> > > > by real case, this downgrade is not so easy like we image, at least
> > > > for chipidea otg driver, simply replace a queue worker may not work,
> > > > as we have much more difference between the 2 configs.
> > > >
> > > 
> > > Would you show more why chipidea can't work just replace the work item,
> > > and see if anything we still can improve for this framework?
> > 
> > In real OTG, we need enable AVV irq,
> 
> Enable and Handling AVV is platform stuff. In this framework, we are
> focus on how otg device manages host and gadget together, and the state
> machine when the related otg event occurs.
> 
> > but for duel role, nobody care/handle,
> > there are much more resource required for OTG: timers, hnp polling,
> > otg test device handling... 
> 
> They are common things for fully OTG fsm, you can move them
> to common code (In fact, hnp polling handling is already common code).
> 
> > 
> > with current design, chipidea driver can support real OTG with its own
> > queue worker, or DRD with Roger's drd work item if config is correct.
> > 
> > But improve something to work on a *wrong* config will make it complicated
> > and does not make much sense IMO.
> > 
> 
> What does above "config" you mean?
> 
> If the configure is fully OTG, you can choose different state machine,
> eg otg_statemachine, if you find it is hard for chipidea to use this
> framework, just list the reason, and see if we can improve.
> 

Roger, after discussing with Jun off line, we think usb_otg_register
should return -ENOTSUPP if platform is OTG capabilities (HNP || SRP ||
ADP), since this patch set does not cover fully otg features, the users
should not be confused when try to implement fully otg using this
framework.

Later, after your patch set is merged, we can add fully OTG features
using this framework, and remove this check.

What's your opinion?

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1388896

Hi,

On 27/04/16 06:15, Peter Chen wrote:
> On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
>> On Tue, Apr 26, 2016 at 07:00:22AM +0000, Jun Li wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Peter Chen [mailto:hzpeterchen@gmail.com]
>>>> Sent: Tuesday, April 26, 2016 2:28 PM
>>>> To: Jun Li <jun.li@nxp.com>
>>>> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
>>>> balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
>>>> dan.j.williams@intel.com; jun.li@freescale.com;
>>>> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
>>>> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>
>>>> On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Peter Chen [mailto:hzpeterchen@gmail.com]
>>>>>> Sent: Tuesday, April 26, 2016 11:47 AM
>>>>>> To: Jun Li <jun.li@nxp.com>
>>>>>> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
>>>>>> balbi@kernel.org; gregkh@linuxfoundation.org;
>>>>>> peter.chen@freescale.com; dan.j.williams@intel.com;
>>>>>> jun.li@freescale.com; mathias.nyman@linux.intel.com;
>>>>>> tony@atomide.com; Joao.Pinto@synopsys.com; abrestic@chromium.org;
>>>>>> r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
>>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>>>
>>>>>> On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
>>>>>>>> +struct usb_otg *usb_otg_register(struct device *dev,
>>>>>>>> +				 struct usb_otg_config *config) {
>>>>>>>> +	struct usb_otg *otg;
>>>>>>>> +	struct otg_wait_data *wait;
>>>>>>>> +	int ret = 0;
>>>>>>>> +
>>>>>>>> +	if (!dev || !config || !config->fsm_ops)
>>>>>>>> +		return ERR_PTR(-EINVAL);
>>>>>>>> +
>>>>>>>> +	/* already in list? */
>>>>>>>> +	mutex_lock(&otg_list_mutex);
>>>>>>>> +	if (usb_otg_get_data(dev)) {
>>>>>>>> +		dev_err(dev, "otg: %s: device already in otg list\n",
>>>>>>>> +			__func__);
>>>>>>>> +		ret = -EINVAL;
>>>>>>>> +		goto unlock;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	/* allocate and add to list */
>>>>>>>> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>>>>>>>> +	if (!otg) {
>>>>>>>> +		ret = -ENOMEM;
>>>>>>>> +		goto unlock;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	otg->dev = dev;
>>>>>>>> +	otg->caps = config->otg_caps;
>>>>>>>> +
>>>>>>>> +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
>>>>>>>> +	     otg->caps->adp_support) && !config->otg_work)
>>>>>>>> +		dev_info(dev, "otg: limiting to dual-role\n");
>>>>>>>
>>>>>>> dev_err, this should be an error.
>>>>>>
>>>>>> The condition may be wrong, but it is an information to show that
>>>>>> current OTG is dual-role.
>>>>>
>>>>> This should not happen in any correct design, I even doubt if we
>>>>> should try to continue by "downgrade" it to be duel role, currently
>>>>> the only example user is dual role, so doing like this can't be tested
>>>>> by real case, this downgrade is not so easy like we image, at least
>>>>> for chipidea otg driver, simply replace a queue worker may not work,
>>>>> as we have much more difference between the 2 configs.
>>>>>
>>>>
>>>> Would you show more why chipidea can't work just replace the work item,
>>>> and see if anything we still can improve for this framework?
>>>
>>> In real OTG, we need enable AVV irq,
>>
>> Enable and Handling AVV is platform stuff. In this framework, we are
>> focus on how otg device manages host and gadget together, and the state
>> machine when the related otg event occurs.
>>
>>> but for duel role, nobody care/handle,
>>> there are much more resource required for OTG: timers, hnp polling,
>>> otg test device handling... 
>>
>> They are common things for fully OTG fsm, you can move them
>> to common code (In fact, hnp polling handling is already common code).
>>
>>>
>>> with current design, chipidea driver can support real OTG with its own
>>> queue worker, or DRD with Roger's drd work item if config is correct.
>>>
>>> But improve something to work on a *wrong* config will make it complicated
>>> and does not make much sense IMO.
>>>
>>
>> What does above "config" you mean?
>>
>> If the configure is fully OTG, you can choose different state machine,
>> eg otg_statemachine, if you find it is hard for chipidea to use this
>> framework, just list the reason, and see if we can improve.
>>
> 
> Roger, after discussing with Jun off line, we think usb_otg_register
> should return -ENOTSUPP if platform is OTG capabilities (HNP || SRP ||
> ADP), since this patch set does not cover fully otg features, the users

But this series isn't preventing full otg implementation. You can
still do that via config->otg_work.

I can modify the following condition to return -ENOTSUPP instead of
defaulting to dual-role

struct usb_otg *usb_otg_register(...)
{
...
        if ((otg->caps->hnp_support || otg->caps->srp_support ||
             otg->caps->adp_support) && !config->otg_work) {
		dev_err(dev, "otg: otg_work must be provided for OTG support\n");
		return -ENOTSUPP;
	}
...
}

Is this sufficient?

> should not be confused when try to implement fully otg using this
> framework.
> 
> Later, after your patch set is merged, we can add fully OTG features
> using this framework, and remove this check.
> 
> What's your opinion?
> 

--
cheers,
-roger

[toc] | [prev] | [next] | [standalone]

#1389767

On Wed, Apr 27, 2016 at 01:59:44PM +0300, Roger Quadros wrote:
> Hi,
> 
> On 27/04/16 06:15, Peter Chen wrote:
> > On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
> >> On Tue, Apr 26, 2016 at 07:00:22AM +0000, Jun Li wrote:
> >>> Hi
> >>>
> >>>> -----Original Message-----
> >>>> From: Peter Chen [mailto:hzpeterchen@gmail.com]
> >>>> Sent: Tuesday, April 26, 2016 2:28 PM
> >>>> To: Jun Li <jun.li@nxp.com>
> >>>> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> >>>> balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
> >>>> dan.j.williams@intel.com; jun.li@freescale.com;
> >>>> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
> >>>> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> >>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> >>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> >>>>
> >>>> On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
> >>>>> Hi
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Peter Chen [mailto:hzpeterchen@gmail.com]
> >>>>>> Sent: Tuesday, April 26, 2016 11:47 AM
> >>>>>> To: Jun Li <jun.li@nxp.com>
> >>>>>> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
> >>>>>> balbi@kernel.org; gregkh@linuxfoundation.org;
> >>>>>> peter.chen@freescale.com; dan.j.williams@intel.com;
> >>>>>> jun.li@freescale.com; mathias.nyman@linux.intel.com;
> >>>>>> tony@atomide.com; Joao.Pinto@synopsys.com; abrestic@chromium.org;
> >>>>>> r.baldyga@samsung.com; linux-usb@vger.kernel.org;
> >>>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
> >>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> >>>>>>
> >>>>>> On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
> >>>>>>>> +struct usb_otg *usb_otg_register(struct device *dev,
> >>>>>>>> +				 struct usb_otg_config *config) {
> >>>>>>>> +	struct usb_otg *otg;
> >>>>>>>> +	struct otg_wait_data *wait;
> >>>>>>>> +	int ret = 0;
> >>>>>>>> +
> >>>>>>>> +	if (!dev || !config || !config->fsm_ops)
> >>>>>>>> +		return ERR_PTR(-EINVAL);
> >>>>>>>> +
> >>>>>>>> +	/* already in list? */
> >>>>>>>> +	mutex_lock(&otg_list_mutex);
> >>>>>>>> +	if (usb_otg_get_data(dev)) {
> >>>>>>>> +		dev_err(dev, "otg: %s: device already in otg list\n",
> >>>>>>>> +			__func__);
> >>>>>>>> +		ret = -EINVAL;
> >>>>>>>> +		goto unlock;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	/* allocate and add to list */
> >>>>>>>> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> >>>>>>>> +	if (!otg) {
> >>>>>>>> +		ret = -ENOMEM;
> >>>>>>>> +		goto unlock;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	otg->dev = dev;
> >>>>>>>> +	otg->caps = config->otg_caps;
> >>>>>>>> +
> >>>>>>>> +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
> >>>>>>>> +	     otg->caps->adp_support) && !config->otg_work)
> >>>>>>>> +		dev_info(dev, "otg: limiting to dual-role\n");
> >>>>>>>
> >>>>>>> dev_err, this should be an error.
> >>>>>>
> >>>>>> The condition may be wrong, but it is an information to show that
> >>>>>> current OTG is dual-role.
> >>>>>
> >>>>> This should not happen in any correct design, I even doubt if we
> >>>>> should try to continue by "downgrade" it to be duel role, currently
> >>>>> the only example user is dual role, so doing like this can't be tested
> >>>>> by real case, this downgrade is not so easy like we image, at least
> >>>>> for chipidea otg driver, simply replace a queue worker may not work,
> >>>>> as we have much more difference between the 2 configs.
> >>>>>
> >>>>
> >>>> Would you show more why chipidea can't work just replace the work item,
> >>>> and see if anything we still can improve for this framework?
> >>>
> >>> In real OTG, we need enable AVV irq,
> >>
> >> Enable and Handling AVV is platform stuff. In this framework, we are
> >> focus on how otg device manages host and gadget together, and the state
> >> machine when the related otg event occurs.
> >>
> >>> but for duel role, nobody care/handle,
> >>> there are much more resource required for OTG: timers, hnp polling,
> >>> otg test device handling... 
> >>
> >> They are common things for fully OTG fsm, you can move them
> >> to common code (In fact, hnp polling handling is already common code).
> >>
> >>>
> >>> with current design, chipidea driver can support real OTG with its own
> >>> queue worker, or DRD with Roger's drd work item if config is correct.
> >>>
> >>> But improve something to work on a *wrong* config will make it complicated
> >>> and does not make much sense IMO.
> >>>
> >>
> >> What does above "config" you mean?
> >>
> >> If the configure is fully OTG, you can choose different state machine,
> >> eg otg_statemachine, if you find it is hard for chipidea to use this
> >> framework, just list the reason, and see if we can improve.
> >>
> > 
> > Roger, after discussing with Jun off line, we think usb_otg_register
> > should return -ENOTSUPP if platform is OTG capabilities (HNP || SRP ||
> > ADP), since this patch set does not cover fully otg features, the users
> 
> But this series isn't preventing full otg implementation. You can
> still do that via config->otg_work.
> 
> I can modify the following condition to return -ENOTSUPP instead of
> defaulting to dual-role
> 
> struct usb_otg *usb_otg_register(...)
> {
> ...
>         if ((otg->caps->hnp_support || otg->caps->srp_support ||
>              otg->caps->adp_support) && !config->otg_work) {
> 		dev_err(dev, "otg: otg_work must be provided for OTG support\n");
> 		return -ENOTSUPP;
> 	}
> ...
> }
> 

According to Jun, this framework still needs to improve (maybe small) even
using user work item for fully OTG, so the first step is let the drd
work well:)

-- 

Best Regards,
Peter Chen

[toc] | [prev] | [next] | [standalone]

#1389839

On 28/04/16 04:54, Peter Chen wrote:
> On Wed, Apr 27, 2016 at 01:59:44PM +0300, Roger Quadros wrote:
>> Hi,
>>
>> On 27/04/16 06:15, Peter Chen wrote:
>>> On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
>>>> On Tue, Apr 26, 2016 at 07:00:22AM +0000, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Peter Chen [mailto:hzpeterchen@gmail.com]
>>>>>> Sent: Tuesday, April 26, 2016 2:28 PM
>>>>>> To: Jun Li <jun.li@nxp.com>
>>>>>> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
>>>>>> balbi@kernel.org; gregkh@linuxfoundation.org; peter.chen@freescale.com;
>>>>>> dan.j.williams@intel.com; jun.li@freescale.com;
>>>>>> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
>>>>>> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
>>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>>>
>>>>>> On Tue, Apr 26, 2016 at 05:11:36AM +0000, Jun Li wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>>> -----Original Message-----
>>>>>>>> From: Peter Chen [mailto:hzpeterchen@gmail.com]
>>>>>>>> Sent: Tuesday, April 26, 2016 11:47 AM
>>>>>>>> To: Jun Li <jun.li@nxp.com>
>>>>>>>> Cc: Roger Quadros <rogerq@ti.com>; stern@rowland.harvard.edu;
>>>>>>>> balbi@kernel.org; gregkh@linuxfoundation.org;
>>>>>>>> peter.chen@freescale.com; dan.j.williams@intel.com;
>>>>>>>> jun.li@freescale.com; mathias.nyman@linux.intel.com;
>>>>>>>> tony@atomide.com; Joao.Pinto@synopsys.com; abrestic@chromium.org;
>>>>>>>> r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>>>>>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org
>>>>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>>>>>
>>>>>>>> On Tue, Apr 26, 2016 at 02:07:56AM +0000, Jun Li wrote:
>>>>>>>>>> +struct usb_otg *usb_otg_register(struct device *dev,
>>>>>>>>>> +				 struct usb_otg_config *config) {
>>>>>>>>>> +	struct usb_otg *otg;
>>>>>>>>>> +	struct otg_wait_data *wait;
>>>>>>>>>> +	int ret = 0;
>>>>>>>>>> +
>>>>>>>>>> +	if (!dev || !config || !config->fsm_ops)
>>>>>>>>>> +		return ERR_PTR(-EINVAL);
>>>>>>>>>> +
>>>>>>>>>> +	/* already in list? */
>>>>>>>>>> +	mutex_lock(&otg_list_mutex);
>>>>>>>>>> +	if (usb_otg_get_data(dev)) {
>>>>>>>>>> +		dev_err(dev, "otg: %s: device already in otg list\n",
>>>>>>>>>> +			__func__);
>>>>>>>>>> +		ret = -EINVAL;
>>>>>>>>>> +		goto unlock;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	/* allocate and add to list */
>>>>>>>>>> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>>>>>>>>>> +	if (!otg) {
>>>>>>>>>> +		ret = -ENOMEM;
>>>>>>>>>> +		goto unlock;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>> +	otg->dev = dev;
>>>>>>>>>> +	otg->caps = config->otg_caps;
>>>>>>>>>> +
>>>>>>>>>> +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
>>>>>>>>>> +	     otg->caps->adp_support) && !config->otg_work)
>>>>>>>>>> +		dev_info(dev, "otg: limiting to dual-role\n");
>>>>>>>>>
>>>>>>>>> dev_err, this should be an error.
>>>>>>>>
>>>>>>>> The condition may be wrong, but it is an information to show that
>>>>>>>> current OTG is dual-role.
>>>>>>>
>>>>>>> This should not happen in any correct design, I even doubt if we
>>>>>>> should try to continue by "downgrade" it to be duel role, currently
>>>>>>> the only example user is dual role, so doing like this can't be tested
>>>>>>> by real case, this downgrade is not so easy like we image, at least
>>>>>>> for chipidea otg driver, simply replace a queue worker may not work,
>>>>>>> as we have much more difference between the 2 configs.
>>>>>>>
>>>>>>
>>>>>> Would you show more why chipidea can't work just replace the work item,
>>>>>> and see if anything we still can improve for this framework?
>>>>>
>>>>> In real OTG, we need enable AVV irq,
>>>>
>>>> Enable and Handling AVV is platform stuff. In this framework, we are
>>>> focus on how otg device manages host and gadget together, and the state
>>>> machine when the related otg event occurs.
>>>>
>>>>> but for duel role, nobody care/handle,
>>>>> there are much more resource required for OTG: timers, hnp polling,
>>>>> otg test device handling... 
>>>>
>>>> They are common things for fully OTG fsm, you can move them
>>>> to common code (In fact, hnp polling handling is already common code).
>>>>
>>>>>
>>>>> with current design, chipidea driver can support real OTG with its own
>>>>> queue worker, or DRD with Roger's drd work item if config is correct.
>>>>>
>>>>> But improve something to work on a *wrong* config will make it complicated
>>>>> and does not make much sense IMO.
>>>>>
>>>>
>>>> What does above "config" you mean?
>>>>
>>>> If the configure is fully OTG, you can choose different state machine,
>>>> eg otg_statemachine, if you find it is hard for chipidea to use this
>>>> framework, just list the reason, and see if we can improve.
>>>>
>>>
>>> Roger, after discussing with Jun off line, we think usb_otg_register
>>> should return -ENOTSUPP if platform is OTG capabilities (HNP || SRP ||
>>> ADP), since this patch set does not cover fully otg features, the users
>>
>> But this series isn't preventing full otg implementation. You can
>> still do that via config->otg_work.
>>
>> I can modify the following condition to return -ENOTSUPP instead of
>> defaulting to dual-role
>>
>> struct usb_otg *usb_otg_register(...)
>> {
>> ...
>>         if ((otg->caps->hnp_support || otg->caps->srp_support ||
>>              otg->caps->adp_support) && !config->otg_work) {
>> 		dev_err(dev, "otg: otg_work must be provided for OTG support\n");
>> 		return -ENOTSUPP;
>> 	}
>> ...
>> }
>>
> 
> According to Jun, this framework still needs to improve (maybe small) even
> using user work item for fully OTG, so the first step is let the drd
> work well:)
> 
Agreed. Until one OTG implementation is tested we can't really say it works :).

cheers,
-roger

[toc] | [prev] | [next] | [standalone]

#1388900

Hi Jun,

On 26/04/16 05:07, Jun Li wrote:
> Hi Roger
> 
>> -----Original Message-----
>> From: Roger Quadros [mailto:rogerq@ti.com]
>> Sent: Tuesday, April 05, 2016 10:05 PM
>> To: stern@rowland.harvard.edu; balbi@kernel.org;
>> gregkh@linuxfoundation.org; peter.chen@freescale.com
>> Cc: dan.j.williams@intel.com; jun.li@freescale.com;
>> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com;
>> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org;
>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org; Roger Quadros
>> <rogerq@ti.com>
>> Subject: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>
>> It provides APIs for the following tasks
>>
>> - Registering an OTG/dual-role capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> Provide a dual-role device (DRD) state machine.
>> DRD mode is a reduced functionality OTG mode. In this mode we don't
>> support SRP, HNP and dynamic role-swap.
>>
>> In DRD operation, the controller mode (Host or Peripheral) is decided
>> based on the ID pin status. Once a cable plug (Type-A or Type-B) is
>> attached the controller selects the state and doesn't change till the
>> cable in unplugged and a different cable type is inserted.
>>
>> As we don't need most of the complex OTG states and OTG timers we
>> implement a lean DRD state machine in usb-otg.c.
>> The DRD state machine is only interested in 2 hardware inputs 'id' and
>> 'b_sess_vld'.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
> 
> ...
> 
>> +/**
>> + * Register pending host/gadget and remove entry from wait list  */
>> +static void usb_otg_flush_wait(struct device *otg_dev) {
>> +	struct otg_wait_data *wait;
>> +	struct otg_hcd *host;
>> +	struct otg_gcd *gadget;
>> +
>> +	mutex_lock(&wait_list_mutex);
>> +
>> +	wait = usb_otg_get_wait(otg_dev);
>> +	if (!wait)
>> +		goto done;
>> +
>> +	dev_dbg(otg_dev, "otg: registering pending host/gadget\n");
>> +	gadget = &wait->gcd;
>> +	if (gadget)
> 
> If (gadget->gadget)

good catch :)
I'll probably rename the local variables
host to hcd
gadget to gcd.

> 
>> +		usb_otg_register_gadget(gadget->gadget, gadget->ops);
>> +
>> +	host = &wait->primary_hcd;
>> +	if (host->hcd)
>> +		usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
>> +				     host->ops);
>> +
>> +	host = &wait->shared_hcd;
>> +	if (host->hcd)
>> +		usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
>> +				     host->ops);
>> +
>> +	list_del(&wait->list);
>> +	kfree(wait);
>> +
>> +done:
>> +	mutex_unlock(&wait_list_mutex);
>> +}
>> +
>> +/**
>> + * Check if the OTG device is in our OTG list and return
>> + * usb_otg data, else NULL.
>> + *
>> + * otg_list_mutex must be held.
>> + */
>> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev) {
>> +	struct usb_otg *otg;
>> +
>> +	if (!otg_dev)
>> +		return NULL;
>> +
>> +	list_for_each_entry(otg, &otg_list, list) {
>> +		if (otg->dev == otg_dev)
>> +			return otg;
>> +	}
>> +
>> +	return NULL;
>> +}
> 
> Could you export it to be a public API, we may need access usb_otg
> in common host driver for handling of enumeration of otg test device.

We can always do that later. As of now nobody is using it so let's keep it private.
> 
> ...
> 
>> +/**
>> + * Called when entering a DRD state.
>> + * fsm->lock must be held.
>> + */
>> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state
>> +new_state) {
>> +	struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
>> +
>> +	if (otg->state == new_state)
>> +		return;
>> +
>> +	fsm->state_changed = 1;
>> +	dev_dbg(otg->dev, "otg: set state: %s\n",
>> +		usb_otg_state_string(new_state));
>> +	switch (new_state) {
>> +	case OTG_STATE_B_IDLE:
>> +		drd_set_protocol(fsm, PROTO_UNDEF);
>> +		otg_drv_vbus(otg, 0);
>> +		break;
>> +	case OTG_STATE_B_PERIPHERAL:
>> +		drd_set_protocol(fsm, PROTO_GADGET);
>> +		otg_drv_vbus(otg, 0);
>> +		break;
>> +	case OTG_STATE_A_HOST:
>> +		drd_set_protocol(fsm, PROTO_HOST);
>> +		otg_drv_vbus(otg, 1);
>> +		break;
>> +	case OTG_STATE_UNDEFINED:
>> +	case OTG_STATE_B_SRP_INIT:
>> +	case OTG_STATE_B_WAIT_ACON:
>> +	case OTG_STATE_B_HOST:
>> +	case OTG_STATE_A_IDLE:
>> +	case OTG_STATE_A_WAIT_VRISE:
>> +	case OTG_STATE_A_WAIT_BCON:
>> +	case OTG_STATE_A_SUSPEND:
>> +	case OTG_STATE_A_PERIPHERAL:
>> +	case OTG_STATE_A_WAIT_VFALL:
>> +	case OTG_STATE_A_VBUS_ERR:
> 
> Remove above unused states.

OK.
> 
>> +	default:
>> +		dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
>> +			 __func__, usb_otg_state_string(new_state));
>> +		break;
>> +	}
>> +
>> +	otg->state = new_state;
>> +}
>> +
>> +/**
>> + * DRD state change judgement
>> + *
>> + * For DRD we're only interested in some of the OTG states
>> + * i.e. OTG_STATE_B_IDLE: both peripheral and host are stopped
>> + *	OTG_STATE_B_PERIPHERAL: peripheral active
>> + *	OTG_STATE_A_HOST: host active
>> + * we're only interested in the following inputs
>> + *	fsm->id, fsm->b_sess_vld
>> + */
>> +int drd_statemachine(struct usb_otg *otg) {
>> +	struct otg_fsm *fsm = &otg->fsm;
>> +	enum usb_otg_state state;
>> +	int ret;
>> +
>> +	mutex_lock(&fsm->lock);
>> +
>> +	fsm->state_changed = 0;
>> +	state = otg->state;
>> +
>> +	switch (state) {
>> +	case OTG_STATE_UNDEFINED:
>> +		if (!fsm->id)
>> +			drd_set_state(fsm, OTG_STATE_A_HOST);
>> +		else if (fsm->id && fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
>> +		else
>> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
>> +		break;
>> +	case OTG_STATE_B_IDLE:
>> +		if (!fsm->id)
>> +			drd_set_state(fsm, OTG_STATE_A_HOST);
>> +		else if (fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
>> +		break;
>> +	case OTG_STATE_B_PERIPHERAL:
>> +		if (!fsm->id)
>> +			drd_set_state(fsm, OTG_STATE_A_HOST);
>> +		else if (!fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
>> +		break;
>> +	case OTG_STATE_A_HOST:
>> +		if (fsm->id && fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_PERIPHERAL);
>> +		else if (fsm->id && !fsm->b_sess_vld)
>> +			drd_set_state(fsm, OTG_STATE_B_IDLE);
>> +		break;
>> +
>> +	/* invalid states for DRD */
>> +	case OTG_STATE_B_SRP_INIT:
>> +	case OTG_STATE_B_WAIT_ACON:
>> +	case OTG_STATE_B_HOST:
>> +	case OTG_STATE_A_IDLE:
>> +	case OTG_STATE_A_WAIT_VRISE:
>> +	case OTG_STATE_A_WAIT_BCON:
>> +	case OTG_STATE_A_SUSPEND:
>> +	case OTG_STATE_A_PERIPHERAL:
>> +	case OTG_STATE_A_WAIT_VFALL:
>> +	case OTG_STATE_A_VBUS_ERR:
> 
> Remove above unused states and add a default:

OK.
> 
>> +		dev_err(otg->dev, "%s: otg: invalid usb-drd state: %s\n",
>> +			__func__, usb_otg_state_string(state));
>> +		drd_set_state(fsm, OTG_STATE_UNDEFINED);
>> +	break;
>> +	}
>> +
>> +	ret = fsm->state_changed;
>> +	mutex_unlock(&fsm->lock);
>> +	dev_dbg(otg->dev, "otg: quit statemachine, changed %d\n",
>> +		fsm->state_changed);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(drd_statemachine);
>> +
>> +/**
>> + * OTG FSM/DRD work function
> 
> DRD work function

Yes.
> 
>> + */
>> +static void usb_otg_work(struct work_struct *work) {
> 
> usb_drd_work() name is better as it's only for drd.

Agreed.
> 
>> +	struct usb_otg *otg = container_of(work, struct usb_otg, work);
>> +
>> +	pm_runtime_get_sync(otg->dev);
>> +	drd_statemachine(otg);
>> +	pm_runtime_put_sync(otg->dev);
>> +}
>> +
>> +/**
>> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
>> + * @dev: OTG/dual-role controller device.
>> + * @config: OTG configuration.
>> + *
>> + * Registers the OTG/dual-role controller device with the USB OTG core.
>> + *
>> + * Return: struct usb_otg * if success, ERR_PTR() if error.
>> + */
>> +struct usb_otg *usb_otg_register(struct device *dev,
>> +				 struct usb_otg_config *config)
>> +{
>> +	struct usb_otg *otg;
>> +	struct otg_wait_data *wait;
>> +	int ret = 0;
>> +
>> +	if (!dev || !config || !config->fsm_ops)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	/* already in list? */
>> +	mutex_lock(&otg_list_mutex);
>> +	if (usb_otg_get_data(dev)) {
>> +		dev_err(dev, "otg: %s: device already in otg list\n",
>> +			__func__);
>> +		ret = -EINVAL;
>> +		goto unlock;
>> +	}
>> +
>> +	/* allocate and add to list */
>> +	otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>> +	if (!otg) {
>> +		ret = -ENOMEM;
>> +		goto unlock;
>> +	}
>> +
>> +	otg->dev = dev;
>> +	otg->caps = config->otg_caps;
>> +
>> +	if ((otg->caps->hnp_support || otg->caps->srp_support ||
>> +	     otg->caps->adp_support) && !config->otg_work)
>> +		dev_info(dev, "otg: limiting to dual-role\n");
> 
> dev_err, this should be an error.

Yes, I'll update it to like so.

		dev_err(dev, "otg: otg_work function must be provided for OTG\n");
		return -EINVAL;

cheers,
-roger
> 
>> +
>> +	if (config->otg_work)	/* custom otg_work ? */
>> +		INIT_WORK(&otg->work, config->otg_work);
>> +	else
>> +		INIT_WORK(&otg->work, usb_otg_work);
>> +
>> +	otg->wq = create_singlethread_workqueue("usb_otg");
>> +	if (!otg->wq) {
>> +		dev_err(dev, "otg: %s: can't create workqueue\n",
>> +			__func__);
>> +		ret = -ENOMEM;
>> +		goto err_wq;
>> +	}
>> +
>> +	/* set otg ops */
>> +	otg->fsm.ops = config->fsm_ops;
>> +
>> +	mutex_init(&otg->fsm.lock);
>> +
>> +	list_add_tail(&otg->list, &otg_list);
>> +	mutex_unlock(&otg_list_mutex);
>> +
>> +	/* were we in wait list? */
>> +	mutex_lock(&wait_list_mutex);
>> +	wait = usb_otg_get_wait(dev);
>> +	mutex_unlock(&wait_list_mutex);
>> +	if (wait) {
>> +		/* register pending host/gadget and flush from list */
>> +		usb_otg_flush_wait(dev);
>> +	}
>> +
>> +	return otg;
>> +
>> +err_wq:
>> +	kfree(otg);
>> +unlock:
>> +	mutex_unlock(&otg_list_mutex);
>> +	return ERR_PTR(ret);
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_register);
>> +
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web