lkml.org 
[lkml]   [2016]   [Apr]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux
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

\
 
 \ /
  Last update: 2016-04-06 11:41    [W:0.122 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site