Hi Andy,
On Thu, 22 Feb 2024 17:47:34 +0200
Andy Shevchenko <andriy.shevche...@linux.intel.com> wrote:
...
> > +static int qmc_hdlc_xlate_slot_map(struct qmc_hdlc *qmc_hdlc,
> > + u32 slot_map, struct qmc_chan_ts_info
> > *ts_info)
> > +{
> > + DECLARE_BITMAP(ts_mask_avail, 64);
> > + DECLARE_BITMAP(ts_mask, 64);
> > + DECLARE_BITMAP(map, 64);
>
> Perhaps more 1:1 naming?
>
> DECLARE_BITMAP(rx_ts_mask_avail, 64);
> DECLARE_BITMAP(tx_ts_mask, 64);
> DECLARE_BITMAP(slot_map, 64);
I disagree.
I first check that ts_info->rx_ts_mask_avail and ts_info->tx_ts_mask_avail are
identical then I use one of them to create the ts_mask_avail.
Then I compute the ts_mask and update both ts_info->tx_ts_mask and
ts_info->rx_ts_mask.
ts_mask_avail and ts_mask bitmaps are used for tx and rx.
I could name them txrx_ts_mask* but that doesn't do much.
For DECLARE_BITMAP(slot_map, 64), slot_map is the name of the function
parameter...
I think we can keep 'map' for the bitmap here.
>
> > + /* Tx and Rx available masks must be identical */
> > + if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
> > + dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch
> > (0x%llx, 0x%llx)\n",
> > + ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
> > + return -EINVAL;
> > + }
> > +
> > + bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
> > + bitmap_from_u64(map, slot_map);
> > + bitmap_scatter(ts_mask, map, ts_mask_avail, 64);
> > +
> > + if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
> > + dev_err(qmc_hdlc->dev, "Cannot translate timeslots %*pb ->
> > (%*pb, %*pb)\n",
> > + 64, map, 64, ts_mask_avail, 64, ts_mask);
>
>
> You can save a bit of code and stack:
Will be updated in the next iteration.
...
> > + ret = qmc_chan_get_ts_info(qmc_hdlc->qmc_chan, &ts_info);
> > + if (ret) {
> > + dev_err(qmc_hdlc->dev, "get QMC channel ts info failed %d\n",
> > ret);
> > + return ret;
>
> return dev_err_probe(...);
Will be updated too :)
>
> > + }
>
Thanks for the review,
Hervé
