On Wed, Nov 07, 2018 at 02:30:43PM +0100, Jonas Bonn wrote:
> Hi Clement,
>
> On 07/11/2018 11:14, Clement Viel wrote:
> >---
> > plugins/udevng.c | 92
> > +++++++++++++++++++++++++++++++++++++++++---------------
> > 1 file changed, 68 insertions(+), 24 deletions(-)
> >
> >diff --git a/plugins/udevng.c b/plugins/udevng.c
> >index ff6e1fc..ee6f9e8 100644
> >--- a/plugins/udevng.c
> >+++ b/plugins/udevng.c
> >@@ -711,8 +711,56 @@ static gboolean setup_telitqmi(struct modem_info *modem)
> > return TRUE;
> > }
> >-/* TODO: Not used as we have no simcom driver */
> >-static gboolean setup_simcom(struct modem_info *modem)
> >+
> >+static gboolean setup_sim800(struct modem_info *modem)
> >+{
> >+ const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
> >+ GSList *list;
> >+
> >+ DBG("%s", modem->syspath);
> >+
> >+ for (list = modem->devices; list; list = list->next) {
> >+ struct device_info *info = list->data;
> >+
> >+ DBG("%s %s %s %s", info->devnode, info->interface,
> >+ info->number, info->label);
> >+
> >+ if (g_strcmp0(info->label, "aux") == 0) {
> >+ DBG("setting aux as info->devnode");
> >+ aux = info->devnode;
> >+ if (mdm != NULL)
> >+ break;
> >+ } else if (g_strcmp0(info->label, "modem") == 0) {
> >+ mdm = info->devnode;
> >+ if (aux != NULL)
> >+ break;
> >+ } else if (g_strcmp0(info->interface, "255/0/0") == 0) {
> >+ if (g_strcmp0(info->number, "00") == 0)
> >+ mdm = info->devnode;
> >+ else if (g_strcmp0(info->number, "01") == 0)
> >+ gps = info->devnode;
> >+ else if (g_strcmp0(info->number, "02") == 0)
> >+ aux = info->devnode;
> >+ else if (g_strcmp0(info->number, "03") == 0)
> >+ mdm = info->devnode;
> >+ }
> >+ }
> >+ DBG("modem=%s aux=%s gps=%s diag=%s", mdm, aux, gps, diag);
> >+
>
> gps is unused.
>
>
> >+ if (mdm == NULL) {
> >+ DBG("modem did not set up");
> >+ return FALSE;
> >+ }
> >+
> >+ ofono_modem_set_string(modem->modem, "Device", mdm);
>
> You probably mean 'aux' here.
>
> >+ ofono_modem_set_string(modem->modem, "Modem", mdm);
> >+
> >+ return TRUE;
> >+}
> >+
> >+
> >+
> >+static gboolean setup_sim900(struct modem_info *modem)
> > {
> > const char *mdm = NULL, *aux = NULL, *gps = NULL, *diag = NULL;
> > GSList *list;
> >@@ -962,6 +1010,8 @@ static gboolean setup_mbim(struct modem_info *modem)
> > ofono_modem_set_string(modem->modem, "Device", ctl);
> > ofono_modem_set_string(modem->modem, "NetworkInterface", net);
> > ofono_modem_set_string(modem->modem, "DescriptorFile", descriptors);
> >+ ofono_modem_set_string(modem->modem, "Vendor", modem->vendor);
> >+ ofono_modem_set_string(modem->modem, "Model", modem->model);
>
> I don't see that these strings are being used anywhere in the modem driver.
>
> > return TRUE;
> > }
> >@@ -1193,22 +1243,12 @@ 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;
> >- }
> >- } 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, "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;
> > }
> > }
>
> This change doesn't look like it belongs to this patch.
>
>
> >@@ -1290,7 +1330,8 @@ static struct {
> > { "nokia", setup_nokia },
> > { "telit", setup_telit, "device/interface" },
> > { "telitqmi", setup_telitqmi },
> >- { "simcom", setup_simcom },
> >+ { "sim800", setup_sim800 },
> >+ { "sim900", setup_sim900 },
>
> Is the sim900 not a serial modem? These setup routines are really only for
> USB devices.
>
> > { "sim7100", setup_sim7100 },
> > { "zte", setup_zte },
> > { "icera", setup_icera },
> >@@ -1308,7 +1349,9 @@ static struct {
> > { "calypso", setup_serial_modem },
> > { "cinterion", setup_serial_modem },
> > { "nokiacdma", setup_serial_modem },
> >+ { "sim800", setup_serial_modem },
> > { "sim900", setup_serial_modem },
> >+ { "sim800", setup_serial_modem },
>
> You've added the same line twice. Furthermore, the sim800 is a USB modem
> (???) so it should not be set up in this way.
>
> > { "wavecom", setup_wavecom },
> > { "tc65", setup_tc65 },
> > { "ehs6", setup_ehs6 },
> >@@ -1471,7 +1514,7 @@ static void add_serial_device(struct udev_device *dev)
> > const char *subsystem;
> > struct udev_device* mdev;
> > const char* driver;
> >-
> >+ DBG("adding %s interface", udev_device_get_devpath(dev));
> > mdev = get_serial_modem_device(dev);
> > if (!mdev) {
> > DBG("Device is missing required OFONO_DRIVER property");
> >@@ -1663,7 +1706,8 @@ static struct {
> > { "alcatel", "option", "1bbb", "0017" },
> > { "novatel", "option", "1410" },
> > { "zte", "option", "19d2" },
> >- { "simcom", "option", "05c6", "9000" },
> >+ { "sim800", "option", "05c6", "9000" }
> >+ { "sim900", "option", "05c6", "9000" },
>
>
> The sim900 is a serial modem... there is no vendor id nor product id.
>
>
> > { "sim7100", "option", "1e0e", "9001" },
> > { "telit", "usbserial", "1bc7" },
> > { "telit", "option", "1bc7" },
> >@@ -1693,7 +1737,6 @@ static struct {
> > { "xmm7xxx", "cdc_ncm", "8087" },
> > { }
> > };
> >-
> > static void check_usb_device(struct udev_device *device)
> > {
> > struct udev_device *usb_device;
> >@@ -1722,9 +1765,10 @@ static void check_usb_device(struct udev_device
> >*device)
> > udev_device_get_parent_with_subsystem_devtype(
> > device, "usb", "usb_interface");
> >- if (usb_interface)
> >+ if (usb_interface) {
> > driver = udev_device_get_property_value(
> > usb_interface, "OFONO_DRIVER");
> >+ }
> > }
>
> No.
>
> > if (driver == NULL) {
> >@@ -1801,7 +1845,7 @@ static gboolean create_modem(gpointer key, gpointer
> >value, gpointer user_data)
> > struct modem_info *modem = value;
> > const char *syspath = key;
> > unsigned int i;
> >-
> >+ DBG("");
> > if (modem->modem != NULL)
> > return FALSE;
>
> Doesn't belong to this patch, if at all.
>
> >
>
> /Jonas
Hi Jonas,
Thanks for the review, this patch is all wrong because it's not the good
one...litlle bit tired, i'm sorry.
_______________________________________________
ofono mailing list
[email protected]
https://lists.ofono.org/mailman/listinfo/ofono
