Re: [PATCH v4 4/4] udev: add Telit UC864-G and update udev rules

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Bernhard,

On 06/08/2011 10:18 AM, Bernhard.Guillon(a)hale.at wrote:
> From: Bernhard Guillon <Bernhard.Guillon@hale.at>
> 
> *add Modem and GPS check for telit
> *make GPS yes/no configurable with udev rules settings
> **this is necessary because telit has different interface
>   numbers on different modems e.g. GPS and UART
> ---
>  plugins/ofono.rules |    8 +++++
>  plugins/udev.c      |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 80 insertions(+), 0 deletions(-)
> 
> diff --git a/plugins/ofono.rules b/plugins/ofono.rules
> index 268b327..64fa4b2 100644
> --- a/plugins/ofono.rules
> +++ b/plugins/ofono.rules
> @@ -344,6 +344,11 @@ ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1485", ENV{OFONO_IFACE_NUM}=="02", E
>  ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1486", ENV{OFONO_IFACE_NUM}=="00", ENV{OFONO_HUAWEI_TYPE}="Modem"
>  ATTRS{idVendor}=="12d1", ATTRS{idProduct}=="1486", ENV{OFONO_IFACE_NUM}=="02", ENV{OFONO_HUAWEI_TYPE}="Pcui"
>  
> +#Telit UC864-G
> +ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_TELIT_GPS}="1"
> +ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="00", ENV{OFONO_TELIT_TYPE}="Modem"
> +ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004", ENV{OFONO_IFACE_NUM}=="02", ENV{OFONO_TELIT_TYPE}="GPS"
> +

What I would do here is something like:
ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004",
ENV{OFONO_IFACE_NUM}=="00", ENV{OFONO_TELIT_TYPE}="Modem"

ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004",
ENV{OFONO_IFACE_NUM}=="02", ENV{OFONO_TELIT_TYPE}="GPS"

ATTRS{idVendor}=="1bc7", ATTRS{idProduct}=="1004",
ENV{OFONO_IFACE_NUM}=="03", ENV{OFONO_TELIT_TYPE}="Data"

>  LABEL="ofono_tty_end"
>  
>  # ISI/Phonet drivers
> @@ -459,4 +464,7 @@ ATTRS{idVendor}=="0421", ATTRS{idProduct}=="00b6", ENV{OFONO_DRIVER}="nokiacdma"
>  # Teracom (Linktop/LW27x) 3G Data Card
>  ATTRS{idVendor}=="230d", ATTRS{idProduct}=="0001", ENV{OFONO_DRIVER}="linktop"
>  
> +# Telit
> +ATTRS{idVendor}=="1bc7", ENV{OFONO_DRIVER}="telit"
> +
>  LABEL="ofono_end"
> diff --git a/plugins/udev.c b/plugins/udev.c
> index 0234fc0..e56d3db 100644
> --- a/plugins/udev.c
> +++ b/plugins/udev.c
> @@ -587,6 +587,76 @@ static void add_linktop(struct ofono_modem *modem,
>  	}
>  }
>  
> +static void add_telit(struct ofono_modem *modem,
> +					struct udev_device *udev_device)
> +{
> +	struct udev_list_entry *entry;
> +	const char *devnode, *type;
> +	int device, gps_device, has_gps;
> +
> +	DBG("modem %p", modem);
> +
> +	/*
> +	 * Some Telit modems have a built in GPS device. According to the
> +	 * manual the the interface numbers can differ between different
> +	 * devices hence we need to use udev rules to set the environment
> +	 * accordingly.
> +	 */
> +	device = ofono_modem_get_integer(modem, "ModemRegistered");
> +	gps_device = ofono_modem_get_integer(modem, "GPSRegistered");
> +	has_gps = ofono_modem_get_integer(modem, "HasGPS");
> +
> +	if ((!has_gps && device) || (device && gps_device))
> +		return;

You can then skip the HasGPS variable.  I would then model the detection
logic after add mbm. e.g. something like:

registered = ofono_modem_get_integer(modem, "Registered");

> +
> +	entry = udev_device_get_properties_list_entry(udev_device);
> +	while (entry) {
> +		const char *name = udev_list_entry_get_name(entry);
> +		type = udev_list_entry_get_value(entry);
> +
> +		if (g_str_equal(name, "OFONO_TELIT_GPS") == TRUE) {
> +			int value = g_str_equal(type, "1");
> +
> +			ofono_modem_set_integer(modem, "HasGPS", value);
> +			entry = udev_list_entry_get_next(entry);
> +			continue;
> +		}

drop this if statement

> +
> +		if (g_str_equal(name, "OFONO_TELIT_TYPE") != TRUE) {
> +			entry = udev_list_entry_get_next(entry);
> +			continue;
> +		}
> +
> +		if (g_str_equal(type, "Modem") == TRUE) {

if (registered == 0 && g_str_equal(type, "Modem") == TRUE) {
	...

> +

We prefer nested if statements like this to be written like:

if (expression) {
	if ()
		...
}

e.g. no spaces before the nested if.

> +			if (device != 0)
> +				return;
> +
> +			devnode = udev_device_get_devnode(udev_device);
> +			ofono_modem_set_string(modem, "Modem", devnode);
> +			device = 1;
> +			ofono_modem_set_integer(modem, "ModemRegistered",
> +							device);

Drop the device=1 and ofono_modem_set_integer calls..

> +		} else if (g_str_equal(type, "GPS") == TRUE) {
> +
> +			if (gps_device != 0)
> +				return;
> +
> +			devnode = udev_device_get_devnode(udev_device);
> +			ofono_modem_set_string(modem, "GPS", devnode);
> +
> +			gps_device = 1;
> +			ofono_modem_set_integer(modem, "GPSRegistered",
> +							gps_device);
> +		}

Same comments as above for this statement

Then add another if statement detecting the Data port.

> +
> +		break;
> +	}
> +
> +	if ((!has_gps && device) || (device && gps_device))
> +		ofono_modem_register(modem);

Then here do something like:

if (registered == 1)
	return;

if (ofono_modem_get_string(modem, "Modem") != NULL &&
		ofono_modem_get_string(modem, "Data") != NULL) {
	ofono_modem_set_integer(modem, "Registered", 1");
	ofono_modem_register(modem);
}

> +}
> +
>  static void add_modem(struct udev_device *udev_device)
>  {
>  	struct ofono_modem *modem;
> @@ -681,6 +751,8 @@ done:
>  		add_calypso(modem, udev_device);
>  	else if (g_strcmp0(driver, "tc65") == 0)
>  		add_tc65(modem, udev_device);
> +	else if (g_strcmp0(driver, "telit") == 0)
> +		add_telit(modem, udev_device);
>  	else if (g_strcmp0(driver, "nokiacdma") == 0)
>  		add_nokiacdma(modem, udev_device);
>          else if (g_strcmp0(driver, "linktop") == 0)

Regards,
-Denis