| Message ID | 20230925101557.3839860-1-sudeep.holla@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21} | expand |
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 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...) 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) I have no string opinions anyway, so I am fine also with this version of the patch...better than the original brain-dead naming scheme that I had chosen indeed :P Thanks, Cristian
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 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
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
On Wed, Sep 27, 2023 at 01:43:17PM +0100, Cristian Marussi wrote: > 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
On Mon, 25 Sep 2023 11:15:57 +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. > Applied to sudeep.holla/linux (for-next/scmi/updates), thanks! [1/1] firmware: arm_scmi: Rename scmi_{msg_,}clock_config_{get,set}_{2,21} https://git.kernel.org/sudeep.holla/c/8b6022be4c6e -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scmi/clock.c b/drivers/firmware/arm_scmi/clock.c index d18bf789fc24..9c0e33c1efab 100644 --- a/drivers/firmware/arm_scmi/clock.c +++ b/drivers/firmware/arm_scmi/clock.c @@ -46,12 +46,13 @@ struct scmi_msg_resp_clock_attributes { __le32 clock_enable_latency; }; -struct scmi_msg_clock_config_set_v2 { +struct scmi_msg_clock_config_set { __le32 id; __le32 attributes; }; -struct scmi_msg_clock_config_set_v21 { +/* Valid only from SCMI clock v2.1 */ +struct scmi_msg_clock_config_set_v2 { __le32 id; __le32 attributes; #define NULL_OEM_TYPE 0 @@ -429,13 +430,13 @@ static int scmi_clock_rate_set(const struct scmi_protocol_handle *ph, } static int -scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id, - enum clk_state state, u8 __unused0, u32 __unused1, - bool atomic) +scmi_clock_config_set(const struct scmi_protocol_handle *ph, u32 clk_id, + enum clk_state state, u8 __unused0, u32 __unused1, + bool atomic) { int ret; struct scmi_xfer *t; - struct scmi_msg_clock_config_set_v2 *cfg; + struct scmi_msg_clock_config_set *cfg; if (state >= CLK_STATE_RESERVED) return -EINVAL; @@ -457,15 +458,16 @@ scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id, return ret; } +/* For SCMI clock v2.1 and onwards */ static int -scmi_clock_config_set_v21(const struct scmi_protocol_handle *ph, u32 clk_id, - enum clk_state state, u8 oem_type, u32 oem_val, - bool atomic) +scmi_clock_config_set_v2(const struct scmi_protocol_handle *ph, u32 clk_id, + enum clk_state state, u8 oem_type, u32 oem_val, + bool atomic) { int ret; u32 attrs; struct scmi_xfer *t; - struct scmi_msg_clock_config_set_v21 *cfg; + struct scmi_msg_clock_config_set_v2 *cfg; if (state == CLK_STATE_RESERVED || (!oem_type && state == CLK_STATE_UNCHANGED)) @@ -513,10 +515,11 @@ static int scmi_clock_disable(const struct scmi_protocol_handle *ph, u32 clk_id, NULL_OEM_TYPE, 0, atomic); } +/* For SCMI clock v2.1 and onwards */ static int -scmi_clock_config_get_v21(const struct scmi_protocol_handle *ph, u32 clk_id, - u8 oem_type, u32 *attributes, bool *enabled, - u32 *oem_val, bool atomic) +scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id, + u8 oem_type, u32 *attributes, bool *enabled, + u32 *oem_val, bool atomic) { int ret; u32 flags; @@ -556,9 +559,9 @@ scmi_clock_config_get_v21(const struct scmi_protocol_handle *ph, u32 clk_id, } static int -scmi_clock_config_get_v2(const struct scmi_protocol_handle *ph, u32 clk_id, - u8 oem_type, u32 *attributes, bool *enabled, - u32 *oem_val, bool atomic) +scmi_clock_config_get(const struct scmi_protocol_handle *ph, u32 clk_id, + u8 oem_type, u32 *attributes, bool *enabled, + u32 *oem_val, bool atomic) { int ret; struct scmi_xfer *t; @@ -781,11 +784,11 @@ static int scmi_clock_protocol_init(const struct scmi_protocol_handle *ph) if (PROTOCOL_REV_MAJOR(version) >= 0x2 && PROTOCOL_REV_MINOR(version) >= 0x1) { - cinfo->clock_config_set = scmi_clock_config_set_v21; - cinfo->clock_config_get = scmi_clock_config_get_v21; - } else { cinfo->clock_config_set = scmi_clock_config_set_v2; cinfo->clock_config_get = scmi_clock_config_get_v2; + } else { + cinfo->clock_config_set = scmi_clock_config_set; + cinfo->clock_config_get = scmi_clock_config_get; } cinfo->version = version;
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. Cc: Cristian Marussi <cristian.marussi@arm.com> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- drivers/firmware/arm_scmi/clock.c | 41 +++++++++++++++++-------------- 1 file changed, 22 insertions(+), 19 deletions(-)