Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Oct 2021 14:10:15 +0300
From:   Heikki Krogerus <heikki.krogerus@linux.intel.com>
To:     Jack Pham <jackp@codeaurora.org>
Cc:     Prashant Malani <pmalani@chromium.org>,
        Benson Leung <bleung@google.com>,
        Adam Thomson <Adam.Thomson.Opensource@diasemi.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Badhri Jagan Sridharan <badhri@google.com>,
        "Gopal, Saranya" <saranya.gopal@intel.com>,
        "Regupathy, Rajaram" <rajaram.regupathy@intel.com>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCH 3/4] usb: typec: ucsi: Add support for PD cdev
Message-ID: <YXkzl+1mTA2pslN0@kuha.fi.intel.com>
References: <20211026143352.78387-1-heikki.krogerus@linux.intel.com>
 <20211026143352.78387-4-heikki.krogerus@linux.intel.com>
 <20211027010035.GA13345@jackp-linux.qualcomm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211027010035.GA13345@jackp-linux.qualcomm.com>
Precedence: bulk

On Tue, Oct 26, 2021 at 06:00:35PM -0700, Jack Pham wrote:
> On Tue, Oct 26, 2021 at 05:33:51PM +0300, Heikki Krogerus wrote:
> >
> > -static int ucsi_get_src_pdos(struct ucsi_connector *con)
> > +int ucsi_read_pdos(struct ucsi_connector *con, int partner, int source, u32 *pdos)
> >  {
> > +	u32 pdo[PDO_MAX_OBJECTS];
> > +	int num_pdos;
> >  	int ret;
> >  
> >  	/* UCSI max payload means only getting at most 4 PDOs at a time */
> > -	ret = ucsi_get_pdos(con, 1, con->src_pdos, 0, UCSI_MAX_PDOS);
> > +	ret = ucsi_get_pdos(con, partner, source, pdo, 0, UCSI_MAX_PDOS);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	con->num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> > -	if (con->num_pdos < UCSI_MAX_PDOS)
> > -		return 0;
> > +	num_pdos = ret / sizeof(u32); /* number of bytes to 32-bit PDOs */
> > +	if (num_pdos < UCSI_MAX_PDOS)
> > +		goto done;
> >  
> >  	/* get the remaining PDOs, if any */
> > -	ret = ucsi_get_pdos(con, 1, con->src_pdos, UCSI_MAX_PDOS,
> > +	ret = ucsi_get_pdos(con, partner, source, pdo, UCSI_MAX_PDOS,
> >  			    PDO_MAX_OBJECTS - UCSI_MAX_PDOS);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	con->num_pdos += ret / sizeof(u32);
> > +	num_pdos += ret / sizeof(u32);
> > +done:
> > +	memcpy(pdos, pdo, num_pdos * sizeof(pdo));
> > +
> > +	return num_pdos;
> > +}
> > +
> > +static int ucsi_get_src_pdos(struct ucsi_connector *con)
> > +{
> > +	int ret;
> > +
> > +	ret = ucsi_read_pdos(con, 0, 1, con->src_pdos);
> 
> Second parameter should be 1 right?  Original intent of get_src_pdos()
> is to retrieve the partner's source capabilities in order to populate
> the power_supply.  Passing 0 as the partner param here changes the
> behavior to retrieve the source PDOs of the port.

Sounds like there is a bug in the existing code. I'm not changing the
behviour in this patch.

> (BTW I'm going to send a quick patch for this to since this assumes that
> port is sink and partner is source; when it's the other way around we
> end up calling GET_PDOS on the sink partner when it might not even be
> source capable).

I'll check it...

thanks,

-- 
heikki