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?
>> 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? :)
Hi Greg,
Just want to make myself clear about your expectation.
Did you mean to create a port mux device and return it to the caller?
The interfaces look like:
struct portmux_dev *devm_portmux_register(struct device *dev,
const struct portmux_desc *desc);
void devm_portmux_unregister(struct device *dev,
struct portmux_dev *pdev)
Do I get it right?
Best regards,
Baolu
>
> thanks,
>
> greg k-h
>
