[Patch] MTK plugin for MediaTek devices

[Dan Williams]

On Mon, 2013-09-09 at 21:07 +0800, 李晋 wrote:
> Hi,
> I am working for MediaTek and finished the first version of MTK
> plug-in for Modem-Manager.
> I have tested it on the latest Linux distro like Ubuntu 13.04, Fedora
> 18 and Debian 7.0 with MediaTek MT6280 and MT6290 devices.
> The patch and debug log (use mmcli to control modem) are in the
> attachments. I hope this plugin will be accept as part of MM, and I
> will update it if there is any issue or MediaTek device change.

Thanks!  A couple comments:

1) Style issues; we try to keep a uniform code style in ModemManager.
This block shows two issues:

+    response = mm_base_modem_at_command_finish(MM_BASE_MODEM(self), res, &error);
+    if(!response)
+    {
+        mm_dbg("Couldn't query unlock retries: '%s'", error->message);
+        goto error_ret;
+    }

First, all functions should have a space between the function name and
the "(".  Second, the { for if/else/while/for statements goes on the
same line as the if/else/while/for.  So this should be:

+    response = mm_base_modem_at_command_finish (MM_BASE_MODEM (self), res, &error);
+    if (!response) {
+        mm_dbg ("Couldn't query unlock retries: '%s'", error->message);
+        goto error_ret;
+    }

Also, arguments to continuation lines should align with the first line,
and ModemManager code only uses spaces for indentation, not tabs.  So
for example, this:

+    if (!mm_get_int_from_match_info (match_info, 1, &pin1) ||
+	!mm_get_int_from_match_info (match_info, 2, &pin2) ||
+	!mm_get_int_from_match_info (match_info, 3, &puk1) ||
+	!mm_get_int_from_match_info (match_info, 4, &puk2))
+    {
+	g_set_error (&error,
+		MM_CORE_ERROR,
+		MM_CORE_ERROR_FAILED,
+		"Failed to parse the EPINC response: '%s'",
+		response);

should now become:

+    if (!mm_get_int_from_match_info (match_info, 1, &pin1) ||
+        !mm_get_int_from_match_info (match_info, 2, &pin2) ||
+        !mm_get_int_from_match_info (match_info, 3, &puk1) ||
+        !mm_get_int_from_match_info (match_info, 4, &puk2))
+    {
+	g_set_error (&error,
+                    MM_CORE_ERROR,
+                    MM_CORE_ERROR_FAILED,
+                    "Failed to parse the EPINC response: '%s'",
+                    response);

2) Next, is there really no command to determine what supported modes
(2g/3g/4g) the device has, other than checking the model number?
Hopefully we can ask the device what it actually supports, instead of
relying on a specific model number like in get_supported_modes_ready().

3) is there any way to get the current access technology (GSM, GPRS,
EDGE, UMTS, HSDPA, HSUPA, etc) that the device is using?

Overall the patch looks pretty good, if you can clean up the style
issues then I think we could apply it and work out the other details
later.  Also, what device are you using?  I've got a D-Link DWM-156 A5
here that has a MediaTek chipset, so hopefully I can test the plugin
with it.

Thanks!
Dan