Re: [RFC PATCH 1/2] ASoC: soc-component: add snd_soc_component_read/write_field()

On Tue, Jan 26, 2021 at 12:20:19PM +0000, Srinivas Kandagatla wrote:

+#define __soc_component_field_shift(x) (__builtin_ffs(x) - 1)

Why not have this be a static inline?

+unsigned int snd_soc_component_read_field(struct snd_soc_component *component,

• 			  unsigned int reg, unsigned int mask)
  

+{

• unsigned int val;
• mutex_lock(&component->io_mutex);
• val = soc_component_read_no_lock(component, reg);
• if (mask)

• val = (val & mask) >> __soc_component_field_shift(mask);
  

I don't understand why this is open coding the locking when it's just a simple read and then shift?

• mutex_lock(&component->io_mutex);
• old = soc_component_read_no_lock(component, reg);
• val = val << __soc_component_field_shift(mask);
• new = (old & ~mask) | (val & mask);
• change = old != new;
• if (change)

• ret = soc_component_write_no_lock(component, reg, new);
  

• mutex_unlock(&component->io_mutex);

This needs the lock as it's a read/modify/write but could also be implemented in terms of the existing update_bits() operation rather than open coding it.