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

Groups > linux.kernel > #1406842 > unrolled thread

Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux

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 v8 2/7] usb: mux: add generic code for dual role port mux Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-25 13:10 +0200
    Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux Lu Baolu <baolu.lu@linux.intel.com> - 2016-05-26 03:10 +0200
      Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux Heikki Krogerus <heikki.krogerus@linux.intel.com> - 2016-05-27 10:10 +0200
        Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux Lu Baolu <baolu.lu@linux.intel.com> - 2016-05-27 10:10 +0200

#1406842 — Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux

Hi Baolu,

Sorry to comment this so late, but we got hardware that needs to
configure the mux in OS, and I noticed some problem. We are missing
means to bind a port to the correct mux on multiport systems. That we
need to solve later in any case, but there is an other issue related
to the fact that the notifiers now have to be extcon devices. It's
related, as extcon offers no means to solve the multiport issue, but
in any case..

> +struct portmux_dev *portmux_register(struct portmux_desc *desc)
> +{
> +	static atomic_t portmux_no = ATOMIC_INIT(-1);
> +	struct portmux_dev *pdev;
> +	struct extcon_dev *edev = NULL;
> +	struct device *dev;
> +	int ret;
> +
> +	/* parameter sanity check */
> +	if (!desc || !desc->name || !desc->ops || !desc->dev ||
> +	    !desc->ops->cable_set_cb || !desc->ops->cable_unset_cb)
> +		return ERR_PTR(-EINVAL);
> +
> +	dev = desc->dev;
> +
> +	if (desc->extcon_name) {
> +		edev = extcon_get_extcon_dev(desc->extcon_name);
> +		if (IS_ERR_OR_NULL(edev))
> +			return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> +	if (!pdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdev->desc = desc;
> +	pdev->edev = edev;
> +	pdev->nb.notifier_call = usb_mux_notifier;
> +	mutex_init(&pdev->mux_mutex);
> +
> +	pdev->dev.parent = dev;
> +	dev_set_name(&pdev->dev, "portmux.%lu",
> +		     (unsigned long)atomic_inc_return(&portmux_no));
> +	pdev->dev.groups = portmux_group;
> +	ret = device_register(&pdev->dev);
> +	if (ret)
> +		goto cleanup_mem;
> +
> +	dev_set_drvdata(&pdev->dev, pdev);
> +
> +	if (edev) {
> +		ret = extcon_register_notifier(edev, EXTCON_USB_HOST,
> +					       &pdev->nb);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to register extcon notifier\n");
> +			goto cleanup_dev;
> +		}
> +	}

So I don't actually think this is correct approach. We are forcing the
notifying drivers, on top of depending on this framework, depend on
extcon too, and that simply is too much. I don't think a USB PHY or
charger detection driver should be forced to generate an extcon device
just to satisfy the mux in general.

Instead IMO, this framework should provide an API also for the
notifiers. The drivers that do the notification should not need to
depend on extcon at all. Instead they should be able to just request
an optional handle to a portmux, and use it with the function that you
already provide (usb_mux_change_state(), which of course needs to be
exported). That would make it much easier for us to make problems like
figuring out the correct mux for the correct port a problem for the
framework and not the drivers.

extcon does not really add any value here, but it does complicate
things a lot. We are even exposing new sysfs attributes to control the
mux, complete separate from extcon.


Thanks,

-- 
heikki

[toc] | [next] | [standalone]

#1407323

Hi Heikki,

On 05/25/2016 07:06 PM, Heikki Krogerus wrote:
> Hi Baolu,
>
> Sorry to comment this so late, but we got hardware that needs to
> configure the mux in OS, and I noticed some problem.

Comments are always welcome. :-)

> We are missing
> means to bind a port to the correct mux on multiport systems. That we
> need to solve later in any case, but there is an other issue related
> to the fact that the notifiers now have to be extcon devices. It's
> related, as extcon offers no means to solve the multiport issue, but
> in any case..
>
>> +struct portmux_dev *portmux_register(struct portmux_desc *desc)
>> +{
>> +	static atomic_t portmux_no = ATOMIC_INIT(-1);
>> +	struct portmux_dev *pdev;
>> +	struct extcon_dev *edev = NULL;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	/* parameter sanity check */
>> +	if (!desc || !desc->name || !desc->ops || !desc->dev ||
>> +	    !desc->ops->cable_set_cb || !desc->ops->cable_unset_cb)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	dev = desc->dev;
>> +
>> +	if (desc->extcon_name) {
>> +		edev = extcon_get_extcon_dev(desc->extcon_name);
>> +		if (IS_ERR_OR_NULL(edev))
>> +			return ERR_PTR(-EPROBE_DEFER);
>> +	}
>> +
>> +	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
>> +	if (!pdev)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	pdev->desc = desc;
>> +	pdev->edev = edev;
>> +	pdev->nb.notifier_call = usb_mux_notifier;
>> +	mutex_init(&pdev->mux_mutex);
>> +
>> +	pdev->dev.parent = dev;
>> +	dev_set_name(&pdev->dev, "portmux.%lu",
>> +		     (unsigned long)atomic_inc_return(&portmux_no));
>> +	pdev->dev.groups = portmux_group;
>> +	ret = device_register(&pdev->dev);
>> +	if (ret)
>> +		goto cleanup_mem;
>> +
>> +	dev_set_drvdata(&pdev->dev, pdev);
>> +
>> +	if (edev) {
>> +		ret = extcon_register_notifier(edev, EXTCON_USB_HOST,
>> +					       &pdev->nb);
>> +		if (ret < 0) {
>> +			dev_err(dev, "failed to register extcon notifier\n");
>> +			goto cleanup_dev;
>> +		}
>> +	}
> So I don't actually think this is correct approach. We are forcing the
> notifying drivers, on top of depending on this framework, depend on
> extcon too, and that simply is too much. I don't think a USB PHY or
> charger detection driver should be forced to generate an extcon device
> just to satisfy the mux in general.

Fair enough.

>
> Instead IMO, this framework should provide an API also for the
> notifiers. The drivers that do the notification should not need to
> depend on extcon at all. Instead they should be able to just request
> an optional handle to a portmux, and use it with the function that you
> already provide (usb_mux_change_state(), which of course needs to be
> exported). That would make it much easier for us to make problems like
> figuring out the correct mux for the correct port a problem for the
> framework and not the drivers.
>
> extcon does not really add any value here, but it does complicate
> things a lot. We are even exposing new sysfs attributes to control the
> mux, complete separate from extcon.

I agree with you that we should move extcon out of the framework.

In order to support multiport systems, I have below proposal.

Currently, we have below interfaces.

struct portmux_dev *portmux_register(struct portmux_desc *desc);
void portmux_unregister(struct portmux_dev *pdev);

I will add below ones.

struct portmux_dev *portmux_lookup_by_name(char *name);
int portmux_switch(struct portmux_dev *pdev, enum port_role);

The normal usage mode is
1) the mux device is registered as soon as a mux is detected with
    portmux_register();
2) In components like USB PHY or changer drivers, the mux could
    be retrieved with portmux_lookup_by_name() and controlled via
    portmux_switch().

Is this helpful?

Best regards,
Lu Baolu

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

#1407896

On Thu, May 26, 2016 at 09:03:11AM +0800, Lu Baolu wrote:
> Hi Heikki,
> 
> On 05/25/2016 07:06 PM, Heikki Krogerus wrote:
> > Hi Baolu,
> >
> > Sorry to comment this so late, but we got hardware that needs to
> > configure the mux in OS, and I noticed some problem.
> 
> Comments are always welcome. :-)
> 
> > We are missing
> > means to bind a port to the correct mux on multiport systems. That we
> > need to solve later in any case, but there is an other issue related
> > to the fact that the notifiers now have to be extcon devices. It's
> > related, as extcon offers no means to solve the multiport issue, but
> > in any case..
> >
> >> +struct portmux_dev *portmux_register(struct portmux_desc *desc)
> >> +{
> >> +	static atomic_t portmux_no = ATOMIC_INIT(-1);
> >> +	struct portmux_dev *pdev;
> >> +	struct extcon_dev *edev = NULL;
> >> +	struct device *dev;
> >> +	int ret;
> >> +
> >> +	/* parameter sanity check */
> >> +	if (!desc || !desc->name || !desc->ops || !desc->dev ||
> >> +	    !desc->ops->cable_set_cb || !desc->ops->cable_unset_cb)
> >> +		return ERR_PTR(-EINVAL);
> >> +
> >> +	dev = desc->dev;
> >> +
> >> +	if (desc->extcon_name) {
> >> +		edev = extcon_get_extcon_dev(desc->extcon_name);
> >> +		if (IS_ERR_OR_NULL(edev))
> >> +			return ERR_PTR(-EPROBE_DEFER);
> >> +	}
> >> +
> >> +	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> >> +	if (!pdev)
> >> +		return ERR_PTR(-ENOMEM);
> >> +
> >> +	pdev->desc = desc;
> >> +	pdev->edev = edev;
> >> +	pdev->nb.notifier_call = usb_mux_notifier;
> >> +	mutex_init(&pdev->mux_mutex);
> >> +
> >> +	pdev->dev.parent = dev;
> >> +	dev_set_name(&pdev->dev, "portmux.%lu",
> >> +		     (unsigned long)atomic_inc_return(&portmux_no));
> >> +	pdev->dev.groups = portmux_group;
> >> +	ret = device_register(&pdev->dev);
> >> +	if (ret)
> >> +		goto cleanup_mem;
> >> +
> >> +	dev_set_drvdata(&pdev->dev, pdev);
> >> +
> >> +	if (edev) {
> >> +		ret = extcon_register_notifier(edev, EXTCON_USB_HOST,
> >> +					       &pdev->nb);
> >> +		if (ret < 0) {
> >> +			dev_err(dev, "failed to register extcon notifier\n");
> >> +			goto cleanup_dev;
> >> +		}
> >> +	}
> > So I don't actually think this is correct approach. We are forcing the
> > notifying drivers, on top of depending on this framework, depend on
> > extcon too, and that simply is too much. I don't think a USB PHY or
> > charger detection driver should be forced to generate an extcon device
> > just to satisfy the mux in general.
> 
> Fair enough.
> 
> >
> > Instead IMO, this framework should provide an API also for the
> > notifiers. The drivers that do the notification should not need to
> > depend on extcon at all. Instead they should be able to just request
> > an optional handle to a portmux, and use it with the function that you
> > already provide (usb_mux_change_state(), which of course needs to be
> > exported). That would make it much easier for us to make problems like
> > figuring out the correct mux for the correct port a problem for the
> > framework and not the drivers.
> >
> > extcon does not really add any value here, but it does complicate
> > things a lot. We are even exposing new sysfs attributes to control the
> > mux, complete separate from extcon.
> 
> I agree with you that we should move extcon out of the framework.
> 
> In order to support multiport systems, I have below proposal.
> 
> Currently, we have below interfaces.
> 
> struct portmux_dev *portmux_register(struct portmux_desc *desc);
> void portmux_unregister(struct portmux_dev *pdev);
> 
> I will add below ones.
> 
> struct portmux_dev *portmux_lookup_by_name(char *name);
> int portmux_switch(struct portmux_dev *pdev, enum port_role);
> 
> The normal usage mode is
> 1) the mux device is registered as soon as a mux is detected with
>     portmux_register();
> 2) In components like USB PHY or changer drivers, the mux could
>     be retrieved with portmux_lookup_by_name() and controlled via
>     portmux_switch().
> 
> Is this helpful?

It works for me, and we can improve it later if needed.


Thanks,

-- 
heikki

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

#1407903

Hi,

On 05/27/2016 04:00 PM, Heikki Krogerus wrote:
>> I agree with you that we should move extcon out of the framework.
>> > 
>> > In order to support multiport systems, I have below proposal.
>> > 
>> > Currently, we have below interfaces.
>> > 
>> > struct portmux_dev *portmux_register(struct portmux_desc *desc);
>> > void portmux_unregister(struct portmux_dev *pdev);
>> > 
>> > I will add below ones.
>> > 
>> > struct portmux_dev *portmux_lookup_by_name(char *name);
>> > int portmux_switch(struct portmux_dev *pdev, enum port_role);
>> > 
>> > The normal usage mode is
>> > 1) the mux device is registered as soon as a mux is detected with
>> >     portmux_register();
>> > 2) In components like USB PHY or changer drivers, the mux could
>> >     be retrieved with portmux_lookup_by_name() and controlled via
>> >     portmux_switch().
>> > 
>> > Is this helpful?
> It works for me, and we can improve it later if needed.

Okay, thanks.

Best regards,
Lu Baolu

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web