| Message ID | 20211202113553.238011-4-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Add initial support for MHI endpoint stack | expand |
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", > > >
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]) 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",
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> --- drivers/bus/mhi/common.h | 13 ++++++++++++- drivers/bus/mhi/host/init.c | 12 ------------ 2 files changed, 12 insertions(+), 13 deletions(-)