From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753534AbcDFKZZ (ORCPT ); Wed, 6 Apr 2016 06:25:25 -0400 Received: from mga01.intel.com ([192.55.52.88]:48600 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751115AbcDFKZX (ORCPT ); Wed, 6 Apr 2016 06:25:23 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,447,1455004800"; d="asc'?scan'208";a="926566627" From: Felipe Balbi To: Greg Kroah-Hartman , Lu Baolu Cc: Mathias Nyman , Lee Jones , Heikki Krogerus , MyungJoo Ham , Chanwoo Choi , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux In-Reply-To: <20160406092854.GB29930@kroah.com> References: <1457423628-3183-1-git-send-email-baolu.lu@linux.intel.com> <1457423628-3183-4-git-send-email-baolu.lu@linux.intel.com> <20160311000643.GD2586@kroah.com> <56E60F42.8030701@linux.intel.com> <20160314032714.GA4665@kroah.com> <5704B067.1010001@linux.intel.com> <20160406092854.GB29930@kroah.com> User-Agent: Notmuch/0.21+96~g9bbc54b (http://notmuchmail.org) Emacs/25.0.90.3 (x86_64-pc-linux-gnu) Date: Wed, 06 Apr 2016 13:23:26 +0300 Message-ID: <87mvp72fz5.fsf@intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Greg Kroah-Hartman writes: > On Wed, Apr 06, 2016 at 02:44:55PM +0800, Lu Baolu wrote: >> Hi, >>=20 >> On 03/14/2016 11:27 AM, Greg Kroah-Hartman wrote: >> > On Mon, Mar 14, 2016 at 09:09:22AM +0800, Lu Baolu wrote: >> >> On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote: >> >>> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote: >> >>>> +struct intel_mux_dev { >> >>>> + struct device *dev; >> >>>> + char *extcon_name; >> >>>> + char *cable_name; >> >>>> + int (*cable_set_cb)(struct intel_mux_dev *mux); >> >>>> + int (*cable_unset_cb)(struct intel_mux_dev *mux); >> >>>> +}; >> >>> This is a device, why not make it one? Don't just hold a reference. >> >>> And do you really even hold that reference? >> >> It's not a device. It's just an encapsulation for parameters passed i= nto >> >> intel_usb_mux_register(). >> > But you called it a device, so you can understand my confusion. >> > >> > And why not make it a device? Why isn't this one? Hint, I really thi= nk >> > it should be... >> > >> >>> And your api is horrid, think about what you want the "core" to do h= ere, >> >>> it should be the one creating the device and returning it to the cal= ler, >> >>> not forcing the caller to somehow create it first and then pass it i= n. >> >> This isn't a layer or core. It doesn't create any new devices. It's a= ctually >> >> some shared code which can be used by all Intel dual role port driver= s. >> > It should be a device, as you are treating it like one :) >> > >> >> I put it in a separated file because 1) this can avoid duplication; 2= ) this code >> >> could be used for any architectures as long as a USB port is shared by >> >> two components and it needs an OS response when event triggers. >> > It's a bit hard for other arches to be using something called "intel_" >> > :( >>=20 >> Are there any other implementations which need an external mux >> to swap the usb roles? > > Why wouldn't there be? :) Well, e.g. dwc3 can be configured with an optional dual-role block which handles role swapping. It's not the same as intel mux. What Intel did is that we have completely separate xHCI (Intel's own) and dwc3 (licensed from SNPS) IPs and another physical mux (not discrete, though) which controls where the data lines should go (it also tells dwc3 about voltage levels, etc). What I'm trying to say is that there are other means for implementing this sort of solution. Also, I still subscribe to Dave B's "3-user requirement": if there are 3 or more users for something, then it really deserves some deeper thoughts on how it needs to be abstracted. There is, however, the detail that we're exposing a sysfs ABI, so we shouldn't carelessly choose a name to the sysfs file; in any case, if you know of any other similar dual-role implementations, that would help guiding Baolu's implementation, right ? ;-) >> void devm_portmux_unregister(struct device *dev, >> struct portmux_dev *pdev) >>=20 >> Do I get it right? > > Why devm? Your lifetime rules don't allow that at all. I agree here, devm doesn't make a lot of sense. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXBOOeAAoJEIaOsuA1yqREDmEQAI8ycErx90+x9tXb6vmtKEhG VxwLlQ1iBf4mpK+8cRDsDizShFzDEvF9M61p6C/6hgPKxuxh/V/jI39v6ak1P40x 2e2Q31+uXjsvUlLhLNxdcqd6NeaPJ10kFBEjGAfEPhBS3qnErBzhhb25z8CKy07r JwdQigZJHKyEIsXuOqnW2OBriG/YtTskPjWmcuj0IOoDkuBXCTOVWuBfrZqJCHXd XrYOBrSoVc5v/W33XIAyDE9++ogZ6921QH9UsiQgblx+NR3cvK/lYH+YSEwFFmpD I8c13O41RxShuOvtTLcW5b8CYNJofbLtY+gqR2sCEqOV9mIdTu/eU4QkzprEwICS esRfDiuaroo4xsZm7PuIrNBsVkpT2t6KdmpfeXP7bcwwB4DcLgxSo7d7rLhEaV7N POzORkAAiaJHugVOitPgIgRnDZxAXs4pqRPgMCiGBeMPsEPY2N5BVA1CAyIwuhW3 UMAa43IDNIufz71N1s8vDcWg4h5G5Euci2pwM/RRgdLPgTsYJWVjsQT/TI+3YrVM eFviLVMSpJYo9uvBUKhlg4Yqb45J6BPody1k1xlAEPkbjiqDHWk3PzMAvjlK+4ae 0CM8uOFo2t4KZ5IteaH/KQ79Ctq13s56nhYo3ziECmH27/SbYDGMJYFv4N5OSsZ8 yrDTjSLvbt4ZXK5qGD/v =BYPA -----END PGP SIGNATURE----- --=-=-=--