Re: [PATCH 04/49] ath11k: add ahb.c

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux