On 2019-08-21 01:53, Johannes Berg wrote:
On Tue, 2019-08-20 at 18:47 +0300, Kalle Valo wrote:
+static const struct ce_attr host_ce_config_wlan[] = {
+ /* CE0: host->target HTC control and raw streams */
+ {
+ .flags = CE_ATTR_FLAGS,
+ .src_nentries = 16,
+ .src_sz_max = 2048,
+ .dest_nentries = 0,
+ },
Curious - this looks a lot like a similar thing in AHB, but there you
did it all in little endian? Totally different really, just looks
similar, or what's the reason?
This particular table is for the driver use only. This has Copy Engine
configurations
and any Copy Engine specific send/receive callbacks. The other in ahb.c
is the configuration
sent to firmware.
+ ring->skb[write_index] = skb;
+ write_index = CE_RING_IDX_INCR(nentries_mask, write_index);
+ ring->write_index = write_index;
+
+ ath11k_hal_srng_access_end(ab, srng);
+
+ spin_unlock_bh(&srng->lock);
+
+ pipe->rx_buf_needed--;
+
+ return 0;
+
+err:
+ ath11k_hal_srng_access_end(ab, srng);
+
+ spin_unlock_bh(&srng->lock);
Seems like you could unify those unlock paths, the rx_buf_needed-- can
almost certainly be before the access_end/unlock, and then just set
ret=0?
Sure.
+static int ath11k_ce_completed_send_next(struct ath11k_ce_pipe *pipe,
+ struct sk_buff **skb)
Personally, I'd have preferred to have the *skb as the return value,
and
use ERR_PTR()/IS_ERR() etc. to encode the error values, rather than the
double pointer.
Ok.
+{
+ struct ath11k_base *ab = pipe->ab;
+ struct hal_srng *srng;
+ unsigned int sw_index;
+ unsigned int nentries_mask;
+ u32 *desc;
+ int ret = 0;
Maybe don't initialize that to a success value, just for robustness?
You
don't actually need to initialize it at all though if you set ret=0
when
you actually succeed, which might be even nicer given that the function
is simple enough for the compiler to figure out if you used 'ret'
uninitialized or not.
Ok.
+static struct ath11k_ce_ring *
+ath11k_ce_alloc_ring(struct ath11k_base *ab, int nentries, int
desc_sz)
+{
+ struct ath11k_ce_ring *ce_ring;
+ dma_addr_t base_addr;
+
+ ce_ring = kzalloc(sizeof(*ce_ring) + (nentries *
sizeof(*ce_ring->skb)),
+ GFP_KERNEL);
You should probably use struct_size().
Sure.
Thanks.
Vasanth
