Hi Ping-Ke,
as always: thank you so much for taking time to go through this!
On Wed, Dec 28, 2022 at 10:39 AM Ping-Ke Shih <pkshih@xxxxxxxxxxx> wrote:
[...]
>
> > +
> > +static void rtw_sdio_writel(struct rtw_sdio *rtwsdio, u32 val,
> > + u32 addr, int *ret)
> > +{
> > + u8 buf[4];
> > + int i;
> > +
> > + if (!(addr & 3) && rtwsdio->is_powered_on) {
> > + sdio_writel(rtwsdio->sdio_func, val, addr, ret);
> > + return;
> > + }
> > +
> > + *(__le32 *)buf = cpu_to_le32(val);
> > +
> > + for (i = 0; i < 4; i++) {
> > + sdio_writeb(rtwsdio->sdio_func, buf[i], addr + i, ret);
> > + if (*ret)
>
> Do you need some messages to know something wrong?
It's not obvious but we're already logging that something went wrong.
The messages are logged in rtw_sdio_{read,write}{8,16,32}.
We do this because there's multiple ways to access data (direct,
indirect, ...) and some of them require multiple register operations.
So we print one message in the end.
[...]
> > +static u8 rtw_sdio_read_indirect8(struct rtw_dev *rtwdev, u32 addr, int *ret)
> > +{
> > + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > + u32 reg_cfg, reg_data;
> > + int retry;
> > + u8 tmp;
> > +
> > + reg_cfg = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_CFG);
> > + reg_data = rtw_sdio_to_bus_offset(rtwdev, REG_SDIO_INDIRECT_REG_DATA);
> > +
> > + rtw_sdio_writel(rtwsdio, BIT(19) | addr, reg_cfg, ret);
> > + if (*ret)
> > + return 0;
> > +
> > + for (retry = 0; retry < RTW_SDIO_INDIRECT_RW_RETRIES; retry++) {
> > + tmp = sdio_readb(rtwsdio->sdio_func, reg_cfg + 2, ret);
> > + if (!ret && tmp & BIT(4))
>
> 'ret' is pointer, do you need '*' ?
Well spotted - thank you!
[...]
> As I look into sdio_readb(), it use 'int *err_ret' as arugment.
> Would you like to change ' int *ret' to 'int *err_ret'?
> It could help to misunderstand.
Sure, I'll do that
[...]
> > + rtw_write16(rtwdev, REG_RXDMA_AGG_PG_TH, size |
> > + (timeout << BIT_SHIFT_DMA_AGG_TO_V1));
>
> BIT_RXDMA_AGG_PG_TH GENMASK(7, 0) // for size
> BIT_DMA_AGG_TO_V1 GENMASK(15, 8) // for timeout
Thanks, I'll use these
[...]
> > +static void rtw_sdio_rx_isr(struct rtw_dev *rtwdev)
> > +{
> > + u32 rx_len;
> > +
> > + while (true) {
>
> add a limit to prevent infinite loop.
Do you have any recommendations on how many packets to pull in one go?
My thinking is: pulling to little data at once can hurt performance
[...]
>
> > +
> > +static void rtw_sdio_process_tx_queue(struct rtw_dev *rtwdev,
> > + enum rtw_tx_queue_type queue)
> > +{
> > + struct rtw_sdio *rtwsdio = (struct rtw_sdio *)rtwdev->priv;
> > + struct sk_buff *skb;
> > + int ret;
> > +
> > + while (true) {
>
> Can we have a limit?
Similar to the question above: do you have any recommendations on how
many packets (per queue) to send in one go?
Best regards,
Martin
