Attention is currently required from: fixeria, pespin.

View Change

15 comments:

• Commit Message:

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

    this sentence is not ending (no verb).

    Done

• 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? […]

    You are right, thats indeed a bit awkward. However, with the new approach we can just check the the_pcu->phy_ops pointer. So, it should be clear then, no phy_ops, no direct phy access.

• 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. […]

    Done

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

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

    Done

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

  • Patch Set #3, Line 418: LC15BTS

    copy-paste, should be OC2G

    Done

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

    wrong name, it's oc2g.

    Done

• File src/osmo-bts-sysmo/sysmo_l1_if.c:

  • Patch Set #3, Line 395: LC15BTS

    SYSMO?

    Done

• 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. […]

    (see above)

• File src/pcu_l1_if_phy.h:

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

    l1if_phy_ops?

    Done

• 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.

    Done

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

    This can go inside l1if_phy_ops

    Done

  • Patch Set #3, Line 38: const uint8_t *name

    Why the name is not char?

    Done

  • 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.

    Done

  • 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 thought about this for a second, but I think having it separate is better since then the ops need to contain members that are only really needed by pcu_l1_if_phy.c internally.

  • 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 s […]

    Done

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