Attention is currently required from: dexter.

View Change

14 comments:

• Commit Message:

  • Patch Set #3, Line 22: moment only "solitary" modules. The selection mechanism will be

    this sentence is not ending (no verb).

• Patchset:

  • Patch Set #4:

    The idea is good but it needs a bit of rework to follow usual way of specifying drivers (eg. like in the kernel).

• File src/gprs_rlcmac_sched.cpp:

  • Patch Set #3, Line 492: if (the_pcu->phy.l1if_pdch_req) {

    this is a bit confusing. Having l1if_pdch_req means there's direct phy access? why?
    Can you perhaps add an API "is_direct_phy()" or alike?

• File src/osmo-bts-litecell15/lc15_l1_if.c:

  • Patch Set #3, Line 403: static void use_phy(struct l1if_phy *phy)

    imho this is following the wrong approach. Instead of passing a function, you should pass a pointer to a struct containing a set of functions (ops). Kind of a virtual function table.

  • Patch Set #3, Line 413: static const uint8_t name[] = "LC15BTS";

    why do you need this to be static? just pass te string directly?

• File src/osmo-bts-oc2g/oc2g_l1_if.c:

  • Patch Set #3, Line 418: static const uint8_t name[] = "LC15BTS";

    wrong name, it's oc2g.

• File src/osmobts_sock.c:

  • Patch Set #3, Line 113: if (the_pcu->phy.l1if_close_pdch && bts->trx[trx].fl1h) {

    not criticial but less confusing: I'd first check for f1h and finally for l1if_close_pdch.
    Logically you first want to check whether there's a l1 context, and then, if the driver supports the function, call the function.

    Is fl1h actually allocated by the driver? You could even add new APIs to let the driver internally decide whether the trx is being handled or not, not relying on the fl1h pointer being set here.

• File src/pcu_l1_if.cpp:

  • Patch Set #3, Line 215: if (the_pcu->phy.l1if_pdch_req && bts->trx[trx].fl1h) {

    Same comments as in the other file regarding order, API, etc.
    You could even simply always call l1if_pdch_req regardless of fl1h being set, and let the driver internally do a noop if not ready?

• File src/pcu_l1_if_phy.h:

  • Patch Set #3, Line 6: struct l1if_phy {

    l1if_phy_ops?

• File src/pcu_l1_if_phy.c:

  • Patch Set #3, Line 21: void (*use_phy_cb)(struct l1if_phy *phy);

    This is not needed if you pass a struct l1if_phy_ops to pcu_l1if_phy_register.

  • Patch Set #3, Line 24: void (*init_phy_cb)(void *ctx);

    This can go inside l1if_phy_ops

  • Patch Set #3, Line 45: l1if_phy_entry->init_phy_cb = init_phy_cb;

    this can all go inside l1if_phy_ops and be copied at once copying the whole struct.

  • Patch Set #3, Line 90: l1if_phy_entry->use_phy_cb(phy);

    I think you can really merge l1if_phy_entry and l1if_phy_ops. I see no real reason to have 2 structs.

  • Patch Set #3, Line 96: memset(phy, 0, sizeof(*phy));

    This function should only update a the_pcu->l1if_phy pointer to point to the selected l1if_phy_ops struct.

To view, visit change 31341. To unsubscribe, or for help writing mail filters, visit settings.