diff mbox

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

Message ID 1394845987-21729-1-git-send-email-anayuso@sysmocom.de
State Accepted
Headers show

Commit Message

Alvaro Neira March 15, 2014, 1:13 a.m. UTC 
From: Álvaro Neira Ayuso <anayuso@sysmocom.de>

I have extended the principal function that we use for requesting
information to the microcontroller for switching off/on the board
and the PA. And I have extended it for requesting the power status
information of the board and the PA.

Signed-off-by: Alvaro Neira Ayuso <anayuso@sysmocom.de>
---
v6: Added the BUILD_SBTS2050 for fixing the error when
we don't have the header.

 src/osmo-bts-sysmo/misc/sysmobts_misc.c |   93 +++++++++++++++++++++++++++++++
 src/osmo-bts-sysmo/misc/sysmobts_misc.h |   10 ++++
 2 files changed, 103 insertions(+)

Comments

Peter Stuge March 15, 2014, 1:45 a.m. UTC | #1 
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
Alvaro Neira March 15, 2014, 2:10 a.m. UTC | #2 
Hello Peter

El 15/03/14 02:45, Peter Stuge escribió:
> 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.

I have followed the kernel coding style, and he said doesn't speak about 
use sizeof without bracket.

>
>
>> @@ -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.

Yes, it's true, I don't know why I have done that...

>
>
>> +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?

I have used DTEMP, because i'm going to have a relation between the 
temperature and this function but it's true that somebody can use it in 
other cases. I don't know what do you think about that, if somebody can 
help me.

>
>
>> +++ 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
>

Thanks a lot Peter
Peter Stuge March 15, 2014, 7:56 p.m. UTC | #3 
Álvaro Neira Ayuso wrote:
>>> +	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.
>
> I have followed the kernel coding style, and he said doesn't speak about 
> use sizeof without bracket.

All right!


>>> +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?
>
> I have used DTEMP, because i'm going to have a relation between the 
> temperature and this function but it's true that somebody can use it in 
> other cases. I don't know what do you think about that, if somebody can 
> help me.

I also don't know enough to make a suggestion here - it depends on
how versatile these new sbts2050_uc_* functions should be.

Maybe they are only intended to be called from the TEMP subsystem,
then the current patch is fine. Hopefully someone with more overview
can comment on this?


//Peter
diff mbox

Patch

diff --git a/src/osmo-bts-sysmo/misc/sysmobts_misc.c b/src/osmo-bts-sysmo/misc/sysmobts_misc.c
index 55a7a2a..9ea26c2 100644
--- a/src/osmo-bts-sysmo/misc/sysmobts_misc.c
+++ 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;
+	case SBTS2050_PWR_STATUS:
+		num = sizeof(command->cmd.pwrGetStatus);
+		break;
 	default:
 		return NULL;
 	}
@@ -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);
+		break;
 	default:
 		goto err;
 	}
@@ -182,6 +199,82 @@  err:
 #endif
 
 /**********************************************************************
+ *	Get power status function
+ *********************************************************************/
+int sbts2050_uc_status(struct uc *ucontrol, enum sbts2050_status_rqt status)
+{
+#ifdef BUILD_SBTS2050
+	struct msgb *msg;
+	struct ucinfo info = {
+		.id = SBTS2050_PWR_STATUS,
+	};
+	rsppkt_t *response;
+	int val_status;
+
+	msg = sbts2050_ucinfo_get(ucontrol, info);
+
+	if (msg == NULL) {
+		LOGP(DTEMP, LOGL_ERROR, "Error switching off some unit");
+		return -1;
+	}
+
+	response = (rsppkt_t *)msg->data;
+
+	switch (status) {
+	case SBTS2050_STATUS_MASTER:
+		val_status = response->rsp.pwrGetStatus.u1MasterEn;
+		break;
+	case SBTS2050_STATUS_SLAVE:
+		val_status = response->rsp.pwrGetStatus.u1SlaveEn;
+		break;
+	case SBTS2050_STATUS_PA:
+		val_status = response->rsp.pwrGetStatus.u1PwrAmpEn;
+		break;
+	default:
+		msgb_free(msg);
+		return -1;
+	}
+	msgb_free(msg);
+	return val_status;
+#else
+	return -1;
+#endif
+}
+
+/**********************************************************************
+ *	Uc Power Switching handling
+ *********************************************************************/
+void sbts2050_uc_power(struct uc *ucontrol, int pmaster, int pslave, int ppa)
+{
+#ifdef BUILD_SBTS2050
+	struct msgb *msg;
+	struct ucinfo info = {
+		.id = 0x00,
+		.master = pmaster,
+		.slave = pslave,
+		.pa = ppa
+	};
+
+	msg = sbts2050_ucinfo_get(ucontrol, info);
+
+	if (msg == NULL) {
+		LOGP(DTEMP, LOGL_ERROR, "Error switching off some unit");
+		return;
+	}
+
+	LOGP(DTEMP, LOGL_DEBUG, "Switch off/on success:\n"
+				"MASTER %s\n"
+				"SLAVE %s\n"
+				"PA %s\n",
+				pmaster ? "ON" : "OFF",
+				pslave ? "ON" : "OFF",
+				ppa ? "ON" : "OFF");
+
+	msgb_free(msg);
+#endif
+}
+
+/**********************************************************************
  *	Uc temperature handling
  *********************************************************************/
 void sbts2050_uc_check_temp(struct uc *ucontrol, int *temp_pa, int *temp_board)
diff --git a/src/osmo-bts-sysmo/misc/sysmobts_misc.h b/src/osmo-bts-sysmo/misc/sysmobts_misc.h
index 3c6513e..01878f2 100644
--- a/src/osmo-bts-sysmo/misc/sysmobts_misc.h
+++ 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
+};
+
 struct uc {
 	int id;
 	int fd;
@@ -33,6 +39,10 @@  void sysmobts_check_temp(int no_eeprom_write);
 
 void sbts2050_uc_check_temp(struct uc *ucontrol, int *temp_pa, int *temp_board);
 
+void sbts2050_uc_power(struct uc *ucontrol, int pmaster, int pslave, int ppa);
+
+int sbts2050_uc_status(struct uc *ucontrol, enum sbts2050_status_rqt status);
+
 int sysmobts_update_hours(int no_epprom_write);
 
 enum sysmobts_firmware_type {