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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 20 Jun 2016 13:57:16 +0800
From:	Chris Zhong <zyw@...k-chips.com>
To:	Guenter Roeck <groeck@...gle.com>
Cc:	Douglas Anderson <dianders@...omium.org>,
	Tomasz Figa <tfiga@...omium.org>,
	Heiko Stübner <heiko@...ech.de>,
	yzq@...k-chips.com, Guenter Roeck <groeck@...omium.org>,
	linux-rockchip@...ts.infradead.org,
	Kever Yang <kever.yang@...k-chips.com>,
	Kishon Vijay Abraham I <kishon@...com>,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399

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@...k-chips.com> 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)