From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751674AbcFRPpv (ORCPT ); Sat, 18 Jun 2016 11:45:51 -0400 Received: from mail-qk0-f170.google.com ([209.85.220.170]:33286 "EHLO mail-qk0-f170.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751377AbcFRPpu (ORCPT ); Sat, 18 Jun 2016 11:45:50 -0400 MIME-Version: 1.0 In-Reply-To: <1465810789-22303-3-git-send-email-zyw@rock-chips.com> References: <1465810789-22303-1-git-send-email-zyw@rock-chips.com> <1465810789-22303-3-git-send-email-zyw@rock-chips.com> From: Guenter Roeck Date: Sat, 18 Jun 2016 08:45:48 -0700 Message-ID: Subject: Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399 To: Chris Zhong Cc: Douglas Anderson , Tomasz Figa , =?UTF-8?Q?Heiko_St=C3=BCbner?= , yzq@rock-chips.com, Guenter Roeck , linux-rockchip@lists.infradead.org, Kever Yang , Kishon Vijay Abraham I , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Chris, On Mon, Jun 13, 2016 at 2:39 AM, Chris Zhong 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399 Date: Sat, 18 Jun 2016 08:45:48 -0700 Message-ID: References: <1465810789-22303-1-git-send-email-zyw@rock-chips.com> <1465810789-22303-3-git-send-email-zyw@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1465810789-22303-3-git-send-email-zyw-TNX95d0MmH7DzftRWevZcw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "Linux-rockchip" Errors-To: linux-rockchip-bounces+glpar-linux-rockchip=m.gmane.org-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org To: Chris Zhong Cc: =?UTF-8?Q?Heiko_St=C3=BCbner?= , yzq-TNX95d0MmH7DzftRWevZcw@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Douglas Anderson , Tomasz Figa , Kever Yang , linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Guenter Roeck , Kishon Vijay Abraham I , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: linux-rockchip.vger.kernel.org Hi Chris, On Mon, Jun 13, 2016 at 2:39 AM, Chris Zhong 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: groeck@google.com (Guenter Roeck) Date: Sat, 18 Jun 2016 08:45:48 -0700 Subject: [v2 PATCH 2/4] phy: Add USB Type-C PHY driver for rk3399 In-Reply-To: <1465810789-22303-3-git-send-email-zyw@rock-chips.com> References: <1465810789-22303-1-git-send-email-zyw@rock-chips.com> <1465810789-22303-3-git-send-email-zyw@rock-chips.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Chris, On Mon, Jun 13, 2016 at 2:39 AM, Chris Zhong 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