Re: [PATCH v2] usb: typec: tcpm: Use configured PD revision for negotiation

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Tue, May 13, 2025 at 10:14:32PM +0800, Cosmo Chou wrote:
> On Tue, May 13, 2025 at 04:50:49PM +0300, Heikki Krogerus wrote:
> >
> > On Tue, May 13, 2025 at 04:39:09PM +0300, Heikki Krogerus wrote:
> > > On Tue, May 13, 2025 at 09:08:34PM +0800, Cosmo Chou wrote:
> > > > Initialize negotiated_rev and negotiated_rev_prime based on the port's
> > > > configured PD revision (rev_major) rather than always defaulting to
> > > > PD_MAX_REV. This ensures ports start PD communication using their
> > > > appropriate revision level.
> > > >
> > > > This allows proper communication with devices that require specific
> > > > PD revision levels, especially for the hardware designed for PD 1.0
> > > > or 2.0 specifications.
> > > >
> > > > Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com>
> > > > ---
> > > > Change log:
> > > >
> > > > v2:
> > > >   - Add PD_CAP_REVXX macros and use switch-case for better readability.
> > > >
> > > > ---
> > > >  drivers/usb/typec/tcpm/tcpm.c | 29 +++++++++++++++++++++++++----
> > > >  1 file changed, 25 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> > > > index 8adf6f954633..48e9cfc2b49a 100644
> > > > --- a/drivers/usb/typec/tcpm/tcpm.c
> > > > +++ b/drivers/usb/typec/tcpm/tcpm.c
> > > > @@ -313,6 +313,10 @@ struct pd_data {
> > > >     unsigned int operating_snk_mw;
> > > >  };
> > > >
> > > > +#define PD_CAP_REV10       0x1
> > > > +#define PD_CAP_REV20       0x2
> > > > +#define PD_CAP_REV30       0x3
> > > > +
> > > >  struct pd_revision_info {
> > > >     u8 rev_major;
> > > >     u8 rev_minor;
> > > > @@ -4665,6 +4669,25 @@ static void tcpm_set_initial_svdm_version(struct tcpm_port *port)
> > > >     }
> > > >  }
> > > >
> > > > +static void tcpm_set_initial_negotiated_rev(struct tcpm_port *port)
> > > > +{
> > > > +   switch (port->pd_rev.rev_major) {
> > > > +   case PD_CAP_REV10:
> > > > +           port->negotiated_rev = PD_REV10;
> > > > +           break;
> > > > +   case PD_CAP_REV20:
> > > > +           port->negotiated_rev = PD_REV20;
> > > > +           break;
> > > > +   case PD_CAP_REV30:
> > > > +           port->negotiated_rev = PD_REV30;
> > > > +           break;
> > > > +   default:
> > > > +           port->negotiated_rev = PD_MAX_REV;
> > > > +           break;
> > > > +   }
> > > > +   port->negotiated_rev_prime = port->negotiated_rev;
> > > > +}
> > >
> > > Do we need this? Couldn't you just add one to rev_major?
> > >
> > >         port->negotiated_rev = port->pd_rev.rev_major + 1;
> > >         port->negotiated_rev_prime = port->pd_rev.rev_major + 1;
> > >
> > > Or am I missing something?
> >
> > Sorry, I mean minus one :-)
> >
> >         port->negotiated_rev = port->pd_rev.rev_major - 1;
> >         port->negotiated_rev_prime = port->pd_rev.rev_major - 1;
> >
> > --
> > heikki
> 
> It seems to be the PATCH v1:
> https://lore.kernel.org/all/20250508174756.1300942-1-chou.cosmo@gmail.com/
> 
> if (port->pd_rev.rev_major > 0 && port->pd_rev.rev_major <= PD_MAX_REV + 1) {
>         port->negotiated_rev = port->pd_rev.rev_major - 1;
>         port->negotiated_rev_prime = port->pd_rev.rev_major - 1;
> } else {
>         port->negotiated_rev = PD_MAX_REV;
>         port->negotiated_rev_prime = PD_MAX_REV;
> }

Okay, sorry I missed that. I still don't like the extra definitions,
but I don't have any better idea (I guess macro is not an option?).

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

thanks,

-- 
heikki