Hi Varun,

On 06/18/2019 08:05 AM, Varun Gargi wrote:
Adding support for enumerating PCIe types of modems in ofono
---
  plugins/udevng.c | 190 ++++++++++++++++++++++++++++++++++++++++++++++++-------
  1 file changed, 168 insertions(+), 22 deletions(-)
  mode change 100644 => 100755 plugins/udevng.c

Please don't touch the mode


diff --git a/plugins/udevng.c b/plugins/udevng.c
old mode 100644
new mode 100755
index 4b420dc..79eec42
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -41,6 +41,7 @@
  enum modem_type {
        MODEM_TYPE_USB,
        MODEM_TYPE_SERIAL,
+       MODEM_TYPE_PCIE,
  };
struct modem_info {
@@ -1198,26 +1199,39 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
                                info->interface, info->number, info->label,
                                info->sysattr, info->subsystem);
- if (g_strcmp0(modem->model,"095a") == 0) {
-                       if (g_strcmp0(info->subsystem, "tty") == 0) {
-                               if (g_strcmp0(info->number, "00") == 0)
-                                       mdm = info->devnode;
-                       } else if (g_strcmp0(info->subsystem, "net") == 0) {
-                               if (g_strcmp0(info->number, "06") == 0)
-                                       net = info->devnode;
-                               if (g_strcmp0(info->number, "08") == 0)
-                                       net2 = info->devnode;
-                               if (g_strcmp0(info->number, "0a") == 0)
-                                       net3 = info->devnode;
-                       }
-               } else {
-                       if (g_strcmp0(info->subsystem, "tty") == 0) {
-                               if (g_strcmp0(info->number, "02") == 0)
-                                       mdm = info->devnode;
-                       } else if (g_strcmp0(info->subsystem, "net") == 0) {
-                               if (g_strcmp0(info->number, "00") == 0)
-                                       net = info->devnode;
+               if (g_strcmp0(info->subsystem, "pci") == 0) {
+                       mdm = "/dev/iat";
+                       net = "/dev/inm0";
+                       net2 = "/dev/inm1";
+                       net3 = "/dev/inm2";

These seem to be quite hardcoded, what if multiple modems are inserted?

+                       ofono_modem_set_string(modem->modem,
+                                       "CtrlPath", "/PCIE/IOSM/CTRL/1");
+                       ofono_modem_set_string(modem->modem, "DataPath",
+                                       "/PCIE/IOSM/IPS/");

This seems to treat all PCIE modems the same way. Is there a possibility of this changing in the future? E.g. should you actually check the vid/pid or limit which devices this applies to some other way? It would be preferable if we had to 'opt-in' devices along the way, rather than having to deal with a new device being detected and categorized incorrectly.

+               } else { /* For USB */
+                       if (g_strcmp0(modem->model,"095a") == 0) {
+                               if (g_strcmp0(info->subsystem, "tty") == 0) {
+                                       if (g_strcmp0(info->number, "00") == 0)
+                                               mdm = info->devnode;
+                               } else if (g_strcmp0(info->subsystem, "net") == 
0) {
+                                       if (g_strcmp0(info->number, "06") == 0)
+                                               net = info->devnode;
+                                       if (g_strcmp0(info->number, "08") == 0)
+                                               net2 = info->devnode;
+                                       if (g_strcmp0(info->number, "0a") == 0)
+                                               net3 = info->devnode;
+                               }
+                       } else {
+                               if (g_strcmp0(info->subsystem, "tty") == 0) {
+                                       if (g_strcmp0(info->number, "02") == 0)
+                                               mdm = info->devnode;
+                               } else if (g_strcmp0(info->subsystem, "net") == 
0) {
+                                       if (g_strcmp0(info->number, "00") == 0)
+                                               net = info->devnode;
+                               }
                        }
+               ofono_modem_set_string(modem->modem, "CtrlPath", "/USBCDC/0");
+               ofono_modem_set_string(modem->modem, "DataPath", "/USBHS/NCM/");

Coding style is all wrong here

                }
        }
@@ -1235,9 +1249,6 @@ static gboolean setup_xmm7xxx(struct modem_info *modem)
        if (net3)
                ofono_modem_set_string(modem->modem, "NetworkInterface3", net3);
- ofono_modem_set_string(modem->modem, "CtrlPath", "/USBCDC/0");
-       ofono_modem_set_string(modem->modem, "DataPath", "/USBHS/NCM/");
-
        return TRUE;
  }
@@ -1406,6 +1417,7 @@ static void destroy_modem(gpointer data) switch (modem->type) {
        case MODEM_TYPE_USB:
+       case MODEM_TYPE_PCIE:
                for (list = modem->devices; list; list = list->next) {
                        struct device_info *info = list->data;
@@ -1436,6 +1448,7 @@ static gboolean check_remove(gpointer key, gpointer value, gpointer user_data) switch (modem->type) {
        case MODEM_TYPE_USB:
+       case MODEM_TYPE_PCIE:
                for (list = modem->devices; list; list = list->next) {
                        struct device_info *info = list->data;
@@ -1565,6 +1578,86 @@ static void add_serial_device(struct udev_device *dev) modem->serial = info;
  }
+static void add_pci_device(const char *syspath, const char *devname,
+                       const char *driver, const char *vendor,
+                       const char *model, struct udev_device *device)
+{
+       const char *devpath, *devnode , *interface , *number;
+       const char *label, *sysattr, *subsystem;
+       struct modem_info *modem;
+       struct device_info *info;
+       struct udev_device *parent;
+
+       devpath = udev_device_get_syspath(device);
+
+       if (devpath == NULL)
+               return;
+
+       devnode = udev_device_get_devnode(device);
+
+       if (devnode == NULL) {
+               devnode = udev_device_get_property_value(device, "INTERFACE");
+               DBG("devnode = %s", devnode );
+       }
+
+       modem = g_hash_table_lookup(modem_list, syspath);
+
+       if (modem == NULL) {
+               modem = g_try_new0(struct modem_info, 1);
+
+               if (modem == NULL)
+                       return;
+
+               modem->type = MODEM_TYPE_PCIE;
+               modem->syspath = g_strdup(syspath);
+               modem->devname = g_strdup(devname);
+               modem->driver = g_strdup(driver);
+               modem->vendor = g_strdup(vendor);
+               modem->model = g_strdup(model);
+               modem->sysattr = get_sysattr(driver);
+
+               g_hash_table_replace(modem_list, modem->syspath, modem);
+       }
+
+       interface = udev_device_get_property_value(device, "INTERFACE");
+       /* If environment variable is not set, get value from attributes (or 
parent's ones) */
+       number = udev_device_get_sysattr_value(device, "bInterfaceNumber");
+
+       if (number == NULL) {
+               parent = udev_device_get_parent(device);
+               number = udev_device_get_sysattr_value(parent, 
"bInterfaceNumber");
+       }
+
+       label = udev_device_get_property_value(device, "OFONO_LABEL");
+
+       if (!label)
+               label = udev_device_get_property_value(device, "OFONO_LABEL");
+
+       subsystem = udev_device_get_subsystem(device);
+
+       if (modem->sysattr != NULL)
+               sysattr = udev_device_get_sysattr_value(device, modem->sysattr);
+       else
+               sysattr = NULL;
+
+       DBG("%s (%s) %s [%s] ==> %s %s", devnode, driver,
+                                       interface, number, label, sysattr);
+       info = g_try_new0(struct device_info, 1);
+
+       if (info == NULL)
+               return;
+
+       info->devpath = g_strdup(devpath);
+       info->devnode = g_strdup(devnode);
+       info->interface = g_strdup(interface);
+       info->number = g_strdup(number);
+       info->label = g_strdup(label);
+       info->sysattr = g_strdup(sysattr);
+       info->subsystem = g_strdup(subsystem);
+
+       modem->devices = g_slist_insert_sorted(modem->devices, info,
+                                                       compare_device);

This function seems to be mostly copy-pasted version of add_device with a few minor variations. Can we commonize any code between these two?

+}
static void add_device(const char *syspath, const char *devname,
                        const char *driver, const char *vendor,
@@ -1594,8 +1687,10 @@ static void add_device(const char *syspath, const char 
*devname,
                return;
modem = g_hash_table_lookup(modem_list, syspath);
+
        if (modem == NULL) {
                modem = g_try_new0(struct modem_info, 1);
+
                if (modem == NULL)
                        return;

Superfluous changes?

@@ -1739,6 +1834,7 @@ static struct {
        { "telit",    "cdc_acm",    "1bc7", "0036"      },
        { "xmm7xxx",  "cdc_acm",    "8087"                },
        { "xmm7xxx",  "cdc_ncm",    "8087"                },
+    { "xmm7xxx",     "imc_ipc",    "0x8086", "0x7560"},

Something is funky with the formatting here

        { }
  };
@@ -1825,6 +1921,53 @@ static void check_usb_device(struct udev_device *device)
        add_device(syspath, devname, driver, vendor, model, device);
  }
+static void check_pci_device(struct udev_device *device)
+{
+       const char *syspath, *devname , *driver;
+       const char *vendor = NULL, *model = NULL, *drv = NULL;
+       unsigned int i;
+
+       syspath = udev_device_get_syspath(device);
+
+       if (syspath == NULL)
+               return;
+
+       devname = udev_device_get_devnode(device);
+       vendor = udev_device_get_sysattr_value(device, "vendor");
+       model = udev_device_get_sysattr_value(device, "device");
+       driver = udev_device_get_property_value(device, "OFONO_DRIVER");
+       drv = udev_device_get_property_value(device, "DRIVER");
+       DBG("%s [%s:%s]", drv, vendor, model);
+
+       if (vendor == NULL || model == NULL)
+               return;
+
+       for (i = 0; vendor_list[i].driver; i++) {
+               if (( drv != NULL) && (g_str_equal(vendor_list[i].drv, drv) == 
TRUE)) {
+                       DBG("vendor_list[%d].drv = %s",i, vendor_list[i].drv );
+               } else {
+                        continue;
+               }

Not our style, also {} are not needed

+
+               if (vendor_list[i].vid) {
+                       if (!g_str_equal(vendor_list[i].vid, vendor))
+                               continue;
+               }
+
+               if (vendor_list[i].pid) {
+                       if (!g_str_equal(vendor_list[i].pid, model))
+                               continue;
+                       }
+                       driver = vendor_list[i].driver;

Not our style

+               }
+
+               if (driver == NULL)
+                       return;
+               else
+                       DBG("Driver Found %s", driver);
+
+       add_pci_device(syspath, devname, driver, vendor, model, device);
+}

doc/coding-style.txt item M1

  static void check_device(struct udev_device *device)
  {
        const char *bus;
@@ -1839,6 +1982,8 @@ static void check_device(struct udev_device *device)
        if ((g_str_equal(bus, "usb") == TRUE) ||
                        (g_str_equal(bus, "usbmisc") == TRUE))
                check_usb_device(device);
+       else if (g_str_equal(bus, "pci") == TRUE)
+               check_pci_device(device);
        else
                add_serial_device(device);
@@ -1899,6 +2044,7 @@ static void enumerate_devices(struct udev *context)
        udev_enumerate_add_match_subsystem(enumerate, "usbmisc");
        udev_enumerate_add_match_subsystem(enumerate, "net");
        udev_enumerate_add_match_subsystem(enumerate, "hsi");
+       udev_enumerate_add_match_subsystem(enumerate, "pci");
udev_enumerate_scan_devices(enumerate);

Regards,
-Denis
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono

Reply via email to