Re: [PATCHv2 1/2] extcon: add driver for Intel USB mux

[Heikki Krogerus]

Hi,

> I do never want to add some specific funtcion for only this driver.
> I think is not appropriate way.
> - intel_usb_mux_unregister
> - intel_usb_mux_register
> 
> The client driver using extcon driver should use the standard extcon API
> for code consistency. Also, I'll do the more detailed review for this patch.

The internal mux we are controlling here is physically separate
device. Ideally we could populate child device for it, but since that
is not possible because of the resource conflict, we use the library
approach, which is really not that uncommon.

I don't think I agree with your point even at general level. The
control required to handle this mux, even though simple, is enough to
deserve to be separated from xHCI code. xHCI should not need to care
about anything else expect does it have the mux, i.e. does it need to
register it or not. It should not need to care about how it needs to
be controlled or even what it is. We may decide to create something
else out of it instead of an extcon device later.

But in any case, the mux is available on all new Intel platforms, but
it needs to be controlled by OS only in few "special" cases. We can
not force xHCI (or pci-quirks.c to be more precise) to be aware of
these "special" cases. The only way to make it work like that would
bet by using ifdefs, and we really really don't want that.

And please also note that, though for now we only expect the mux
control registers to be part of xHCI MMIO, that is not always the
case. The control registers are part of the device controller MMIO on
some platforms. We do not want to duplicate the whole control of the
mux if/when we need the OS to be in control of it on a platform that
has those control registers mapped somewhere else then xHCI MMIO,

So I would say that we have pretty good justification for separating
the mux control, which means unfortunately custom API in this case.

But if you would prefer that we put the files somewhere else then
drivers/extcon/ and include/linux/extcon/ I'm fine with that. If you
like, we can put it to drivers/usb/host/ as that is where
pci-quirks.c is. That way I think we can also put the header to
include/usb/.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/