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@xxxxxxxxxx>
>
> 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",
> >
>
