From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932167AbcEMRsM (ORCPT ); Fri, 13 May 2016 13:48:12 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:51291 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751573AbcEMRsL (ORCPT ); Fri, 13 May 2016 13:48:11 -0400 Date: Fri, 13 May 2016 10:48:10 -0700 From: Guenter Roeck To: Heikki Krogerus Cc: Greg KH , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Mathias Nyman , Felipe Balbi Subject: Re: [PATCH 0/3] usb: USB Type-C Class and driver for UCSI Message-ID: <20160513174810.GA1999@roeck-us.net> References: <1455037283-106479-1-git-send-email-heikki.krogerus@linux.intel.com> <20160505030544.GA25632@roeck-us.net> <20160506080840.GB29820@kuha.fi.intel.com> <5732A39A.8070807@roeck-us.net> <20160511094011.GA26869@kuha.fi.intel.com> <20160511144718.GC8192@roeck-us.net> <20160513142321.GA21576@kuha.fi.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160513142321.GA21576@kuha.fi.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Authenticated_sender: guenter@roeck-us.net X-OutGoing-Spam-Status: No, score=-1.0 X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - bh-25.webhostbox.net X-AntiAbuse: Original Domain - vger.kernel.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - roeck-us.net X-Get-Message-Sender-Via: bh-25.webhostbox.net: authenticated_id: guenter@roeck-us.net X-Authenticated-Sender: bh-25.webhostbox.net: guenter@roeck-us.net X-Source: X-Source-Args: X-Source-Dir: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Fri, May 13, 2016 at 05:23:21PM +0300, Heikki Krogerus wrote: > Hi, > > On Wed, May 11, 2016 at 07:47:18AM -0700, Guenter Roeck wrote: > > Hi, > > > > On Wed, May 11, 2016 at 12:40:11PM +0300, Heikki Krogerus wrote: > > > On Tue, May 10, 2016 at 08:14:34PM -0700, Guenter Roeck wrote: > > > > Heikki, > > > > > > > > On 05/06/2016 01:08 AM, Heikki Krogerus wrote: > > > > > Hi, > > > > > > > > > [ ... ] > > > > > > > > > > I don't have not made any new code for the class driver yet, but I'm > > > > > attempting to prepare v2 next week. > > > > > > > > > Would it make sense to send feedback about v1 now, or should I wait for v2 ? > > > > > > I don't think I'm able to send v2 today, or even tomorrow, so feel > > > free to give the feedback. Just be aware that I've rewritten the > > > alternate mode part completely. > > > > > Alternate mode handling was my major concern, actually. > > > > > I'm creating a separate device for the partner and also the cable > > > during connection. I'm also already going to introduce a small bus for > > > the AltModes. It's clear that we need to have AltMode specific > > > drivers. The generic parts can't take care of all the AltMode specific > > > requirements and VDMs. The bus will give us a nice way to bind those > > > drivers to the actual AltModes a partner and the cable plugs offer. > > > > > > So if there are dependencies between the altmodes, for example if the > > > cable plugs needs to be in a certain mode in order for the partner to > > > be able to function in some specific mode, the responsibility of > > > taking care of those will fall primarily to in the AltMode drivers. > > > So not userspace. > > > > > > The AltMode drivers actually are useful also as they can be part of > > > the relevant frameworks, for example DP in some graphics framework. > > > For example in case of DP, the number of lanes (I guess 2 or 4) should > > > be ideally known if I have understood correctly. Knowledge about the > > > connection seems to also be needed, and I've so far seen some pretty > > > weird solutions for hotplug events with the DP AltMode. With the > > > driver we should be able to avoid those. > > > > > > But in any case, every SVIDs a partner (or plug) offers will have > > > their own device registered with the partner (or cable) itself as > > > parent in this design. I'm expecting a little bit of conversation > > > about this plan, but right now I feel confident about it. > > > > > > How does this sound to you? > > > > > Looking forward to it. My major problem so far was that alternate mode > > handling is very platform specific, which didn't seem to be well supported > > in v1 of your patch. I thought about implementing a hierarchy of drivers > > below the type-c class to solve that problem. Looks like you just solved > > it for me. > > > > Other than that, my major concern is the lack of synchronization/protection > > between the type-c class and the drivers. Setting port parameters (data role, > > power role, operational power role, partner alternate modes, partner type) > > from registered drivers may need to be synchronzed/protected. For example, > > data and power role are set during connection establishment, but can be > > overwritten from the typec class code. Right now I am just setting the > > respective variables in struct typec_port directly, but that doesn't seem > > right. > > I'm actually struggling with this same question. I decided to protect > the whole struct typec_port by not allowing the drivers to touch it, > but I'm still working on it.. > Sounds good to me, as long as you provide APIs to change the values. > > For partner_type, I don't really know how to map the options to the identity > > reported by the partner. The reported product types are unknown / hub / > > peripheral / passive cable / active cable / alternate mode adapter. > > The available partner types are unknown / USB / Charger / Alternate Mode / > > Audio Accessory / Debug Accessory. What am I missing here ? > > The partner types don't map directly to the USB PD Product Types. We > need to describe the partner even when USB PD is not available. > Accessory Modes are for example out side the scope of USB PD. > > But I'll try to propose something for those. > Ok. > > The rest is just nitpicks. > > > > - alternate_modes_show() and partner_alt_modes_show() discard the last byte > > of the generated string and replace it with \0. > > - s/Accessroy/Accessory/ > > - typec_connect() and typec_disconnect() should probably also set > > port->connected. > > OK. I'll check these. > > So I failed to finish my proposal for v2 this week. On top of the > sync/protection problems, I'm also still trying to tune my AltMode > bus. I'm going to attempt to send it as an RFC on Monday or Tuesday in > any case. > No worries. It turns out the PD code in existing type-c devices isn't exactly stable or predictable, so I end up spending a lot of time trying to stabilize my state machine anyway. Thanks, Guenter