Re: [PATCH 2/3] atmodem: Add gprs-context quirk for SIM900

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Jesper,

On 07/17/2013 02:14 AM, Jesper Larsen wrote:
> The SIM900 module from SIMCOM does have a AT+CGDATA command.
> However, it is not possible to make a ppp connection when CGDATA
> has been used to bring up the gprs context.
>
> This patch ads a quirk that uses the alternative ATD*99***<cid>#
> command instead.
> ---
>   drivers/atmodem/gprs-context.c |    8 ++++++--
>   drivers/atmodem/vendor.h       |    1 +

I'd like the vendor.h change in a separate commit

>   plugins/sim900.c               |    2 +-

The changes to files in different directories should be separated 
whenever possible.  See 'HACKING' document in the top level directory of 
the source tree, particularly the 'Submitting Patches' section.

So your commits should be:
vendor.h change
gprs-context.c change
sim900.c change

>   3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/atmodem/gprs-context.c b/drivers/atmodem/gprs-context.c
> index 3694c27..f46d881 100644
> --- a/drivers/atmodem/gprs-context.c
> +++ b/drivers/atmodem/gprs-context.c
> @@ -208,8 +208,12 @@ static void at_cgdcont_cb(gboolean ok, GAtResult *result, gpointer user_data)
>   		return;
>   	}
>
> -	sprintf(buf, "AT+CGDATA=\"PPP\",%u", gcd->active_context);
> -	if (g_at_chat_send(gcd->chat, buf, none_prefix,
> +        if (gcd->vendor == OFONO_VENDOR_SIMCOM_SIM900)
> +		sprintf(buf, "ATD*99***%u#", gcd->active_context);
> +	else
> +		sprintf(buf, "AT+CGDATA=\"PPP\",%u", gcd->active_context);
> +
> +        if (g_at_chat_send(gcd->chat, buf, none_prefix,

You're mixing tabs and spaces for indentation all over here.  We only 
use tabs.

>   				at_cgdata_cb, gc, NULL) > 0)
>   		return;
>
> diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
> index 0532d10..2b1a567 100644
> --- a/drivers/atmodem/vendor.h
> +++ b/drivers/atmodem/vendor.h
> @@ -39,6 +39,7 @@ enum ofono_vendor {
>   	OFONO_VENDOR_SPEEDUP,
>   	OFONO_VENDOR_SAMSUNG,
>   	OFONO_VENDOR_SIMCOM,
> +        OFONO_VENDOR_SIMCOM_SIM900,

Again, tabs for indentation only please

>   	OFONO_VENDOR_ICERA,
>   	OFONO_VENDOR_WAVECOM_Q2XXX,
>   	OFONO_VENDOR_ALCATEL
> diff --git a/plugins/sim900.c b/plugins/sim900.c
> index 643de51..3b4fd84 100644
> --- a/plugins/sim900.c
> +++ b/plugins/sim900.c
> @@ -361,7 +361,7 @@ static void sim900_post_sim(struct ofono_modem *modem)
>   	if (gprs == NULL)
>   		return;
>
> -	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM,
> +	gc = ofono_gprs_context_create(modem, OFONO_VENDOR_SIMCOM_SIM900,
>   					"atmodem", data->dlcs[GPRS_DLC]);
>   	if (gc)
>   		ofono_gprs_add_context(gprs, gc);
>

Regards,
-Denis