Messages in this thread | | | Date | Sun, 13 Mar 2016 20:27:14 -0700 | From | Greg Kroah-Hartman <> | Subject | Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux |
| |
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...
> >> +#if IS_ENABLED(CONFIG_INTEL_USB_MUX) > >> +extern int intel_usb_mux_register(struct intel_mux_dev *mux); > >> +extern int intel_usb_mux_unregister(struct device *dev); > > It's obvious you didn't run this through checkpatch.pl, please do so... > > I did, but didn't hit any errors or warnings.
Odd, don't put extern in .h files for functions, I thought checkpatch catches that...
Try it with --strict, as you should with all new code you submit.
> > 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_" :(
> 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? :)
thanks,
greg k-h
| |