Re: [PATCH 1/3] Support Telit cdc acm device

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Mingli,

On 12/17/2012 10:22 AM, Mingli Wu wrote:
> This patch is for Telit HE910 modem. Configure modem and aux ports
> of ACM devices. some code of this patch was found on the Internet.
> ---
>   include/modem.h  |    3 +++
>   plugins/telit.c  |   24 +++++++++++++++++++++++-
>   plugins/udevng.c |   30 +++++++++++++++++++++++++++++-
>   src/modem.c      |   11 +++++++++++
>   4 files changed, 66 insertions(+), 2 deletions(-)

You need to break up these patches, see our patch submission guidelines 
in HACKING document, 'Submitting Patches' section.  Also please review 
the coding style guidelines in doc/coding-style.txt.

>
> diff --git a/include/modem.h b/include/modem.h
> index 6e08275..303bc24 100644
> --- a/include/modem.h
> +++ b/include/modem.h
> @@ -112,6 +112,9 @@ int ofono_modem_set_boolean(struct ofono_modem *modem,
>   ofono_bool_t ofono_modem_get_boolean(struct ofono_modem *modem,
>   					const char *key);
>
> +void ofono_modem_set_modem_info(struct ofono_modem *omodem, void *modem_info);
> +void *ofono_modem_get_modem_info(struct ofono_modem *omodem);
> +

This is not required, ofono_modem_set_string, set_integer, set_boolean 
are already enough.

>   int ofono_modem_driver_register(const struct ofono_modem_driver *);
>   void ofono_modem_driver_unregister(const struct ofono_modem_driver *);
>
> diff --git a/plugins/telit.c b/plugins/telit.c
> index a0f7deb..aaf2a9d 100644
> --- a/plugins/telit.c
> +++ b/plugins/telit.c
> @@ -180,6 +180,7 @@ static gboolean hw_event_cb(GIOChannel *hw_io, GIOCondition condition,
>   	return FALSE;
>   }
>
> +extern gboolean udevng_match_modem_id(struct ofono_modem *modem, char *vendorid, char *productid);
>   static GAtChat *open_device(struct ofono_modem *modem,
>   				const char *key, char *debug)
>   {
> @@ -187,6 +188,7 @@ static GAtChat *open_device(struct ofono_modem *modem,
>   	GAtSyntax *syntax;
>   	GIOChannel *channel;
>   	GAtChat *chat;
> +	GHashTable *options = NULL;
>
>   	device = ofono_modem_get_string(modem, key);
>   	if (device == NULL)
> @@ -194,7 +196,27 @@ static GAtChat *open_device(struct ofono_modem *modem,
>
>   	DBG("%s %s", key, device);
>
> -	channel = g_at_tty_open(device, NULL);
> +	/* This is for Telit HE910. */
> +	if(udevng_match_modem_id(modem, "1bc7", "0021")){
> +		options = g_hash_table_new(g_str_hash, g_str_equal);
> +
> +		if (options == NULL)
> +			return NULL;
> +
> +		g_hash_table_insert(options, "Baud", "115200");
> +		g_hash_table_insert(options, "Parity", "none");
> +		g_hash_table_insert(options, "StopBits", "1");
> +		g_hash_table_insert(options, "DataBits", "8");
> +		g_hash_table_insert(options, "XonXoff", "off");
> +		g_hash_table_insert(options, "Local", "on");
> +		g_hash_table_insert(options, "RtsCts", "on");
> +	}
> +
> +	channel = g_at_tty_open(device, options);
> +	if(options){
> +		g_hash_table_destroy(options);
> +	}
> +

This was already taken care of by the patch from Christopher that was 
applied today.

>   	if (channel == NULL)
>   		return NULL;
>
> diff --git a/plugins/udevng.c b/plugins/udevng.c
> index afb02ca..b7cdef6 100644
> --- a/plugins/udevng.c
> +++ b/plugins/udevng.c
> @@ -57,6 +57,20 @@ struct device_info {
>   	char *sysattr;
>   };
>
> +
> +gboolean udevng_match_modem_id(struct ofono_modem *modem, char *vendorid, char *productid)
> +{
> +	struct modem_info *modem_info = ofono_modem_get_modem_info(modem);
> +
> +	if(!modem_info){
> +		return FALSE;
> +	}
> +	if (g_strcmp0(modem_info->vendor, vendorid) || g_strcmp0(modem_info->model, productid)){
> +		return FALSE;
> +	}
> +	return TRUE;
> +}
> +
>   static gboolean setup_isi(struct modem_info *modem)
>   {
>   	const char *node = NULL;
> @@ -620,6 +634,17 @@ static gboolean setup_telit(struct modem_info *modem)
>   			else if (g_strcmp0(info->number, "03") == 0)
>   				aux = info->devnode;
>   		}
> +		/* This for Telit HE910 */
> +		else if (!g_strcmp0(modem->vendor, "1bc7")&&  !g_strcmp0(modem->model, "0021")){
> +			if (g_strcmp0(info->interface, "2/2/1") == 0) {
> +				if (g_strcmp0(info->number, "00") == 0)
> +					mdm = info->devnode;
> +				else if (g_strcmp0(info->number, "02") == 0)
> +					diag = info->devnode;
> +				else if (g_strcmp0(info->number, "06") == 0)
> +					aux = info->devnode;
> +			}
> +		}
>   	}
>
>   	if (aux == NULL || mdm == NULL)
> @@ -629,7 +654,8 @@ static gboolean setup_telit(struct modem_info *modem)
>
>   	ofono_modem_set_string(modem->modem, "Modem", mdm);
>   	ofono_modem_set_string(modem->modem, "Aux", aux);
> -	ofono_modem_set_string(modem->modem, "GPS", gps);
> +	//ofono_modem_set_string(modem->modem, "GPS", gps);
> +	ofono_modem_set_string(modem->modem, "Diag", diag);

Please do not leave dead code in your patch submissions.

>
>   	return TRUE;
>   }
> @@ -988,6 +1014,7 @@ static struct {
>   	{ "simcom",	"option",	"05c6", "9000"	},
>   	{ "telit",	"usbserial",	"1bc7"		},
>   	{ "telit",	"option",	"1bc7"		},
> +	{ "telit",      "cdc_acm",      "1bc7", "0021"  },
>   	{ "nokia",	"option",	"0421", "060e"	},
>   	{ "nokia",	"option",	"0421", "0623"	},
>   	{ "samsung",	"option",	"04e8", "6889"	},
> @@ -1113,6 +1140,7 @@ static gboolean create_modem(gpointer key, gpointer value, gpointer user_data)
>   	if (modem->modem == NULL)
>   		return TRUE;
>
> +	ofono_modem_set_modem_info(modem->modem, modem);
>   	for (i = 0; driver_list[i].name; i++) {
>   		if (g_str_equal(driver_list[i].name, modem->driver) == FALSE)
>   			continue;
> diff --git a/src/modem.c b/src/modem.c
> index 3c7c80a..207dce8 100644
> --- a/src/modem.c
> +++ b/src/modem.c
> @@ -87,6 +87,7 @@ struct ofono_modem {
>   	void			*driver_data;
>   	char			*driver_type;
>   	char			*name;
> +	void                    *modem_info;
>   };
>
>   struct ofono_devinfo {
> @@ -119,6 +120,16 @@ struct modem_property {
>   	void *value;
>   };
>
> +void ofono_modem_set_modem_info(struct ofono_modem *omodem, void *modem_info)
> +{
> +	omodem->modem_info = modem_info;
> +}
> +
> +void *ofono_modem_get_modem_info(struct ofono_modem *omodem)
> +{
> +	return omodem->modem_info;
> +}
> +
>   static const char *modem_type_to_string(enum ofono_modem_type type)
>   {
>   	switch (type) {

Regards,
-Denis