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
