linux-kernel - Re: [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode for the USB connector

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]

Message-ID: <20190508113017.GC19816@kuha.fi.intel.com>
Date:   Wed, 8 May 2019 14:30:17 +0300
From:   Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        linux-acpi@...r.kernel.org, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org,
        Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v3 11/13] platform/x86: intel_cht_int33fe: Provide fwnode
 for the USB connector

On Wed, Apr 17, 2019 at 11:52:00AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 12-04-19 15:41, Heikki Krogerus wrote:
> > In ACPI, and now also in DT, the USB connectors usually have
> > their own device nodes. In case of USB Type-C, those
> > connector (port) nodes are child nodes of the controller or
> > PHY device, in our case the fusb302. The software fwnodes
> > allow us to create a similar child node for fusb302 that
> > represents the connector also on Intel CHT.
> > 
> > This makes it possible replace the fusb302 specific device
> > properties which were deprecated with the common USB
> > connector properties that tcpm.c is able to use directly.
> > 
> > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> > ---
> >   drivers/platform/x86/intel_cht_int33fe.c | 37 ++++++++++++++++++++++--
> >   1 file changed, 34 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
> > index 863a792d9282..eff5990322ff 100644
> > --- a/drivers/platform/x86/intel_cht_int33fe.c
> > +++ b/drivers/platform/x86/intel_cht_int33fe.c
> > @@ -24,6 +24,7 @@
> >   #include <linux/platform_device.h>
> >   #include <linux/regulator/consumer.h>
> >   #include <linux/slab.h>
> > +#include <linux/usb/pd.h>
> >   #define EXPECTED_PTYPE		4
> > @@ -31,6 +32,7 @@ enum {
> >   	INT33FE_NODE_FUSB302,
> >   	INT33FE_NODE_MAX17047,
> >   	INT33FE_NODE_PI3USB30532,
> > +	INT33FE_NODE_USB_CONNECTOR,
> >   	INT33FE_NODE_MAX,
> >   };
> > @@ -111,9 +113,29 @@ cht_int33fe_register_max17047(struct device *dev, struct cht_int33fe_data *data)
> >   static const struct property_entry fusb302_props[] = {
> >   	PROPERTY_ENTRY_STRING("linux,extcon-name", "cht_wcove_pwrsrc"),
> > -	PROPERTY_ENTRY_U32("fcs,max-sink-microvolt", 12000000),
> > -	PROPERTY_ENTRY_U32("fcs,max-sink-microamp",   3000000),
> > -	PROPERTY_ENTRY_U32("fcs,max-sink-microwatt", 36000000),
> 
> Note that the 36000000 value being removed here is max-sink-microwatt,
> esp. the _max_ part is important. And recent versions of the fusb302
> code ignore this entirely.
> 
> > +	{ }
> > +};
> > +
> > +#define PDO_FIXED_FLAGS \
> > +	(PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP | PDO_FIXED_USB_COMM)
> > +
> > +static const u32 src_pdo[] = {
> > +	PDO_FIXED(5000, 1500, PDO_FIXED_FLAGS),
> > +};
> > +
> > +static const u32 snk_pdo[] = {
> > +	PDO_FIXED(5000, 400, PDO_FIXED_FLAGS),
> > +	PDO_VAR(5000, 12000, 3000),
> > +};
> > +
> > +static const struct property_entry usb_connector_props[] = {
> > +	PROPERTY_ENTRY_STRING("name", "connector"),
> > +	PROPERTY_ENTRY_STRING("data-role", "dual"),
> > +	PROPERTY_ENTRY_STRING("power-role", "dual"),
> > +	PROPERTY_ENTRY_STRING("try-power-role", "sink"),
> > +	PROPERTY_ENTRY_U32_ARRAY("source-pdos", src_pdo),
> > +	PROPERTY_ENTRY_U32_ARRAY("sink-pdos", snk_pdo),
> > +	PROPERTY_ENTRY_U32("op-sink-microwatt", 36000000),
> 
> Where as "op-sink-microwatt" is more interpreted as a minimum
> value for non PPS supplies not being able to deliver this causes
> the Capability Mismatch to get set. But for PPS supplies if I'm
> reading the code correctly, the entire PPS negotiation is failed
> by tcpm.c if this cannot be matched. I guess / hope that there
> is a fallback to none PPS PDOs then but I'm not sure.

OK. I'll change that to the current default value, 2500000.

> Anyways the charger the GPD-win ships with is a non PD capable
> 5V/2A charger and the GPD-pocket ships with a charger which does
> max 12V/2A. The device itself will work fine on around 10W and
> even charge at that level (albeit slowly). So I believe that 10W
> would be a better value for "op-sink-microwatt" (the dt-binding
> says it is mandatory so we cannot just leave it out).

I have no objections. If you prefer, I can include a separate patch
where I change the value to 10W.

thanks,

-- 
heikki