Received: by 2002:a05:6a10:1a4d:0:0:0:0 with SMTP id nk13csp5221572pxb; Sun, 6 Feb 2022 18:14:01 -0800 (PST) X-Google-Smtp-Source: ABdhPJz81X5rxWzpF1/SLR+lnC1Q2KhVWDCTmXO0+uyp23M7eFrBtTm2doTCvVXumxBS7kkrZe7C X-Received: by 2002:a63:2cc6:: with SMTP id s189mr7595448pgs.285.1644200041546; Sun, 06 Feb 2022 18:14:01 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1644200041; cv=none; d=google.com; s=arc-20160816; b=Dz+cropMO42RgZRRkTgSShlxJ2tt6t9Ev+RS0m0GYXOuxIXTAud+Op6z6nFMCFIBVV 5uIDxNSS2HSbcEpQNO0Wv2jb71GoFyzRi+JsDjUEg0ZFBSxWFjWf+7tvSDZCDLmWwIEZ mIo8EHPEKrgGGL01AyPTbRLTG7UbBPqefxWLnQhbW6cd/BPyVRfkJliV28qBqMJT+hov 7YgkfNNZou3M1X0jweyRKfa+Pm8pIiij9HXHMBW1qd9uN5ai6akoYeSF1oYQTFUXqMae 3Xx/eIciom2IPB7v3LVP8e9V+MHad2cmwdHbivav6pb34ftOPaS54b78xwJk5lQMImKb Plhw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-disposition:mime-version :references:message-id:subject:cc:to:from:date:dkim-signature; bh=dK3cQrSLKuhf+Hx949qAygrnDs/dqSOx3NSZF+njzRw=; b=CUriqAxBw8l2cH4pyj0fsqCCriN/bv0l2LymIS9B1X/m6QR5t+QnRSU1O+sNbt74mr 4imB6LqtYlTVIjCqzujw2eQ+lsdJZKzHON2R6LMbb6rDmHkt6uifOlEivppwpXBgIA+I hXxSAJshH24fNENEsznunhFGUjCzDS3Fs8SzjTwSGgbUXuxLk8eBkLmlyR0KLP2BkKXq Fzesti7vwSjs9xuLUyjvNyANFyMqt/cVcWRT4M0J9HjZJQ1rnG3LkfloM+zoZkFmSsng lBigBQZ0XvrqNBqPwZqIu/7FA4ZlWI/SYgMrU4R/0Rw1r+SZ/AShocD0fKHlAuoLCSYH D+ow== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=jDsv5vYm; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id c3si9332520plh.48.2022.02.06.18.13.47; Sun, 06 Feb 2022 18:14:01 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@intel.com header.s=Intel header.b=jDsv5vYm; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=intel.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1357143AbiBDPHf (ORCPT + 99 others); Fri, 4 Feb 2022 10:07:35 -0500 Received: from mga02.intel.com ([134.134.136.20]:19976 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232560AbiBDPHe (ORCPT ); Fri, 4 Feb 2022 10:07:34 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1643987254; x=1675523254; h=date:from:to:cc:subject:message-id:references: mime-version:in-reply-to; bh=h/9khmtg2t68pwXtFE8VZDwdVbkQjmJgc0X80tEipNI=; b=jDsv5vYmAJrItxxCVe8bX9z/mMBlLAycuMc0K/WnkX355tUWzynV3hnD bIAKOK/sw/pgzntlxAXSNHQAwnbrrRgF09NkXbODM989SoNuZX8C9mfrh JyvPwWTjKbskrN4DqhYFMdUwLPtQX6JWREK1Q9ERyB+RBqBB/Ru5JIu0V 3l/i1backYydFsUks9eSuhe84VuYeBv/wfoqXapyQXkaOOpRJRGTx3Pya KUEBPY41Iq9Wsifq4/sc3ZUvX1lnfsvY7TYy1L9E5MuBZQyqARpuxnK8D qHOaf0FCjbjUUYh8kg93N4xk2Ykx9QatbTj88OhO0DYcpzwaiVKTQrapb w==; X-IronPort-AV: E=McAfee;i="6200,9189,10247"; a="235780881" X-IronPort-AV: E=Sophos;i="5.88,343,1635231600"; d="scan'208";a="235780881" Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Feb 2022 07:07:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.88,343,1635231600"; d="scan'208";a="677119496" Received: from kuha.fi.intel.com ([10.237.72.185]) by fmsmga001.fm.intel.com with SMTP; 04 Feb 2022 07:07:31 -0800 Received: by kuha.fi.intel.com (sSMTP sendmail emulation); Fri, 04 Feb 2022 17:07:30 +0200 Date: Fri, 4 Feb 2022 17:07:30 +0200 From: Heikki Krogerus To: Greg Kroah-Hartman Cc: Guenter Roeck , Benson Leung , Prashant Malani , Jameson Thies , "Regupathy, Rajaram" , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v1 1/3] usb: typec: Separate USB Power Delivery from USB Type-C Message-ID: References: <20220203144657.16527-1-heikki.krogerus@linux.intel.com> <20220203144657.16527-2-heikki.krogerus@linux.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Feb 04, 2022 at 02:59:26PM +0100, Greg Kroah-Hartman wrote: > On Fri, Feb 04, 2022 at 12:04:41PM +0200, Heikki Krogerus wrote: > > Hi Greg, > > > > On Thu, Feb 03, 2022 at 03:55:19PM +0100, Greg Kroah-Hartman wrote: > > > On Thu, Feb 03, 2022 at 05:46:55PM +0300, Heikki Krogerus wrote: > > > > +/* These additional details are only available with vSafe5V supplies */ > > > > +static struct kobj_attribute dual_role_power_attr = __ATTR_RO(dual_role_power); > > > > +static struct kobj_attribute usb_suspend_supported_attr = __ATTR_RO(usb_suspend_supported); > > > > +static struct kobj_attribute unconstrained_power_attr = __ATTR_RO(unconstrained_power); > > > > +static struct kobj_attribute usb_communication_capable_attr = __ATTR_RO(usb_communication_capable); > > > > +static struct kobj_attribute dual_role_data_attr = __ATTR_RO(dual_role_data); > > > > +static struct kobj_attribute > > > > +unchunked_extended_messages_supported_attr = __ATTR_RO(unchunked_extended_messages_supported); > > > > > > Note, no 'struct device' should ever have a "raw" kobject hanging off of > > > it. If so, something went wrong. > > > > > > If you do this, userspace will never be notified of the attributes and > > > any userspace representation of the tree will be messed up. > > > > > > Please, use an attribute directory with a name, or if you really need to > > > go another level deep, use a real 'struct device'. As-is here, I can't > > > take it. > > > > OK, got it. I don't think we can avoid the deeper levels, not without > > making this really cryptic, and not really usable in all cases. These > > objects are trying to represent (parts) of the protocol - the > > messages, the objects in those messages, and later the responses to > > those messages. > > > > But I'm also trying to avoid having to claim that these objects are > > "devices", because honestly, claiming that the packages used in > > communication are devices is confusing, and just wrong. If we take > > that road, then we really should redefine what struct device is > > supposed to represent, and rename it also. > > Fair enough, this isn't really a device, it's an "attribute" of your > device you are wanting to show. It's just that you are really "deep". > > You asked for: > > /sys/class/typec/port0/usb_power_delivery > |-- revision > |-- sink_capabilities/ > | |-- 1:fixed_supply/ > | | |-- dual_role_data > | | |-- dual_role_power > | | |-- fast_role_swap_current > | | |-- operational_current > | | |-- unchunked_extended_messages_supported > | | |-- unconstrained_power > | | |-- usb_communication_capable > | | |-- usb_suspend_supported > | | `-- voltage > | |-- 2:variable_supply/ > | | |-- maximum_voltage > | | |-- minimum_voltage > | | `-- operational_current > | `-- 3:battery/ > | |-- maximum_voltage > | |-- minimum_voltage > | `-- operational_power > `-- source_capabilities/ > `-- 1:fixed_supply/ > |-- dual_role_data > |-- dual_role_power > |-- maximum_current > |-- unchunked_extended_messages_supported > |-- unconstrained_power > |-- usb_communication_capable > |-- usb_suspend_supported > `-- voltage > > > To start with, your "attribute" is really "usb_power_delivery" here, so > you can just use an attribute group name to get the "revision" file. > > But then the later ones could be flat in that directory as well, using a > ':' to split as you did already, and the above could turn into: > > /sys/class/typec/port0/usb_power_delivery > |-- revision > |-- sink_capabilites:1:fixed_supply:dual_role_data > |-- sink_capabilites:1:fixed_supply:dual_role_power > |-- sink_capabilites:1:fixed_supply:fase_role_swap_current > .... > |-- sink_capabilites:2:variable_supply:maximum_voltage > |-- sink_capabilites:2:variable_supply:minimum_voltage > ... > |-- source_capabilities:1:fixed_supply:dual_role_data > |-- source_capabilities:1:fixed_supply:dual_role_power > |-- source_capabilities:1:fixed_supply:maximum_current > ... > > But ick, that's also a mess as you are now forced to parse filenames in > userspace in a different way than "normal". > > Is there anything special about the number here? It's the "position" > which will be unique. So make that position a device, as that's kind of > what it is (like usb endpoints are devices) > > Then you could make a bus for the positions and all would be good, and > you could turn this into: > > > /sys/class/typec/port0/usb_power_delivery > |-- revision > |-- sink_capabilities:1/ > | `-- fixed_supply/ > | |-- dual_role_data > | |-- dual_role_power > | |-- fast_role_swap_current > | |-- operational_current > | |-- unchunked_extended_messages_supported > | |-- unconstrained_power > | |-- usb_communication_capable > | |-- usb_suspend_supported > | `-- voltage > |-- sink_capabilities:2/ > | `-- variable_supply/ > | |-- maximum_voltage > | |-- minimum_voltage > | `-- operational_current > |-- sink_capabilities:3/ > | `-- battery/ > | |-- maximum_voltage > | |-- minimum_voltage > | `-- operational_power > `-- source_capabilities:1/ > `-- fixed_supply/ > |-- dual_role_data > |-- dual_role_power > |-- maximum_current > |-- unchunked_extended_messages_supported > |-- unconstrained_power > |-- usb_communication_capable > |-- usb_suspend_supported > `-- voltage > > Would that work? Unfortunately the object position is only defined for these capability messages, not for the other messages. It's not going to work :-( > > So would it be OK that, instead of registering these objects as > > devices, we just introduce a kset where we can group them > > (/sys/kernel/usb_power_delivery)? > > You want to show this as attched to a specific port somehow, so that > location is not going to work. But the idea with that kset would be that you have a separate directory for each port there for this stuff: /sys/kernel/usb_power_delivery/port0 |-- revision ... And those directories we could then link to the actual device: /sys/class/typec/port0/usb_power_delivery -> ../../../kernel/usb_power_delivery/port0 thanks, -- heikki