Re: [PATCH v2 0/9] ofono/wwan modem support

[Tony Espy <espy canonical com>]

On 06/17/2016 07:47 AM, Thomas Haller wrote:
> On Mon, 2016-06-13 at 13:52 -0500, Dan Williams wrote:
> > On Mon, 2016-06-13 at 18:51 +0200, Thomas Haller wrote:
> > > Here: https://cgit.freedesktop.org/NetworkManager/NetworkManager/lo
> > > g/
> > > ?h=th/review/ofono
> > Cleanups look good so far, though we need to pkg-config check for
> > libsystemd-daemon instead of just HAVE_SYSTEMD.
> >
> > Also we should rename NM_LINK_TYPE_WWAN_ETHERNET
> > to NM_LINK_TYPE_WWAN_NET and get the platform to recognize rmnet and
> > ccmni devices as this type.  Then the ignore stuff should work
> > correctly.  Ideally we can do this on the driver name instead of
> > matching the device name, but I'm not sure.  Neither of these drivers
> > are upstream yet, so we can't get them to do SET_NETDEV_DEVTYPE().
>
> these two issues are fixed and merged to master.

Thanks Thomas.

> for now I only looked at patch 3/9, which is on th/review/ofono.
> I think while there are issues, we could merge that branch and
> fix them afterwards. Otherwise, this off-line improving of this large
> patchset is cumbersome.
>
> ACK to merge th/review/ofono ?

I just reviewed your branch yesterday, and it looks good to me, so yes 
ACK to the changes.

Also, sorry for mangled formatting/indentation. There doesn't seem to 
have been much consistency in the Ubuntu patches in the past.

Is there a formal style-guide for NetworkManager?  Any suggestions as 
for the best emacs mode for NM code?

> Note that by default almost none of the code is actually compiled.

I just applied most of the functional changes from your branch locally, 
and rebuilt, so yes, it compiles.  ;)-

> Only question I have is about the changes to modem_manager_poke_cb()

You mean the re-working of the error logic in that function?

I re-worked Mathieu's original logic as we found that on Touch devices, 
modem_manager_poke_cb () was called on a periodic basis even though 
modem-manager isn't installed on these devices.  So I changed it to no 
longer do this if the error returned indicates that the service isn't 
available.

/tony