[osmo-bts PATCH v6] misc/sysmobts_misc: function for switching off/on and requesting status power

[Peter Stuge]

Alvaro Neira Ayuso wrote:
> +++ b/src/osmo-bts-sysmo/misc/sysmobts_misc.c
> @@ -120,6 +120,12 @@ static struct msgb *sbts2050_ucinfo_get(struct uc *ucontrol, struct ucinfo info)
>  	case SBTS2050_TEMP_RQT:
>  		num = sizeof(command->cmd.tempGet);
>  		break;
> +	case SBTS2050_PWR_RQT:
> +		num = sizeof(command->cmd.pwrSetState);
> +		break;

A small style remark is that I like to use: sizeof command->cmd.pwerSetState
since sizeof isn't really a function. Similar to return.


> @@ -138,6 +144,17 @@ static struct msgb *sbts2050_ucinfo_get(struct uc *ucontrol, struct ucinfo info)
>  		command->u8Id = info.id;
>  		command->u8Len = sizeof(command->cmd.tempGet);
>  		break;
> +	case SBTS2050_PWR_RQT:
> +		command->u8Id = info.id;
> +		command->u8Len = sizeof(command->cmd.pwrSetState);
> +		command->cmd.pwrSetState.u1MasterEn = !!info.master;
> +		command->cmd.pwrSetState.u1SlaveEn  = !!info.slave;
> +		command->cmd.pwrSetState.u1PwrAmpEn = !!info.pa;
> +		break;
> +	case SBTS2050_PWR_STATUS:
> +		command->u8Id     = info.id;
> +		command->u8Len    = sizeof(command->cmd.pwrGetStatus);

The above lines have a couple of extra spaces before the = which may
or may not be worth fixing.


> +int sbts2050_uc_status(struct uc *ucontrol, enum sbts2050_status_rqt status)
..
> +	msg = sbts2050_ucinfo_get(ucontrol, info);
> +
> +	if (msg == NULL) {
> +		LOGP(DTEMP, LOGL_ERROR, "Error switching off some unit");

This error message doesn't contain very much information. It might be
nice to know both which unit could not be switched off *and* what the
error was.

Also, is DTEMP guaranteed to always be the appropriate subsystem for
this function? Maybe yes, but this code is in sysmobts_misc.c which
suggests  that it may be something more general?


> +++ b/src/osmo-bts-sysmo/misc/sysmobts_misc.h
> @@ -13,6 +13,12 @@ enum sysmobts_temp_type {
>  	_NUM_TEMP_TYPES
>  };
>  
> +enum sbts2050_status_rqt {
> +	SBTS2050_STATUS_MASTER,
> +	SBTS2050_STATUS_SLAVE,
> +	SBTS2050_STATUS_PA

Maybe add a , after the last enum to make it easier to add more
values in the future.


//Peter