On 26/01/2021 13:36, Mark Brown wrote:
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?
Sure, that makes it even better to validate the mask aswell!
+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?
I agree! something like this should be good I guess:
unsigned int snd_soc_component_read_field(...) { unsigned int val;
val = snd_soc_component_read(component, reg);
val = (val & mask) >> __soc_component_field_shift(mask);
return val; }
- 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.
True!, we could simplify this to :
int snd_soc_component_write_field(struct snd_soc_component *component, unsigned int reg, unsigned int mask, unsigned int val) { val = (val << __soc_component_field_shift(mask)) & mask;
return snd_soc_component_update_bits(component, reg, mask, val); }
Does that look okay to you?
--srini