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

[Lu Baolu]

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