Attention is currently required from: fixeria, 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 5:

(5 comments)

File src/osmobts_sock.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/ecc9bf19_708699b9 PS5, Line 115: bts->trx[trx].fl1h = NULL; After another looks, this looks even worse, because you are not skipping setting fl1h to NULL if phy_ops is not defined. Are you shure that's fine? I'd really do a 2 level if() first checking for fl1h and later calling the function ig phy_ops.l1if_close_pdch exists.

File src/pcu_l1_if.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/44c26336_8f8c36b3 PS5, Line 218: the_pcu->phy_ops->l1if_pdch_req(bts->trx[trx].fl1h, ts, 0, fn, arfcn, block_nr, guard this with "if (the_pcu->phy_ops->l1if_pdch_req)". Similar for all other function calls.

File src/pcu_l1_if_phy.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/3baf3944_2dc76b1d PS5, Line 15: struct llist_head list; You can really put this inside ops too, and get rid of l1if_phy_entry.

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/9730a7aa_e15775a2 PS5, Line 18: const char *name; this can really be in li1f_phy_ops and assign the pointer statically, there's no need to have it separate. IT makes more sense to have it inside the ops since it goes together, it identifies a given set of ops.

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/0c9ea33c_53bae5dc PS5, Line 27: bool singleton; this too can be set in phy_ops. I don't see the need of passing several params to register(), one is enough.