Re: [PATCH 3/6] plugins: new driver for u-blox SARA-U270 modems

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Philip,

> +static void ublox_remove(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	ofono_modem_set_data(modem, NULL);
> +	g_at_chat_unref(data->aux);

Any reason data->modem is not being unrefed? e.g in the case of hot-unplug?

> +	g_free(data);
> +}
> +
> +static GAtChat *open_device(struct ofono_modem *modem,
> +				const char *key, char *debug)
> +{
> +	const char *device;
> +	GAtSyntax *syntax;
> +	GIOChannel *channel;
> +	GAtChat *chat;
> +
> +	device = ofono_modem_get_string(modem, key);
> +	if (device == NULL)
> +		return NULL;
> +
> +	DBG("%s %s", key, device);
> +
> +	channel = g_at_tty_open(device, NULL);
> +	if (channel == NULL)
> +		return NULL;
> +
> +	syntax = g_at_syntax_new_gsm_permissive();
> +	chat = g_at_chat_new(channel, syntax);
> +	g_at_syntax_unref(syntax);
> +
> +	g_io_channel_unref(channel);
> +
> +	if (chat == NULL)
> +		return NULL;
> +
> +	if (getenv("OFONO_AT_DEBUG"))
> +		g_at_chat_set_debug(chat, ublox_debug, debug);
> +
> +	return chat;
> +}
> +
> +

Please remove one whitespace.  Our coding style likes to use a single
empty line to separate functions.

> +static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct ublox_data * data = ofono_modem_get_data(modem);
> +
> +	DBG("ok %d", (int)ok);

No need to cast

> +
> +	if (!ok) {
> +		g_at_chat_unref(data->aux);
> +		data->aux = NULL;
> +		g_at_chat_unref(data->modem);
> +		data->modem = NULL;
> +	}
> +
> +	ofono_modem_set_powered(modem, TRUE);
> +
> +	g_at_chat_send(data->modem, "AT&C0", NULL, NULL, NULL, NULL);
> +	g_at_chat_send(data->aux, "AT&C0", NULL, NULL, NULL, NULL);

Might want to use prefixes here

> +}
> +
> +static int ublox_enable(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	data->modem = open_device(modem, "Modem", "Modem: ");
> +	if (data->modem == NULL)
> +		return -EINVAL;
> +
> +	data->aux = open_device(modem, "Aux", "Aux: ");
> +	if (data->aux == NULL) {
> +		g_at_chat_unref(data->modem);
> +		data->modem = NULL;
> +		return -EIO;
> +	}
> +	g_at_chat_set_slave(data->modem, data->aux);
> +
> +	g_at_chat_send(data->modem, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
> +	g_at_chat_send(data->aux, "ATE0 +CMEE=1", NULL, NULL, NULL, NULL);
> +
> +	g_at_chat_send(data->aux, "AT+CFUN=1", NULL, cfun_enable,
> +					modem, NULL);

Prefix should probably be used.  Also, should this be CFUN=4 and not 1?
 enable() callback should be putting the modem in the SIM powered, RX/TX
off mode (e.g. Offline).

> +
> +	return -EINPROGRESS;
> +}
> +
> +static void cfun_disable(gboolean ok, GAtResult *result, gpointer user_data)
> +{
> +	struct ofono_modem *modem = user_data;
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("");
> +
> +	g_at_chat_unref(data->aux);
> +	data->aux = NULL;
> +
> +	if (ok)
> +		ofono_modem_set_powered(modem, FALSE);
> +}
> +
> +static int ublox_disable(struct ofono_modem *modem)
> +{
> +	struct ublox_data *data = ofono_modem_get_data(modem);
> +
> +	DBG("%p", modem);
> +
> +	g_at_chat_cancel_all(data->modem);
> +	g_at_chat_unregister_all(data->modem);
> +	g_at_chat_unref(data->modem);
> +	data->modem = NULL;
> +
> +	g_at_chat_cancel_all(data->aux);
> +	g_at_chat_unregister_all(data->aux);
> +
> +	g_at_chat_send(data->aux, "AT+CFUN=0", NULL,
> +					cfun_disable, modem, NULL);

This modem doesn't jump off the bus on CFUN=0, right?

> +
> +	return -EINPROGRESS;
> +}
> +

<snip>

Regards,
-Denis