Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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