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

[Amit Sunil Dhamne <amitsd@google.com>]

Hi Heikki,

On 5/14/25 1:12 AM, Heikki Krogerus wrote:
> 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;

The only reason I asked for macros is that in the case of Spec Revision
for header, the value for PD 3.0 is 0x2, PD 2.0 is 0x1 & so on. While
for PD max revisions, it's the exact values. Having a clear distinction
may be easier to follow. If you want to go with the +/- approach you can
add a comment stating the above. 

I don't have a hard opinion on either approach :).

Thanks,

Amit

>>>
>>> --
>>> 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,
>