Re: [PATCH] Added Wavecom modem plugin.

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Matthias,

> The Wavecom WMP100 assumes CPIN to be the final response, therefore some
>  special handling is neccessary (see
>  6d28f82dc1ccb3de3a028a88eafb96fb001c7e61). Now the vendor id
>  OFONO_VENDOR_WAVECOM is passed with all "*create" functions.

You should also set your authorship information correctly.  E.g. first + last 
name.

> +	options = g_hash_table_new_full(g_str_hash, g_str_equal,
> +					g_free, g_free);
> +	if (!options)
> +		return -ENOMEM;
> +
> +	for (i = 0; tty_opts[i]; i++) {
> +		value = ofono_modem_get_string(modem, tty_opts[i]);
> +
> +		if (value == NULL)
> +			continue;
> +
> +		g_hash_table_insert(options, g_strdup(tty_opts[i]),
> +					g_strdup(value));
> +	}
> +
> +	channel = g_at_tty_open(device, options);

I suggest hardcoding these options (see calypso.c) unless you really want to 
change them from modem.conf for some reason.

> +	value = ofono_modem_get_string(modem, "GsmSyntax");
> +	if (value) {
> +		if (g_str_equal(value, "V1"))
> +			syntax = g_at_syntax_new_gsmv1();
> +		else if (g_str_equal(value, "Permissive"))
> +			syntax = g_at_syntax_new_gsm_permissive();
> +		else
> +			return -EINVAL;
> +	} else {
> +		syntax = g_at_syntax_new_gsmv1();
> +	}

Same here, you should know whether the wavecom is 27.007 compliant or not.

> +	ofono_devinfo_create(modem, OFONO_VENDOR_WAVECOM, "atmodem", chat);
> +	ofono_sim_create(modem, OFONO_VENDOR_WAVECOM, "atmodem", chat);
> +	ofono_voicecall_create(modem, OFONO_VENDOR_WAVECOM, "atmodem", chat);

You really don't want to do that, only pass it to the place where it is 
relevant (e.g. the SIM atom.) That way we know what quirks the modem actually 
has instead of looking at all drivers just to find it has completely stock 
behavior.

Regards,
-Denis