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;
MIME-Version: 1.0
References: <20210115163311.391332-1-kyletso@google.com> <20210121084101.GC423216@kuha.fi.intel.com>
In-Reply-To: <20210121084101.GC423216@kuha.fi.intel.com>
From:   Kyle Tso <kyletso@google.com>
Date:   Thu, 21 Jan 2021 17:48:46 +0800
Message-ID: <CAGZ6i=0M6=cZpfm=Eu4s4XTjwz5GPbpStNNOdjnPAkJ1y2MRRA@mail.gmail.com>
Subject: Re: [PATCH] usb: typec: tcpm: Create legacy PDOs for PD2 connection
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Greg KH <gregkh@linuxfoundation.org>,
        Badhri Jagan Sridharan <badhri@google.com>,
        USB <linux-usb@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jan 21, 2021 at 4:41 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Kyle,
>
> On Sat, Jan 16, 2021 at 12:33:11AM +0800, Kyle Tso wrote:
> > If the port partner is PD2, the PDOs of the local port should follow the
> > format defined in PD2 Spec. Dynamically modify the pre-defined PD3 PDOs
> > and transform them into PD2 format before sending them to the PD2 port
> > partner.
>
> I guess it's not possible for the system to supply separate static
> PDOs for each PD revision?
>
We can do that for sure. But a problem is that if there are more PD
revisions in the future, we will need to add more PDO arrays.
For backward compatibility, the new revision usually uses the
previously-reserved bits for the new features.
From my perspective, the better way to achieve the backward
compatibility is to just clear the bits if those are reserved in the
previous revision.

I can submit another patch which adds another PDO array for PD2 if you
think it is more appropriate.

> > Signed-off-by: Kyle Tso <kyletso@google.com>
> > ---
> >  drivers/usb/typec/tcpm/tcpm.c | 62 +++++++++++++++++++++++++++++------
> >  include/linux/usb/pd.h        |  1 +
> >  2 files changed, 53 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > index 22a85b396f69..1220ab1ed47d 100644
> > --- a/drivers/usb/typec/tcpm/tcpm.c
> > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > @@ -911,13 +911,47 @@ static int tcpm_set_pwr_role(struct tcpm_port *port, enum typec_role role)
> >       return 0;
> >  }
> >
> > +/*
> > + * Transform the PDO to be compliant to PD rev2.0.
> > + * Return 0 if the PDO type is not defined in PD rev2.0.
> > + * Otherwise, return the converted PDO.
> > + */
> > +static u32 tcpm_forge_legacy_pdo(struct tcpm_port *port, u32 pdo, enum typec_role role)
> > +{
> > +     switch (pdo_type(pdo)) {
> > +     case PDO_TYPE_FIXED:
> > +             if (role == TYPEC_SINK)
> > +                     return pdo & ~PDO_FIXED_FRS_CURR_MASK;
> > +             else
> > +                     return pdo & ~PDO_FIXED_UNCHUNK_EXT;
> > +     case PDO_TYPE_VAR:
> > +     case PDO_TYPE_BATT:
> > +             return pdo;
> > +     case PDO_TYPE_APDO:
> > +     default:
> > +             return 0;
> > +     }
> > +}
> > +
> >  static int tcpm_pd_send_source_caps(struct tcpm_port *port)
> >  {
> >       struct pd_message msg;
> > -     int i;
> > +     u32 pdo;
> > +     unsigned int i, nr_pdo = 0;
>
> Side note. I think this driver uses the "reverse christmas tree"
> style with the variables.

I will change the order (if there is a next version)

thanks,
Kyle