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

[Vasanthakumar Thiagarajan <vthiagar@codeaurora.org>]

On 2019-08-21 01:35, Johannes Berg wrote:

Thanks for the comments!

> 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.

Sure.

> 
>> +	{ /* 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?

Ok.

> 
>> +#define ATH11K_TX_RING_MASK_3 0x0
> 
> You have a LOT of masks here that are 0, that seems odd?

We'll remove them.

> 
>> +/* 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?

Yes, these read/write functions are used from other files as well. May 
be define
them as static inline in ahb.c will be fine.

> 
>> +		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.

Sure.

> 
>> +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?

These comments added during development not to miss anything during
bring up. Now it is not really needed, we'll remove them.

> 
>> +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?
> 

Sure. Thanks.

Vasanth