On Tue, 2019-08-20 at 18:47 +0300, Kalle Valo wrote:
>
> +static const struct service_to_pipe target_service_to_ce_map_wlan[] = {
> + {
> + __cpu_to_le32(ATH11K_HTC_SVC_ID_WMI_DATA_VO),
> + __cpu_to_le32(PIPEDIR_OUT), /* out = UL = host -> target */
> + __cpu_to_le32(3),
> + },
this might be nicer as C99 initializers as well? It's a struct of some
sort, after all.
> + { /* must be last */
> + __cpu_to_le32(0),
> + __cpu_to_le32(0),
> + __cpu_to_le32(0),
> + },
You don't need endian conversion for 0, even sparse will not complain,
but I'd argue it should anyway be something like
{ /* terminator entry */ }
since that's why it's there I guess?
> +#define ATH11K_TX_RING_MASK_3 0x0
You have a LOT of masks here that are 0, that seems odd?
> +/* enum ext_irq_num - irq nubers that can be used by external modules
typo ("numbers")
> +inline u32 ath11k_ahb_read32(struct ath11k_base *ab, u32 offset)
> +{
> + return ioread32(ab->mem + offset);
> +}
> +
> +inline void ath11k_ahb_write32(struct ath11k_base *ab, u32 offset, u32 value)
> +{
> + iowrite32(value, ab->mem + offset);
> +}
Just "inline" doesn't seem to make that much sense? If it's only used
here then I guess it should be static, otherwise not inline? Or maybe
you want it to be inlined *in this file* but available out-of-line
otherwise? I'm not sure that actually is guaranteed to work right in C?
> + val = ath11k_ahb_read32(ab, CE_HOST_IE_ADDRESS);
> + val |= BIT(ce_id);
> + ath11k_ahb_write32(ab, CE_HOST_IE_ADDRESS, val);
You could perhaps benefit from ath11k_ahb_setbit32() or something like
that, this repeats a few times?
> + if (__le32_to_cpu(ce_config->pipedir) & PIPEDIR_OUT) {
> + val = ath11k_ahb_read32(ab, CE_HOST_IE_ADDRESS);
> + val &= ~BIT(ce_id);
> + ath11k_ahb_write32(ab, CE_HOST_IE_ADDRESS, val);
and clearbit32() maybe. Dunno, I saw only 3 instances of each here I
guess, but still, feels repetitive.
> +int ath11k_ahb_start(struct ath11k_base *ab)
> +{
> + ath11k_ahb_ce_irqs_enable(ab);
> + ath11k_ce_rx_post_buf(ab);
> +
> + /* Bring up other components as appropriate */
Hmm. What would be appropriate? It's not really doing anything else?
> +void ath11k_ahb_stop(struct ath11k_base *ab)
> +{
> + if (!test_bit(ATH11K_FLAG_CRASH_FLUSH, &ab->dev_flags))
> + ath11k_ahb_ce_irqs_disable(ab);
> + ath11k_ahb_sync_ce_irqs(ab);
> + ath11k_ahb_kill_tasklets(ab);
> + del_timer_sync(&ab->rx_replenish_retry);
> + ath11k_ce_cleanup_pipes(ab);
> + /* Shutdown other components as appropriate */
likewise, it's not doing anything else?
johannes
