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

[Lu Baolu <baolu.lu@linux.intel.com>]

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...

I am sorry for the confusion.

The mux device has already been created. It could be a child of a mfd device,
or be created explicitly as a platform device. The mux driver (for example,
intel-mux-gpio.ko) will bind to the device. intel-mux-gpio.ko will then call into this
shared code. This shared code is actually part of the mux driver, except that it
could also be used in other mux driver (for example, intel-mux-drcfg.ko).

I can't always expect the mux device to be created with intel_usb_mux_register()
since it could possibly be a cell of a mfd, or just an ACPI device. :-)

>
>>>> +#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.

Yes, if I add --strict, I hit some warnings.

I will fix these warnings and always run checkpatch.pl with --strict
before I submit patches.

Thank you!

>
>>> 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 have answered above.

>
>> 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_"
> :(

Do you mind if I change the symbols to something like "usb_mux_"?
Or, keep it Intel specific now and change it when there is real other
consumers later?

>> 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? :)

The mux is a real device. :-)   As my understanding (please correct me if I
misunderstood it), the question is that should 1) the device be created before
passing it to intel_usb_mux_register(), or 2) let intel_usb_mux_register() create
the device.

IMHO, option 2) is not possible for Intel platform. The port mux in Intel device
could be an ACPI device or part of an ACPI device. Hence, the mux device could
be created implicitly by ACPI or mfd framework.

ps. this function name is really confusing. It isn't designed to register a new
mux device, but an interface for a shared common code. Sorry about it and
I will look for a meaningful one for the next version.

Best Regards,
Baolu

>
> thanks,
>
> greg k-h
>