Re: [PATCH 03/20] bus: mhi: Make mhi_state_str[] array static const and move to common.h

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Alex Elder <elder@ieee.org>
Cc: mhi@lists.linux.dev, hemantk@codeaurora.org,
	bbhatt@codeaurora.org, quic_jhugo@quicinc.com,
	vinod.koul@linaro.org, bjorn.andersson@linaro.org,
	dmitry.baryshkov@linaro.org, skananth@codeaurora.org,
	vpernami@codeaurora.org, vbadigan@codeaurora.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 03/20] bus: mhi: Make mhi_state_str[] array static const and move to common.h
Date: Thu, 3 Feb 2022 16:04:44 +0530	[thread overview]
Message-ID: <20220203103444.GB6298@thinkpad> (raw)
In-Reply-To: <0d850b27-5684-2ecf-fc96-3258c0462d3d@ieee.org>

On Wed, Jan 05, 2022 at 06:22:59PM -0600, Alex Elder wrote:
> On 12/2/21 5:35 AM, Manivannan Sadhasivam wrote:
> > mhi_state_str[] array could be used by MHI endpoint stack also. So let's
> > make the array as "static const" and move it inside the "common.h" header
> > so that the endpoint stack could also make use of it. Otherwise, the
> > structure definition should be present in both host and endpoint stack and
> > that'll result in duplication.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> This result in common source code (which is good), but it will be
> duplicated in everything that includes this file.
> 
> Do you have no common code, available to both the endpoint and host?
> You could (in drivers/bus/mhi/common.c, for example).
> 
> If you don't, I have a different suggestion, below.  It does
> basically the same thing you're doing here, but I much prefer
> duplicating an inline function than a data structure.
> 
> > ---
> >   drivers/bus/mhi/common.h    | 13 ++++++++++++-
> >   drivers/bus/mhi/host/init.c | 12 ------------
> >   2 files changed, 12 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/bus/mhi/common.h b/drivers/bus/mhi/common.h
> > index 0f4f3b9f3027..2ea438205617 100644
> > --- a/drivers/bus/mhi/common.h
> > +++ b/drivers/bus/mhi/common.h
> > @@ -174,7 +174,18 @@ struct mhi_cmd_ctxt {
> >   	__u64 wp __packed __aligned(4);
> >   };
> > -extern const char * const mhi_state_str[MHI_STATE_MAX];
> > +static const char * const mhi_state_str[MHI_STATE_MAX] = {
> > +	[MHI_STATE_RESET] = "RESET",
> > +	[MHI_STATE_READY] = "READY",
> > +	[MHI_STATE_M0] = "M0",
> > +	[MHI_STATE_M1] = "M1",
> > +	[MHI_STATE_M2] = "M2",
> > +	[MHI_STATE_M3] = "M3",
> > +	[MHI_STATE_M3_FAST] = "M3 FAST",
> > +	[MHI_STATE_BHI] = "BHI",
> > +	[MHI_STATE_SYS_ERR] = "SYS ERROR",
> > +};
> > +
> >   #define TO_MHI_STATE_STR(state) ((state >= MHI_STATE_MAX || \
> >   				  !mhi_state_str[state]) ? \
> >   				"INVALID_STATE" : mhi_state_str[state])
> 
> You could easily and safely define this as an inline function instead.
> 

Sounds good!

> #define MHI_STATE_CASE(x)	case MHI_STATE_ ## x: return #x
> static inline const char *mhi_state_string(enum mhi_state state)
> {
> 	switch(state) {
> 	MHI_STATE_CASE(RESET);
> 	MHI_STATE_CASE(READY);
> 	MHI_STATE_CASE(M0);
> 	MHI_STATE_CASE(M1);
> 	MHI_STATE_CASE(M2);
> 	MHI_STATE_CASE(M3_FAST);
> 	MHI_STATE_CASE(BHI);
> 	MHI_STATE_CASE(SYS_ERR);
> 	default: return "(unrecognized MHI state)";
> 	}
> }
> #undef MHI_STATE_CASE

I've used the below one:

static inline const char * const mhi_state_str(enum mhi_state state)
{
        switch(state) {
        case MHI_STATE_RESET:
                return "RESET";
        case MHI_STATE_READY:
                return "READY";
        case MHI_STATE_M0:
                return "M0";
        case MHI_STATE_M1:
                return "M1";
        case MHI_STATE_M2:
                return"M2";
        case MHI_STATE_M3:
                return"M3";
        case MHI_STATE_M3_FAST:
                return"M3 FAST";
        case MHI_STATE_BHI:
                return"BHI";
        case MHI_STATE_SYS_ERR:
                return "SYS ERROR";
        default:
                return "Unknown state";
        }
};

Thanks,
Mani

> 
> 					-Alex
> 
> > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> > index 5aaca6d0f52b..fa904e7468d8 100644
> > --- a/drivers/bus/mhi/host/init.c
> > +++ b/drivers/bus/mhi/host/init.c
> > @@ -44,18 +44,6 @@ const char * const dev_state_tran_str[DEV_ST_TRANSITION_MAX] = {
> >   	[DEV_ST_TRANSITION_DISABLE] = "DISABLE",
> >   };
> > -const char * const mhi_state_str[MHI_STATE_MAX] = {
> > -	[MHI_STATE_RESET] = "RESET",
> > -	[MHI_STATE_READY] = "READY",
> > -	[MHI_STATE_M0] = "M0",
> > -	[MHI_STATE_M1] = "M1",
> > -	[MHI_STATE_M2] = "M2",
> > -	[MHI_STATE_M3] = "M3",
> > -	[MHI_STATE_M3_FAST] = "M3 FAST",
> > -	[MHI_STATE_BHI] = "BHI",
> > -	[MHI_STATE_SYS_ERR] = "SYS ERROR",
> > -};
> > -
> >   const char * const mhi_ch_state_type_str[MHI_CH_STATE_TYPE_MAX] = {
> >   	[MHI_CH_STATE_TYPE_RESET] = "RESET",
> >   	[MHI_CH_STATE_TYPE_STOP] = "STOP",
> > 
> 