Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[Peter Rosin]

On 2017-05-30 19:47, sathyanarayanan kuppuswamy wrote:
> Hi Peter,
> 
> Thanks for your comments.
> 
> On 05/30/2017 06:40 AM, Peter Rosin wrote:
>> On 2017-05-30 02:47, sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx wrote:
>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
>>>
>>> In some Intel SOCs, a single USB port is shared between USB device and
>>> host controller and an internal mux is used to control the selection of
>>> port by host/device controllers. This driver adds support for the USB
>>> internal mux, and all consumers of this mux can use interfaces provided
>>> by mux subsytem to control the state of the internal mux.
>>>
>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>
>> Hi!
>>
>> A few things make me curious...
>>
>> 1. How are platform devices w/o any match table probed? Do you have to
>>     manually load them, or what?
> Yes, for now, I am manually creating this device from dwc3 driver to 
> test it. But in future I am planning to get ACPI ID for this device to 
> probe it from BIOS.
>>
>> 2. What are these "all the consumers of this mux" that you mention,
> Currently the only consumer for this mux device is, Broxton USB PHY 
> driver. Its not yet upstreamed. I hoping to get this driver merged first 
> before submitting the other driver for review.

Is any other consumer in the charts at all? Can this existing consumer
ever make use of some other mux? If the answer to both those questions
are 'no', then I do not see much point in involving the mux subsystem at
all. The Broxton USB PHY driver could just as well write to the register
all by itself, no?

>>   and how
>>     do they find the correct mux control to interact with?
> Your current mux_control_get() API has tight dependency on device tree 
> node and hence we can't use it for this use case.

Yes, that was all I needed, so far. Trying to keep things simple...

> So I am planning to add a new API to get the mux-control based on 
> mux-device name. API interface looks some thing like below. I haven't 
> finalized the patch yet. I will send it you for review in next few days. 
> Let me know if you agree with this idea.
> 
> struct mux_chip *devm_mux_chip_get_by_index(struct device *dev,
>                  const char *parent_name, unsigned int index)
> 
> struct mux_control *devm_mux_control_get_by_index(struct device *dev,
>                  struct mux_chip *mux_chip, unsigned int index)

I don't like exposing struct mux_chip to clients.

My lose plan was to try to dig into the device name if the of_node is
not present, and thus overload the mux_control_get() api. Not sure about
how to handle non-DT muxes with more than one mux-control though, maybe
add an index argument that is ignored when it's not needed? But you
don't really need the index either, so that can wait...

Cheers,
peda