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

From: Sergei Shtylyov
Date: Wed Apr 06 2016 - 08:40:19 EST


Hello.

On 4/6/2016 9:44 AM, 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?

There are, of course, like Renesas R-Car gen2 SoCs. I've implemented the USB port multiplexing as a part of the PHY driver (see drivers/phy/phy-rcar-gen2.c).

MBR, Sergei