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

[Johannes Berg <johannes@sipsolutions.net>]

On Wed, 2019-08-21 at 14:59 +0530, Vasanthakumar Thiagarajan wrote:

> > > +#define ATH11K_TX_RING_MASK_3 0x0
> > 
> > You have a LOT of masks here that are 0, that seems odd?
> 
> We'll remove them.

I'm not sure you should just *remove* them, that might very well be
valid and what you need here, I'm just saying it looks odd since you
usually expect masks to, well, not really mask *everything*?

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

No, if they're static they cannot be used from other files, but if
they're declared and used elsewhere they can't really be inline ...

You could declare them static inline in ahb.h I guess, instead.

johannes