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

[sathyanarayanan kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com>]

Hi,


On 05/30/2017 11:29 PM, Peter Rosin wrote:
> 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@linux.intel.com wrote:
>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>
>>>> 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@linux.intel.com>
>>> 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.
I think this would make the implementation bit complex.

Why not maintain uniformity ? Like searching for mux control based on
device name or using unique chip name + index. This would work for both
DT and non DT cases.
>   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
>

-- 
Sathyanarayanan Kuppuswamy
Linux kernel developer