Re: [RFC 1/3] STE-plugin: Add vendor STE

[Marcel Holtmann <marcel@holtmann.org>]

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

Hi Sjur,

> This patch add STE as vendor, and a few adjustments needed in "atmodem" 
> for ST-Ericsson modem.
> 
> ---
>  drivers/atmodem/gprs.c   |   15 +++++++++++++--
>  drivers/atmodem/vendor.h |    5 +++++
>  plugins/modemconf.c      |    5 +++++
>  3 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
> index 76085d9..305f22f 100644
> --- a/drivers/atmodem/gprs.c
> +++ b/drivers/atmodem/gprs.c
> @@ -17,6 +17,10 @@
>   *  along with this program; if not, write to the Free Software
>   *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   *
> + *  Copyright (C) 2010 ST-Ericsson AB.
> + *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.
> + *  STE specific implementation.
> + *
>   */

please don't do that. Add the proper copyright to the header if the
changes you are making are major. We track authorship via the GIT
itself.
 
>  #ifdef HAVE_CONFIG_H
> @@ -38,6 +42,7 @@
>  #include "gatresult.h"
>  
>  #include "atmodem.h"
> +#include "vendor.h"
>  
>  static const char *cgreg_prefix[] = { "+CGREG:", NULL };
>  static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
> @@ -45,6 +50,7 @@ static const char *none_prefix[] = { NULL };
>  
>  struct gprs_data {
>  	GAtChat *chat;
> +	unsigned int vendor;
>  };
>  
>  static void at_cgatt_cb(gboolean ok, GAtResult *result, gpointer user_data)
> @@ -216,7 +222,12 @@ static void at_cgreg_test_cb(gboolean ok, GAtResult *result,
>  
>  	g_at_chat_send(gd->chat, cmd, none_prefix, NULL, NULL, NULL);
>  	g_at_chat_send(gd->chat, "AT+CGAUTO=0", none_prefix, NULL, NULL, NULL);
> -	g_at_chat_send(gd->chat, "AT+CGEREP=2,1", none_prefix,
> +
> +	if (gd->vendor == OFONO_VENDOR_STE)
> +		g_at_chat_send(gd->chat, "AT+CGEREP=1,0", none_prefix,
> +			gprs_initialized, gprs, NULL);
> +	else
> +		g_at_chat_send(gd->chat, "AT+CGEREP=2,1", none_prefix,
>  			gprs_initialized, gprs, NULL);
>  	return;

Can you add some extra comment here explaining why this is needed.
Otherwise it looks like some black magic.
 
> @@ -289,7 +300,7 @@ static int at_gprs_probe(struct ofono_gprs *gprs,
>  
>  	gd = g_new0(struct gprs_data, 1);
>  	gd->chat = chat;
> -
> +	gd->vendor = vendor;
>  	ofono_gprs_set_data(gprs, gd);
>  
>  	g_at_chat_send(chat, "AT+CGDCONT=?", cgdcont_prefix,
> diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
> index 8d9ed47..d7b5210 100644
> --- a/drivers/atmodem/vendor.h
> +++ b/drivers/atmodem/vendor.h
> @@ -17,11 +17,16 @@
>   *  along with this program; if not, write to the Free Software
>   *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   *
> + *  Copyright (C) 2010 ST-Ericsson AB.
> + *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.
> + *  STE specific implementation.
> + *
>   */

To be honest, just adding a new entry in enum is not really a reason to
add a copyright statement here.

>  enum ofono_vendor {
>  	OFONO_VENDOR_GENERIC = 0,
>  	OFONO_VENDOR_CALYPSO,
> +	OFONO_VENDOR_STE,
>  	OFONO_VENDOR_QUALCOMM_MSM,
>  	OFONO_VENDOR_OPTION_HSO,
>  };
> diff --git a/plugins/modemconf.c b/plugins/modemconf.c
> index c8c261f..41c7428 100644
> --- a/plugins/modemconf.c
> +++ b/plugins/modemconf.c
> @@ -17,6 +17,10 @@
>   *  along with this program; if not, write to the Free Software
>   *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
>   *
> + *  Copyright (C) 2010 ST-Ericsson AB.
> + *  Author: Marit Henriksen, marit.xx.henriksen(a)stericsson.com.
> + *  STE specific implementation.
> + *
>   */
>  
>  #ifdef HAVE_CONFIG_H
> @@ -128,6 +132,7 @@ static struct ofono_modem *create_modem(GKeyFile *keyfile, const char *group)
>  		set_address(modem, keyfile, group);
>  
>  	if (!g_strcmp0(driver, "atgen") || !g_strcmp0(driver, "g1") ||
> +						!g_strcmp0(driver, "ste") ||
>  						!g_strcmp0(driver, "calypso") ||
>  						!g_strcmp0(driver, "hfp") ||
>  						!g_strcmp0(driver, "palmpre"))

I am failing to see the reason for this. I makes no sense to me. The
CAIF kernel code (as far as I understand it) exports AT command sockets
and they are configured differently. So this seems like a change without
any functionality.

Regards

Marcel