Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 8 Sep 2020 12:38:04 +0300 | Subject | Re: [PATCH net-next v7 1/6] net: marvell: prestera: Add driver for Prestera family ASIC devices |
| |
On Tue, Sep 8, 2020 at 11:35 AM Vadym Kochan <vadym.kochan@plvision.eu> wrote: > On Fri, Sep 04, 2020 at 10:12:07PM +0300, Andy Shevchenko wrote: > > On Fri, Sep 4, 2020 at 7:52 PM Vadym Kochan <vadym.kochan@plvision.eu> wrote:
...
> > > + words[3] |= FIELD_PREP(PRESTERA_W3_HW_DEV_NUM, (dsa->hw_dev_num >> 5)); > > > > Ditto. > > > I am not sure 5 needs to be defined as macro as it just moves > hw_dev_num's higher bits into the last word.
And why 5? I want 6, for example!
...
> > > + err = prestera_switch_init(sw); > > > + if (err) { > > > + kfree(sw); > > > > > + return err; > > > + } > > > + > > > + return 0; > > > > return err; > > > why not keep 'return 0' as indication of success point ?
Simple longer, but I'm not insisting. Your choice.
...
> > > + if (b == 0) > > > + continue; > > > + > > > + prestera_sdma_rx_desc_set_next(sdma, > > > + ring->bufs[b - 1].desc, > > > + buf->desc_dma); > > > + > > > + if (b == PRESTERA_SDMA_RX_DESC_PER_Q - 1) > > > + prestera_sdma_rx_desc_set_next(sdma, buf->desc, > > > + head->desc_dma); > > > > I guess knowing what the allowed range of bnum the above can be optimized. > > > You mean to replace PRESTERA_SDMA_RX_DESC_PER_Q by bnum ?
I don't know what you meant in above. It might be a bug, it might be that bnum is redundant and this definition may be used everywhere... But I believe there is room for improvement when I see pattern like
for (i < X) { ... if (i == 0) { ... } else if (i == X - 1) { ... } }
Either it can be while-loop (or do-while) with better semantics for the first and last item to handle or something else. Example from another review [1] in case you wonder how changes can be made. Just think about it.
[1]: https://www.spinics.net/lists/linux-pci/msg60826.html (before) https://www.spinics.net/lists/linux-pci/msg62043.html (after)
-- With Best Regards, Andy Shevchenko
| |