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

(6 comments)

File src/osmobts_sock.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/d16c7aca_559bdce4 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. […]

(see my other comments.)

File src/osmobts_sock.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/d1af27a2_7a83777c 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 […]

l1if_close_pdch exists when the_pcu->phy_ops (see above). I think it should be ok.

File src/pcu_l1_if.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/f635e26f_3696ac8e 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.

It checks for the_pcu->phy_ops. That should be enough since all callbacks except l1if_init_phy must be populated. I used the pointer to l1if_pdch_req to guard this before but this is no longer required. On all other function I also only use the_pcu->phy_ops now.

File src/pcu_l1_if_phy.c:

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/4ece0901_e9a8accb PS5, Line 15: struct llist_head list;

You can really put this inside ops too, and get rid of l1if_phy_entry.

When I do that then the ops can not be declared as static const inside the module because the list head would have to writable. I have kept the l1if_phy_entry list now but the only member is the list head and the pointer to ops.

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/06d76c9f_4bee4d97 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 sep […]

Done

https://gerrit.osmocom.org/c/osmo-pcu/+/31341/comment/ed00614f_720c8438 PS5, Line 27: bool singleton;

this too can be set in phy_ops. […]

Done