Re: [PATCH 1/5] dt-bindings: connector: add optional properties for Type-B

[Chunfeng Yun <chunfeng.yun@mediatek.com>]

Hi,
On Wed, 2019-03-13 at 11:18 +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/13/19 11:15 AM, Chunfeng Yun wrote:
> > Hi,
> > On Tue, 2019-03-12 at 13:45 +0100, Hans de Goede wrote:
> >> Hi,
> >> On 12-03-19 04:18, Chunfeng Yun wrote:
> >>> Hi,
> >>> On Mon, 2019-03-11 at 09:06 +0100, Hans de Goede wrote:
> >>>> Hi,
> >>>>
> >>>> On 11-03-19 06:33, Chunfeng Yun wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Fri, 2019-03-08 at 13:07 +0100, Hans de Goede wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 08-03-19 07:13, Chunfeng Yun wrote:
> >>>>>>> Add id-gpios, vbus-gpios, vbus-supply and pinctrl properties for
> >>>>>>> usb-b-connector
> >>>>>>>
> >>>>>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> >>>>>>> ---
> >>>>>>>      .../devicetree/bindings/connector/usb-connector.txt    | 10 ++++++++++
> >>>>>>>      1 file changed, 10 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>>>> index a9a2f2fc44f2..7a07b0f4f973 100644
> >>>>>>> --- a/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>>>> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> >>>>>>> @@ -17,6 +17,16 @@ Optional properties:
> >>>>>>>      - self-powered: Set this property if the usb device that has its own power
> >>>>>>>        source.
> >>>>>>>      
> >>>>>>> +Optional properties for usb-b-connector:
> >>>>>>> +- id-gpios: gpio for USB ID pin.
> >>>>>>
> >>>>>> What about boards where the ID pin is *not* connected to a GPIO,
> >>>>>> but e.g. to a special pin on the PMIC which can also detect
> >>>>>> an ACA adapter ? Currently this case is handled by extcon
> >>>>>> drivers, but we have no way to set e.g. vbus-supply for the
> >>>>>> connector. Maybe in this case the usb-connector node should
> >>>>>> be a child of the PMIC node ?
> >>>>> Yes, it would be, PMIC is in charger of detecting the status of ID pin
> >>>>
> >>>> Ok, then I think this should be documented too.
> >>>>
> >>>>>> And in many cases there also is a mux to switch the datalines
> >>>>>> between the host and device(gadget) controllers, how should
> >>>>>> that be described in this model?  See the new usb-role-switch
> >>>>>> code under drivers/usb/roles
> >>>>>>
> >>>>>> In some cases the mux is controlled through a gpio, so we
> >>>>>> may want to add a "mux-gpios" here in which case we also
> >>>>>> need to define what 0/1 means.
> >>>>> I'm not sure, the mux seems not belong to this connector,
> >>>>> and may need another driver to register usb-role-switch,
> >>>>> similar to:
> >>>>>
> >>>>> [v2,2/2] usb: typec: add typec switch via GPIO control
> >>>>> https://patchwork.kernel.org/patch/10834327/
> >>>>
> >>>> Right the mux/role-switch will need a driver, but the "owner"
> >>>> of the usb_connector, e.g. the PMIC or the owner of the
> >>>> id GPIO pin needs to know which device is the role-switch so
> >>>> that it can set the role correctly based on the id-pin.
> >>>>
> >>>> Your binding already contains Vbus info, allowing the owner
> >>>> of the usb_connector to enable/disable Vbus based on the id-pin,
> >>>> but the owner will also be responsible for setting the role-switch.
> >>> In this patch series, I make usb_connector driver enable/disable Vbus,
> >>> but not the parent(USB controller) of usb_connector which registers a
> >>> usb-role-switch, which way do you think it is better?
> >>
> >> IMHO there should not be a driver for the usb_connector child-node,
> >> only for the parent-device of that child-node.
> > If so, each USB controller driver should process this special case by
> > itself when use a gpio to detect the status of ID pin, it's not a
> > friendly way. And it's easy by using extcon-usb-gpio driver before, so
> > we also want to provide a simple way when support usb_connector, no
> > matter what method we choose.
> > 
> > Ideally, the only one thing that USB controller driver need to do, just
> > register a usb-role-switch, and leave decision when switch the role to
> > other drivers, such as type-C, PMIC/charger, also include gpio
> > 
> >>
> >> There are going to be too many specific setups surrounding PMICs to
> >> be able to do a single generic driver for the connector.
> >>
> > Fortunately, these patches only focus on the simplest gpio driven role
> > switch :)
> 
> In that case there should be an extra compatible added to the
> usb_b_connector node to which the "simple gpio" driver binds,
> so that not all devicetree-s declaring a usb_b_connector child-node
> automatically get that driver bound, 
Yes
> and the gpio / vbus
> properties should be mandatory properties for usb_b_connector
> child nodes declaring the extra compatible, rather then being
> optional for the generic compatible.
At least one of id-gpios and vbus-gpios should exist, so both of them
are optional, of course if only vbus-gpios exists, then only device mode
is supported, in this case vbus is also optional.

> 
> Regards,
> 
> Hans
>