Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

[Chris Zhong]

Hi Guenter

On 06/18/2016 11:45 PM, Guenter Roeck wrote:
> Hi Chris,
>
> On Mon, Jun 13, 2016 at 2:39 AM, Chris Zhong <zyw@xxxxxxxxxxxxxx> wrote:
> > Add a PHY provider driver for the rk3399 SoC Type-c PHY. The USB
> > Type-C PHY is designed to support the USB3 and DP applications. The
> > PHY basically has two main components: USB3 and DisplyPort. USB3
> > operates in SuperSpeed mode and the DP can operate at RBR, HBR and
> > HBR2 data rates.
> I started integrating the driver with our code.
> Doing so, I realized a problem in the way you are using extcon.
>
> [ ... ]
>
> > +
> > +static int tcphy_pd_event(struct notifier_block *nb,
> > +                         unsigned long event, void *priv)
> > +{
> > +       struct rockchip_typec_phy *tcphy;
> > +       struct extcon_dev *edev = priv;
> > +       int value = edev->state;
> > +       int mode;
> > +       u8 is_plugged, dfp;
> > +
> > +       tcphy = container_of(nb, struct rockchip_typec_phy, event_nb);
> > +
> > +       is_plugged = GET_PLUGGED(value);
> > +       tcphy->flip = GET_FLIP(value);
> > +       dfp = GET_DFP(value);
> > +       tcphy->map = GET_PIN_MAP(value);
> > +
> I don't think it is a good idea to use the extcon 'state' field like
> this. I don't even think it is possible.
>
> The state is supposed to be a bit mask, each bit indicating if a
> specific connector (or functionality) on the cable is attached. The
> extcon notifier code walks through this bit mask and determines based
> on changed bits if the notifier should be called. So the notifier in
> this case would only be called if bit 1 (EXTCON_USB) of 'state' has
> changed, but not if one of the other bits has changed. One would have
> to define 32 "virtual" cables, one for each bit, for this to work, and
> then you would have to register a notifier for each of the bits. That
> would not really make sense.
>
> Of course, that makes using the extcon notifier quite useless for our
> purpose, since we need the callback not only if a cable has been
> attached or deattached, but also if some secondary state changes. I
> don't really know myself how to solve the problem; I'll need to think
> about it some more. Maybe we can add a callback into the type-c
> infrastructure code and somehow tie into that code, but I don't know
> yet if that is feasible either.
>
> Guenter
Yes, currently, we can get the notification only when bit 0 change.
So the phy driver can know plug/unplug event.
if we need more trigger, how about set the BIT 0 for changed flag.

state = extcon_get_cable_state

state = ~state | is_plugged | flip | dfp | map

extcon_set_state(state)