Re: [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}

From: Cristian Marussi <cristian.marussi@arm.com>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21}
Date: Wed, 27 Sep 2023 13:43:17 +0100	[thread overview]
Message-ID: <ZRQjZSOFZ3EtMVKL@pluto> (raw)
In-Reply-To: <20230927115023.wjxvnpdruv37hid2@bogus>

On Wed, Sep 27, 2023 at 12:50:23PM +0100, Sudeep Holla wrote:
> On Wed, Sep 27, 2023 at 12:15:16PM +0100, Cristian Marussi wrote:
> > On Mon, Sep 25, 2023 at 11:15:57AM +0100, Sudeep Holla wrote:
> > > It is very confusing to use *_v2 for everything applicable until SCMI
> > > clock protocol version v2.0 including v1.0 for example. So let us rename
> > > such that *_v2 is used only for SCMI clock protocol v2.1 onwards. Also
> > > add comment to indicate the same explicitly.
> > > 
> > 

Hi,

> > Hi Sudeep,
> > 
> > looking back at this, indeed, I remember being unsure if it was better
> > to use the v2/v21 naming scheme or the one that this patch propose.
> > 
> > Revisiting this now, I have to say that I agree with you, but why you
> > have also renamed _v21 to v2 ? The idea was to match the exact protocol
> > version ( I see that you added a comment anyway...)
> >
> 
> OK we can do that too. I was thinking of continuous increment in the structure
> version independent of the spec version. Having _v21, _v53, ...etc looks odd
> to me. I was thinking more like _v2(v2.1 onwards) and _v3(v5.3 onwards) for
> example. Hope that clarifies and let me know if that is still confusing in
> your opinion.
> 
> > IOW, the day some further new non-backward compatible features will be
> > possibly introduced (say clock v3), we could go like:
> > 
> >  - _config_set_v21: only v2.1 (the one you have renamed to v2)
> >  - _config_set_v3: only v3
> >  - _config_set : everything else, i.e. up to v2.0 (as you've renamed
> >    now)
> >
> 
> Correct. My main worry is if things get changed at random minor version
> like v2.1, v4.5, v5.3, ...etc. Unlikely to happen but not ruled out 😉,
> blame spec authors.
>

I think using a progressive continuos increment plus a comment should be
fine indeed (like this series does): this is anyway a special case, there
are anyway already many other places in the stack where we have so small
changes from one version to another that can be handled within the same
function version just with an if-check

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel