From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752909AbcEZBDT (ORCPT ); Wed, 25 May 2016 21:03:19 -0400 Received: from mga11.intel.com ([192.55.52.93]:12355 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751810AbcEZBDS (ORCPT ); Wed, 25 May 2016 21:03:18 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,366,1459839600"; d="scan'208";a="814944967" Subject: Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux To: Heikki Krogerus References: <1462426383-3949-1-git-send-email-baolu.lu@linux.intel.com> <1462426383-3949-3-git-send-email-baolu.lu@linux.intel.com> <20160525110652.GA27570@kuha.fi.intel.com> Cc: felipe.balbi@linux.intel.com, Mathias Nyman , Greg Kroah-Hartman , Lee Jones , Liam Girdwood , Mark Brown , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org From: Lu Baolu Message-ID: <57464B4F.8050907@linux.intel.com> Date: Thu, 26 May 2016 09:03:11 +0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20160525110652.GA27570@kuha.fi.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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