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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
