Hi Rémi,

2011/1/28 Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
>
> It looks like you are mixing up services (PN_* from 0 to 255), with device
> addresses (PN_DEV_* multiple of four from 0 to 252).

Yes, you are right. We will change it.



>
> > @@ -61,6 +61,7 @@ struct _GIsiModem {
> >         unsigned index;
> >         GHashTable *services;
> >         gboolean subs_source;
> > +       GIsiVersion version;
>
> I can't see where this gets set?!

In g_isi_modem_set_version() in plugins/u8500.c
New file, that will hopefully be sent in a patch to the mailing list today.



>
> > +
> > +       if (g_isi_modem_version_major(modem) == 2 &&
> > +               g_isi_modem_version_minor(modem) == 5) {
>
> You really need to factor this check out. It's going to be a disaster when
> there are more than two versions otherwise.

Well, we really need the PNS_SUBSCRIBED_RESOURCES_EXTEND_IND because
the other one doesn't work for us.

Would a lookup function do the trick, with filling in the necessary
values in msg depending on which version it is?
If that is not satisfactory, could you please be a bit more specific
in what you would like to see so we don't misunderstand?
>
> >
> > @@ -363,13 +382,31 @@ static gboolean modem_subs_update(gpointer data)
>
>
> That should probably be a separate patch.

Yes, you are right.
>
> > diff --git a/gisi/pipe.c b/gisi/pipe.c
>
>
> We already have this in kernel and I wonder why this needs to be duplicated in
> userspace now?! Furthermore pipe.c is about pipes. Pipe endpoints code belongs
> in pep.c. If you want to implement a pipe controller in gisi, then I think it
> really deserves a new file of its own.
>

isimodem2.5 needs this code, the kernel struct won't be applicable for
us in the modem case.

We will move the pep functions to the pep.c

Best Regards,
Jessica Nilsson
ST-Ericsson