From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964844AbcDFJ3F (ORCPT ); Wed, 6 Apr 2016 05:29:05 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:48262 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964812AbcDFJ3C (ORCPT ); Wed, 6 Apr 2016 05:29:02 -0400 Date: Wed, 6 Apr 2016 05:28:54 -0400 From: Greg Kroah-Hartman To: Lu Baolu Cc: Felipe Balbi , 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 Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5704B067.1010001@linux.intel.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 06, 2016 at 02:44:55PM +0800, Lu Baolu wrote: > Hi, > > 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 into > >> 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 think > > it should be... > > > >>> And your api is horrid, think about what you want the "core" to do here, > >>> it should be the one creating the device and returning it to the caller, > >>> not forcing the caller to somehow create it first and then pass it in. > >> This isn't a layer or core. It doesn't create any new devices. It's actually > >> some shared code which can be used by all Intel dual role port drivers. > > 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_" > > :( > > Are there any other implementations which need an external mux > to swap the usb roles? Why wouldn't there be? :) > >> I guess intel_usb_mux_register/unregister() is a bit misleading. How about > >> changing them to intel_usb_mux_probe/remove()? > > You are going to probe/remove something that isn't a device? Come on > > now... > > > >>> And why is it not symmetrical, you are passing one thing into register > >>> and another into unregister. > >> struct intel_mux_dev is an encapsulation for parameters passed into > >> intel_usb_mux_register(). > > Which is a device. > > > >> It's not a new device structure though the name > >> is a bit misleading. > > Yes it is, hint, you want it to be a device. > > > >> How about remove this structure and put these in function parameters? > > How about making it a real device? :) > > Hi Greg, > > Just want to make myself clear about your expectation. > Did you mean to create a port mux device and return it to the caller? Yes, that's the correct way to do this type of thing. > The interfaces look like: > > struct portmux_dev *devm_portmux_register(struct device *dev, > const struct portmux_desc *desc); > > void devm_portmux_unregister(struct device *dev, > struct portmux_dev *pdev) > > Do I get it right? Why devm? Your lifetime rules don't allow that at all. thanks, greg k-h