Re: [PATCH] net: ipa: add IPA v5.1 and v5.5 to ipa_version_string()

["Luca Weiss" <luca.weiss@fairphone.com>]

On Mon Jul 28, 2025 at 5:53 PM CEST, Alex Elder wrote:
> On 7/28/25 3:35 AM, Luca Weiss wrote:
>> Handle the case for v5.1 and v5.5 instead of returning "0.0".
>
> This makes sense for IPA v5.5.
>
> I have comments below, but I'll do this up front:
>
> Reviewed-by: Alex Elder <elder@riscstar.com>

Thanks!

>
>> Also reword the comment below since I don't see any evidence of such a
>> check happening, and - since 5.5 has been missing - can happen.
>
> You are correct.  Commit dfdd70e24e388 ("net: ipa: kill
> ipa_version_supported()") removed the test that guaranteed
> that the version would be good.  So your comment update
> should have done then.
>
>> Fixes: 3aac8ec1c028 ("net: ipa: add some new IPA versions")
>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>> ---
>>   drivers/net/ipa/ipa_sysfs.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ipa/ipa_sysfs.c b/drivers/net/ipa/ipa_sysfs.c
>> index a59bd215494c9b7cbdd1f000d9f23e100c18ee1e..a53e9e6f6cdf50103e94e496fd06b55636ba02f4 100644
>> --- a/drivers/net/ipa/ipa_sysfs.c
>> +++ b/drivers/net/ipa/ipa_sysfs.c
>> @@ -37,8 +37,12 @@ static const char *ipa_version_string(struct ipa *ipa)
>>   		return "4.11";
>>   	case IPA_VERSION_5_0:
>>   		return "5.0";
>> +	case IPA_VERSION_5_1:
>> +		return "5.1";
>
> IPA v5.1 is not actually supported yet, and this won't be
> used until it is.  Only those platforms with compatible
> strings defined in the ipa_match array in "ipa_main.c" will
> probe successfully.
>
> That said...  I guess it's OK to define this at the same time
> things are added to enum ipa_version.  There are still too
> many little things like this that need to be updated when a
> new version is supported.

Yeah, my point in adding this as well was based on the comment there:

/**
 * [...]
 * Defines the version of IPA (and GSI) hardware present on the platform.
 * Please update ipa_version_string() whenever a new version is added.
 */
enum ipa_version {
    [...]
}

I previously only noticed 5.5 being missing, but before sending I double
checked if anything else was missing and found 5.1. So perhaps if 5.1 is
not going to be added anytime soon, we could remove the 5.1 definition
and the rest.

>
> Thanks for the patch.
>
> 					-Alex
>
>> +	case IPA_VERSION_5_5:
>> +		return "5.5";
>>   	default:
>> -		return "0.0";	/* Won't happen (checked at probe time) */
>> +		return "0.0";	/* Should not happen */
>>   	}
>>   }
>>   
>> 
>> ---
>> base-commit: 038d61fd642278bab63ee8ef722c50d10ab01e8f
>> change-id: 20250728-ipa-5-1-5-5-version_string-a557dc8bd91a
>> 
>> Best regards,