Attention is currently required from: fixeria, pespin. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31341 )
Change subject: pcu_l1_if_phy: flexible phy access ......................................................................
Patch Set 5:
(15 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/d6c99db9_79b17b8d PS3, 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:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/9d34ee07_138d0199 PS3, 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:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/a6704338_2774078d PS3, Line 403: static void use_phy(struct l1if_phy *phy)
imho this is following the wrong approach. […]
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/e3152cc6_1dcab7c5 PS3, 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:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/eedc90b9_5942c7e9 PS3, Line 418: LC15BTS
copy-paste, should be OC2G
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/63fcd249_52a43a61 PS3, Line 418: static const uint8_t name[] = "LC15BTS";
wrong name, it's oc2g.
Done
File src/osmo-bts-sysmo/sysmo_l1_if.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/37268a08_5a7110c5 PS3, Line 395: LC15BTS
SYSMO?
Done
File src/pcu_l1_if.cpp:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/6df1dd8b_9384a4d8 PS3, 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:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/9f76e52d_48de1b8a PS3, Line 6: struct l1if_phy {
l1if_phy_ops?
Done
File src/pcu_l1_if_phy.c:
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b73f6714_40898976 PS3, 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
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/92b5d1a0_65700b2f PS3, Line 24: void (*init_phy_cb)(void *ctx);
This can go inside l1if_phy_ops
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/fc9d53f0_81626325 PS3, Line 38: const uint8_t *name
Why the name is not char?
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/97c73e39_07962057 PS3, 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
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/73497559_bc43bc1a PS3, 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.
https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/3a7c1ab3_3398e0b4 PS3, 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
-
dexter