Attention is currently required from: dexter. pespin 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 4: (14 comments) Commit Message: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/3fc333c4_e65006c1 PS3, Line 22: moment only "solitary" modules. The selection mechanism will be this sentence is not ending (no verb). Patchset: PS4: 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: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/4258c750_3c2eaa1a 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? Can you perhaps add an API "is_direct_phy()" or alike? File src/osmo-bts-litecell15/lc15_l1_if.c: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/e4a2898a_90ae728e PS3, 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. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/b33243c9_15bfa2c0 PS3, 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: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/7302e56e_54159936 PS3, Line 418: static const uint8_t name[] = "LC15BTS"; wrong name, it's oc2g. File src/osmobts_sock.c: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/e52afe65_5690283b PS3, 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: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/c0f37137_9a536212 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. 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: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/98a0856f_8480a5fd PS3, Line 6: struct l1if_phy { l1if_phy_ops? File src/pcu_l1_if_phy.c: https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/631e6054_6eeec2bf 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. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/8ad4eeee_6bd68696 PS3, Line 24: void (*init_phy_cb)(void *ctx); This can go inside l1if_phy_ops https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/227c465d_0a9498f3 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. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/46ce9a9b_7bd6c7ed PS3, 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. https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/34664682_2fd7c060 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 struct. -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31341 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I8692d1bd5d137a17cf596ee2914722f419c9978d Gerrit-Change-Number: 31341 Gerrit-PatchSet: 4 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Thu, 16 Feb 2023 09:34:43 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment