From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============0976987595657287259=="
MIME-Version: 1.0
From: Denis Kenzior <denkenz@gmail.com>
Subject: Re: [PATCH v5] udev: add initial telit udev support
Date: Mon, 13 Jun 2011 06:07:50 -0500
Message-ID: <4DF5EF86.1050104@gmail.com>
In-Reply-To: <1308067114-19473-1-git-send-email-Bernhard.Guillon@hale.at>
List-Id: <ofono.ofono.org>
To: ofono@ofono.org

--===============0976987595657287259==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

Hi Bernhard,

On 06/14/2011 10:58 AM, Bernhard.Guillon(a)hale.at wrote:
> From: Bernhard Guillon <Bernhard.Guillon@hale.at>
> =

> ---
>  plugins/ofono.rules |    8 ++++++++
>  plugins/udev.c      |   48 +++++++++++++++++++++++++++++++++++++++++++++=
+++
>  2 files changed, 56 insertions(+), 0 deletions(-)
> =

> diff --git a/plugins/ofono.rules b/plugins/ofono.rules
> index 268b327..7124143 100644
> --- a/plugins/ofono.rules
> +++ b/plugins/ofono.rules
> @@ -344,6 +344,11 @@ ATTRS{idVendor}=3D=3D"12d1", ATTRS{idProduct}=3D=3D"=
1485", ENV{OFONO_IFACE_NUM}=3D=3D"02", E
>  ATTRS{idVendor}=3D=3D"12d1", ATTRS{idProduct}=3D=3D"1486", ENV{OFONO_IFA=
CE_NUM}=3D=3D"00", ENV{OFONO_HUAWEI_TYPE}=3D"Modem"
>  ATTRS{idVendor}=3D=3D"12d1", ATTRS{idProduct}=3D=3D"1486", ENV{OFONO_IFA=
CE_NUM}=3D=3D"02", ENV{OFONO_HUAWEI_TYPE}=3D"Pcui"
>  =

> +#Telit UC864-G
> +ATTRS{idVendor}=3D=3D"1bc7", ATTRS{idProduct}=3D=3D"1004", ENV{OFONO_IFA=
CE_NUM}=3D=3D"00", ENV{OFONO_TELIT_TYPE}=3D"Modem"
> +ATTRS{idVendor}=3D=3D"1bc7", ATTRS{idProduct}=3D=3D"1004", ENV{OFONO_IFA=
CE_NUM}=3D=3D"02", ENV{OFONO_TELIT_TYPE}=3D"GPS"
> +ATTRS{idVendor}=3D=3D"1bc7", ATTRS{idProduct}=3D=3D"1004", ENV{OFONO_IFA=
CE_NUM}=3D=3D"03", ENV{OFONO_TELIT_TYPE}=3D"Data"
> +
>  LABEL=3D"ofono_tty_end"
>  =

>  # ISI/Phonet drivers
> @@ -459,4 +464,7 @@ ATTRS{idVendor}=3D=3D"0421", ATTRS{idProduct}=3D=3D"0=
0b6", ENV{OFONO_DRIVER}=3D"nokiacdma"
>  # Teracom (Linktop/LW27x) 3G Data Card
>  ATTRS{idVendor}=3D=3D"230d", ATTRS{idProduct}=3D=3D"0001", ENV{OFONO_DRI=
VER}=3D"linktop"
>  =

> +# Telit
> +ATTRS{idVendor}=3D=3D"1bc7", ENV{OFONO_DRIVER}=3D"telit"
> +
>  LABEL=3D"ofono_end"
> diff --git a/plugins/udev.c b/plugins/udev.c
> index 0234fc0..380e968 100644
> --- a/plugins/udev.c
> +++ b/plugins/udev.c
> @@ -587,6 +587,52 @@ 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 registered;
> +
> +	DBG("modem %p", modem);
> +
> +	registered =3D ofono_modem_get_integer(modem, "Registered");
> +
> +	if (registered =3D=3D 1)
> +		return;

You don't want to check this at the beginning, since you might receive
the Modem and Data ports, register, but then receive the GPS node.

> +
> +	entry =3D udev_device_get_properties_list_entry(udev_device);
> +	while (entry) {
> +		const char *name =3D udev_list_entry_get_name(entry);
> +		type =3D udev_list_entry_get_value(entry);
> +
> +		if (g_str_equal(name, "OFONO_TELIT_TYPE") !=3D TRUE) {
> +			entry =3D udev_list_entry_get_next(entry);
> +			continue;
> +		}
> +
> +		if (g_str_equal(type, "Modem") =3D=3D TRUE) {
> +			devnode =3D udev_device_get_devnode(udev_device);
> +			ofono_modem_set_string(modem, "Modem", devnode);

instead do if (registered && g_str_equal(...) here

> +		} else if (g_str_equal(type, "GPS") =3D=3D TRUE) {
> +			devnode =3D udev_device_get_devnode(udev_device);
> +			ofono_modem_set_string(modem, "GPS", devnode);
> +		} else if (g_str_equal(type, "Data") =3D=3D TRUE) {

and here

> +			devnode =3D udev_device_get_devnode(udev_device);
> +			ofono_modem_set_string(modem, "Data", devnode);
> +		}
> +
> +		break;
> +	}
> +

And move the if (registered =3D=3D 1) statement from above down to here

> +	if ((ofono_modem_get_string(modem, "Modem") !=3D NULL) &&
> +			(ofono_modem_get_string(modem, "Data") !=3D 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 +727,8 @@ done:
>  		add_calypso(modem, udev_device);
>  	else if (g_strcmp0(driver, "tc65") =3D=3D 0)
>  		add_tc65(modem, udev_device);
> +	else if (g_strcmp0(driver, "telit") =3D=3D 0)
> +		add_telit(modem, udev_device);
>  	else if (g_strcmp0(driver, "nokiacdma") =3D=3D 0)
>  		add_nokiacdma(modem, udev_device);
>          else if (g_strcmp0(driver, "linktop") =3D=3D 0)

Otherwise this looks good.

Regards,
-Denis

--===============0976987595657287259==--