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

From: sathyanarayanan kuppuswamy
Date: Wed May 31 2017 - 19:37:13 EST


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@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.
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