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

[sathyanarayanan kuppuswamy]

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