From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932675AbcE0IAg (ORCPT ); Fri, 27 May 2016 04:00:36 -0400 Received: from mga14.intel.com ([192.55.52.115]:39879 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932077AbcE0IAd (ORCPT ); Fri, 27 May 2016 04:00:33 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,372,1459839600"; d="scan'208";a="963433857" Date: Fri, 27 May 2016 11:00:27 +0300 From: Heikki Krogerus To: Lu Baolu 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 Subject: Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux Message-ID: <20160527080027.GE22411@kuha.fi.intel.com> 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> <57464B4F.8050907@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <57464B4F.8050907@linux.intel.com> User-Agent: Mutt/1.6.1 (2016-04-27) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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