Re: [PATCH v3 10/10] driver/char: add RX support to the XHCI driver

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Aug 05, 2022 at 02:38:26PM +0200, Jan Beulich wrote:
> On 05.08.2022 11:58, Marek Marczykowski-Górecki wrote:
> > On Fri, Aug 05, 2022 at 10:38:13AM +0200, Jan Beulich wrote:
> >> On 26.07.2022 05:23, Marek Marczykowski-Górecki wrote:
> >>> @@ -454,6 +466,12 @@ static uint32_t xhci_trb_tfre_cc(const struct 
> >>> xhci_trb *trb)
> >>>      return trb->status >> 24;
> >>>  }
> >>>  
> >>> +/* Amount of data _not_ transferred */
> >>> +static uint32_t xhci_trb_tfre_len(const struct xhci_trb *trb)
> >>> +{
> >>> +    return trb->status & 0x1FFFF;
> >>> +}
> >>
> >> Same as xhci_trb_norm_len()?
> > 
> > Yes, I was considering to use that, but technically those are different
> > packets, only incidentally using the same bits.
> 
> I see. That's the problem with using literal numbers rather than #define-s.
> But for a driver like this I didn't want to be overly picky, and hence
> would have wanted to let you get away without introducing many more.

That's kind of the purpose of those helper functions - to extract
specific fields according to the xhci interface, one per function. An
alternative could be #define _instead of_ those functions, but I think
that would be worse.  What I mean, is the function name serves as
description of that those constants are about.

> >>> @@ -985,6 +1054,33 @@ static void dbc_flush(struct dbc *dbc, struct 
> >>> xhci_trb_ring *trb,
> >>>  }
> >>>  
> >>>  /**
> >>> + * Ensure DbC has a pending transfer TRB to receive data into.
> >>> + *
> >>> + * @param dbc the dbc to flush
> >>> + * @param trb the ring for the TRBs to transfer
> >>> + * @param wrk the work ring to receive data into
> >>> + */
> >>> +static void dbc_enqueue_in(struct dbc *dbc, struct xhci_trb_ring *trb,
> >>> +                           struct dbc_work_ring *wrk)
> >>
> >> I can't seem to be able to spot any use of this function - it being
> >> static, how do things build for you?
> > 
> > It's in dbc_uart_poll().
> 
> Oh, interesting. This then means that patch 1 on its own doesn't build.

Right, I need to move the call into this patch.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature