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

[Felipe Balbi]

Hi,

Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> On Wed, Apr 06, 2016 at 02:44:55PM +0800, Lu Baolu wrote:
>> Hi,
>> 
>> 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...
>> >
>> >>> 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?
>
> Why wouldn't there be?  :)

Well, e.g. dwc3 can be configured with an optional dual-role  block
which handles role swapping. It's not the same as intel mux. What Intel
did is that we have completely separate xHCI (Intel's own) and dwc3
(licensed from SNPS) IPs and another physical mux (not discrete, though)
which controls where the data lines should go (it also tells dwc3 about
voltage levels, etc).

What I'm trying to say is that there are other means for implementing
this sort of solution. Also, I still subscribe to Dave B's "3-user
requirement": if there are 3 or more users for something, then it really
deserves some deeper thoughts on how it needs to be abstracted.

There is, however, the detail that we're exposing a sysfs ABI, so we
shouldn't carelessly choose a name to the sysfs file; in any case, if
you know of any other similar dual-role implementations, that would help
guiding Baolu's implementation, right ? ;-)

>> void devm_portmux_unregister(struct device *dev,
>>                                    struct portmux_dev *pdev)
>> 
>> Do I get it right?
>
> Why devm?  Your lifetime rules don't allow that at all.

I agree here, devm doesn't make a lot of sense.

-- 
balbi

Attachment: signature.asc
Description: PGP signature