Re: [PATCH_v6 13/14] cdmamodem: add data strength to CDMA netreg atom.

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2766 bytes --]

Hi Bertrand,

On 08/05/2011 08:21 AM, Bertrand Aygon wrote:
> ---
>  src/cdma-netreg.c |   39 +++++++++++++++++++++++++++++++++++++++
>  1 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/src/cdma-netreg.c b/src/cdma-netreg.c
> index 12ddd23..e57856e 100644
> --- a/src/cdma-netreg.c
> +++ b/src/cdma-netreg.c
> @@ -34,6 +34,7 @@ static GSList *g_drivers;
>  struct ofono_cdma_netreg {
>  	enum cdma_netreg_status status;
>  	int strength;
> +	int data_strength;
>  	const struct ofono_cdma_netreg_driver *driver;
>  	void *driver_data;
>  	struct ofono_atom *atom;
> @@ -82,6 +83,13 @@ static DBusMessage *network_get_properties(DBusConnection *conn,
>  					&strength);
>  	}
>  
> +	if (cdma_netreg->data_strength != -1) {
> +		unsigned char data_strength = cdma_netreg->data_strength;
> +
> +		ofono_dbus_dict_append(&dict, "DataStrength", DBUS_TYPE_BYTE,
> +					&data_strength);
> +	}
> +
>  	dbus_message_iter_close_container(&iter, &dict);
>  
>  	return reply;
> @@ -151,6 +159,36 @@ void ofono_cdma_netreg_strength_notify(struct ofono_cdma_netreg *netreg,
>  	}
>  }
>  
> +void ofono_cdma_netreg_data_strength_notify(struct ofono_cdma_netreg *netreg,
> +					int data_strength)
> +{
> +	if (netreg == NULL)
> +		return;
> +
> +	if (netreg->data_strength == data_strength)
> +		return;
> +
> +	/*
> +	 * Theoretically we can get signal strength even when not registered
> +	 * to any network.  However, what do we do with it in that case?
> +	 */
> +	if (netreg->status == CDMA_NETWORK_REGISTRATION_STATUS_NOT_REGISTERED)
> +		return;
> +
> +	netreg->data_strength = data_strength;
> +
> +	if (data_strength != -1) {
> +		DBusConnection *conn = ofono_dbus_get_connection();
> +		const char *path = __ofono_atom_get_path(netreg->atom);
> +		unsigned char data_strength = netreg->data_strength;
> +
> +		ofono_dbus_signal_property_changed(conn, path,
> +				OFONO_CDMA_NETWORK_REGISTRATION_INTERFACE,
> +				"DataStrength", DBUS_TYPE_BYTE,
> +				&data_strength);
> +	}
> +}
> +

I don't like this, this function is nearly a 100% copy of the
strength_notify function, down to the comment in fact.  Are you sure you
can't refactor the common code into a helper?

>  int ofono_cdma_netreg_driver_register(const struct ofono_cdma_netreg_driver *d)
>  {
>  	DBG("driver: %p, name: %s", d, d->name);
> @@ -216,6 +254,7 @@ struct ofono_cdma_netreg *ofono_cdma_netreg_create(struct ofono_modem *modem,
>  
>  	cdma_netreg->status = CDMA_NETWORK_REGISTRATION_STATUS_NOT_REGISTERED;
>  	cdma_netreg->strength = -1;
> +	cdma_netreg->data_strength = -1;
>  
>  	cdma_netreg->atom = __ofono_modem_add_atom(modem,
>  					OFONO_ATOM_TYPE_CDMA_NETREG,

Regards,
-Denis