[alsa-devel] [PATCH] Clean up ASOC functions to support 32b values instead of 16b
Clean up ASOC functions to support 32b values instead of 16b
Signed-off-by: Jon Smirl jonsmirl@gmail.com ---
include/sound/soc.h | 22 +++++++++++----------- sound/soc/soc-core.c | 35 +++++++++++++++++------------------ sound/soc/soc-dapm.c | 32 ++++++++++++++++---------------- 3 files changed, 44 insertions(+), 45 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index 412cd83..ac59506 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -253,10 +253,10 @@ int snd_soc_set_runtime_hwparams(struct snd_pcm_substream *substream, #define snd_soc_write(codec, reg, value) codec->write(codec, reg, value)
/* codec register bit access */ -int snd_soc_update_bits(struct snd_soc_codec *codec, unsigned short reg, - unsigned short mask, unsigned short value); -int snd_soc_test_bits(struct snd_soc_codec *codec, unsigned short reg, - unsigned short mask, unsigned short value); +int snd_soc_update_bits(struct snd_soc_codec *codec, uint reg, + uint mask, uint value); +int snd_soc_test_bits(struct snd_soc_codec *codec, uint reg, + uint mask, uint value);
int snd_soc_new_ac97_codec(struct snd_soc_codec *codec, struct snd_ac97_bus_ops *ops, int num); @@ -419,8 +419,8 @@ struct snd_soc_codec { hw_write_t hw_write; hw_read_t hw_read; void *reg_cache; - short reg_cache_size; - short reg_cache_step; + uint reg_cache_size; + uint reg_cache_step;
/* dapm */ struct list_head dapm_widgets; @@ -530,11 +530,11 @@ struct soc_mixer_control {
/* enumerated kcontrol */ struct soc_enum { - unsigned short reg; - unsigned short reg2; - unsigned char shift_l; - unsigned char shift_r; - unsigned int max; + uint reg; + uint reg2; + uint shift_l; + uint shift_r; + uint max; const char **texts; void *dapm; }; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index c0e6ab4..358aa95 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -961,7 +961,7 @@ static ssize_t codec_reg_show(struct device *dev, { struct snd_soc_device *devdata = dev_get_drvdata(dev); struct snd_soc_codec *codec = devdata->codec; - int i, step = 1, count = 0; + uint i, step = 1, count = 0;
if (!codec->reg_cache_size) return 0; @@ -1039,11 +1039,11 @@ EXPORT_SYMBOL_GPL(snd_soc_free_ac97_codec); * * Returns 1 for change else 0. */ -int snd_soc_update_bits(struct snd_soc_codec *codec, unsigned short reg, - unsigned short mask, unsigned short value) +int snd_soc_update_bits(struct snd_soc_codec *codec, u32 reg, + u32 mask, u32 value) { int change; - unsigned short old, new; + u32 old, new;
mutex_lock(&io_mutex); old = snd_soc_read(codec, reg); @@ -1069,11 +1069,11 @@ EXPORT_SYMBOL_GPL(snd_soc_update_bits); * * Returns 1 for change else 0. */ -int snd_soc_test_bits(struct snd_soc_codec *codec, unsigned short reg, - unsigned short mask, unsigned short value) +int snd_soc_test_bits(struct snd_soc_codec *codec, u32 reg, + u32 mask, u32 value) { int change; - unsigned short old, new; + u32 old, new;
mutex_lock(&io_mutex); old = snd_soc_read(codec, reg); @@ -1320,7 +1320,7 @@ int snd_soc_get_enum_double(struct snd_kcontrol *kcontrol, { struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; - unsigned short val, bitmask; + u32 val, bitmask;
for (bitmask = 1; bitmask < e->max; bitmask <<= 1) ; @@ -1349,8 +1349,7 @@ int snd_soc_put_enum_double(struct snd_kcontrol *kcontrol, { struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; - unsigned short val; - unsigned short mask, bitmask; + u32 val, mask, bitmask;
for (bitmask = 1; bitmask < e->max; bitmask <<= 1) ; @@ -1512,7 +1511,7 @@ int snd_soc_put_volsw(struct snd_kcontrol *kcontrol, int max = mc->max; uint mask = (1 << fls(max)) - 1; uint invert = mc->invert; - unsigned short val, val2, val_mask; + u32 val, val2, val_mask;
val = (ucontrol->value.integer.value[0] & mask); if (invert) @@ -1618,7 +1617,7 @@ int snd_soc_put_volsw_2r(struct snd_kcontrol *kcontrol, uint mask = (1 << fls(max)) - 1; uint invert = mc->invert; int err; - unsigned short val, val2, val_mask; + u32 val, val2, val_mask;
val_mask = mask << shift; val = (ucontrol->value.integer.value[0] & mask); @@ -1683,12 +1682,12 @@ int snd_soc_get_volsw_s8(struct snd_kcontrol *kcontrol, struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); uint reg = mc->reg; int min = mc->min; - int val = snd_soc_read(codec, reg); + uint val = snd_soc_read(codec, reg);
ucontrol->value.integer.value[0] = - ((signed char)(val & 0xff))-min; + ((signed char)(val & 0xff)) - min; ucontrol->value.integer.value[1] = - ((signed char)((val >> 8) & 0xff))-min; + ((signed char)((val >> 8) & 0xff)) - min; return 0; } EXPORT_SYMBOL_GPL(snd_soc_get_volsw_s8); @@ -1710,10 +1709,10 @@ int snd_soc_put_volsw_s8(struct snd_kcontrol *kcontrol, struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); uint reg = mc->reg; int min = mc->min; - unsigned short val; + uint val;
- val = (ucontrol->value.integer.value[0]+min) & 0xff; - val |= ((ucontrol->value.integer.value[1]+min) & 0xff) << 8; + val = (ucontrol->value.integer.value[0] + min) & 0xff; + val |= ((ucontrol->value.integer.value[1] + min) & 0xff) << 8;
return snd_soc_update_bits(codec, reg, 0xffff, val); } diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 424e4e8..d88b21a 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -103,7 +103,7 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, switch (w->id) { case snd_soc_dapm_switch: case snd_soc_dapm_mixer: { - int val; + uint val; struct soc_mixer_control *mc = (struct soc_mixer_control *) w->kcontrols[i].private_value; uint reg = mc->reg; @@ -123,7 +123,7 @@ static void dapm_set_path_status(struct snd_soc_dapm_widget *w, break; case snd_soc_dapm_mux: { struct soc_enum *e = (struct soc_enum *)w->kcontrols[i].private_value; - int val, item, bitmask; + uint val, item, bitmask;
for (bitmask = 1; bitmask < e->max; bitmask <<= 1) ; @@ -206,8 +206,8 @@ static int dapm_connect_mixer(struct snd_soc_codec *codec, /* update dapm codec register bits */ static int dapm_update_bits(struct snd_soc_dapm_widget *widget) { - int change, power; - unsigned short old, new; + uint change, power; + uint old, new; struct snd_soc_codec *codec = widget->codec;
/* check for valid widgets */ @@ -221,7 +221,7 @@ static int dapm_update_bits(struct snd_soc_dapm_widget *widget)
power = widget->power; if (widget->invert) - power = (power ? 0:1); + power = (power ? 0 : 1);
old = snd_soc_read(codec, widget->reg); new = (old & ~(0x1 << widget->shift)) | (power << widget->shift); @@ -464,7 +464,7 @@ static int is_connected_input_ep(struct snd_soc_dapm_widget *widget) int dapm_reg_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { - unsigned int val; + uint val;
if (SND_SOC_DAPM_EVENT_ON(event)) val = w->on_val; @@ -695,8 +695,8 @@ static void dbg_dump_dapm(struct snd_soc_codec* codec, const char *action)
/* test and update the power status of a mux widget */ static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget, - struct snd_kcontrol *kcontrol, int mask, - int val, struct soc_enum* e) + struct snd_kcontrol *kcontrol, uint mask, + uint val, struct soc_enum *e) { struct snd_soc_dapm_path *path; int found = 0; @@ -733,8 +733,8 @@ static int dapm_mux_update_power(struct snd_soc_dapm_widget *widget,
/* test and update the power status of a mixer or switch widget */ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget, - struct snd_kcontrol *kcontrol, int reg, - int val_mask, int val, int invert) + struct snd_kcontrol *kcontrol, uint reg, + uint val_mask, uint val, uint invert) { struct snd_soc_dapm_path *path; int found = 0; @@ -755,10 +755,10 @@ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget, found = 1; if (val) /* new connection */ - path->connect = invert ? 0:1; + path->connect = invert ? 0 : 1; else /* old connection must be powered down */ - path->connect = invert ? 1:0; + path->connect = invert ? 1 : 0; break; }
@@ -1198,7 +1198,7 @@ int snd_soc_dapm_put_volsw(struct snd_kcontrol *kcontrol, int max = mc->max; uint mask = (1 << fls(max)) - 1; uint invert = mc->invert; - unsigned short val, val2, val_mask; + uint val, val2, val_mask; int ret;
val = (ucontrol->value.integer.value[0] & mask); @@ -1262,7 +1262,7 @@ int snd_soc_dapm_get_enum_double(struct snd_kcontrol *kcontrol, { struct snd_soc_dapm_widget *widget = snd_kcontrol_chip(kcontrol); struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; - unsigned short val, bitmask; + uint val, bitmask;
for (bitmask = 1; bitmask < e->max; bitmask <<= 1) ; @@ -1290,8 +1290,8 @@ int snd_soc_dapm_put_enum_double(struct snd_kcontrol *kcontrol, { struct snd_soc_dapm_widget *widget = snd_kcontrol_chip(kcontrol); struct soc_enum *e = (struct soc_enum *)kcontrol->private_value; - unsigned short val, mux; - unsigned short mask, bitmask; + uint val, mux; + uint mask, bitmask; int ret = 0;
for (bitmask = 1; bitmask < e->max; bitmask <<= 1)
On Wed, Jul 23, 2008 at 6:36 PM, Jon Smirl jonsmirl@gmail.com wrote:
+int snd_soc_update_bits(struct snd_soc_codec *codec, uint reg,
uint mask, uint value);+int snd_soc_test_bits(struct snd_soc_codec *codec, uint reg,
uint mask, uint value);
Please use "unsigned int" instead of "uint".
short reg_cache_size;short reg_cache_step;
uint reg_cache_size;uint reg_cache_step;
size_t?
struct soc_enum {
unsigned short reg;unsigned short reg2;unsigned char shift_l;unsigned char shift_r;unsigned int max;
uint reg;uint reg2;uint shift_l;uint shift_r;uint max;
In this case, 'max' was already the right size. You're now mixing the real types with abbreviated ones, and so it's inconsistent.
On 7/23/08, Timur Tabi timur@freescale.com wrote:
On Wed, Jul 23, 2008 at 6:36 PM, Jon Smirl jonsmirl@gmail.com wrote:
+int snd_soc_update_bits(struct snd_soc_codec *codec, uint reg,
uint mask, uint value);+int snd_soc_test_bits(struct snd_soc_codec *codec, uint reg,
uint mask, uint value);Please use "unsigned int" instead of "uint".
Why? uint is a standard Linux kernel definition. I almost made them all u32.
short reg_cache_size;
short reg_cache_step;
uint reg_cache_size;uint reg_cache_step;size_t?
struct soc_enum {
unsigned short reg;unsigned short reg2;unsigned char shift_l;unsigned char shift_r;unsigned int max;
uint reg;uint reg2;uint shift_l;uint shift_r;uint max;In this case, 'max' was already the right size. You're now mixing the real types with abbreviated ones, and so it's inconsistent.
-- Timur Tabi Linux kernel developer at Freescale
On Wed, Jul 23, 2008 at 9:08 PM, Jon Smirl jonsmirl@gmail.com wrote:
Please use "unsigned int" instead of "uint".
Why? uint is a standard Linux kernel definition. I almost made them all u32.
For consistency. The rest of the source file doesn't use the abbreviated types. Also, sized integer types like u32 should only be used when the variable must have a certain number of bits, so you shouldn't use u32 as a general purpose integer type.
On 7/23/08, Timur Tabi timur@freescale.com wrote:
On Wed, Jul 23, 2008 at 9:08 PM, Jon Smirl jonsmirl@gmail.com wrote:
Please use "unsigned int" instead of "uint".
Why? uint is a standard Linux kernel definition. I almost made them all u32.
For consistency. The rest of the source file doesn't use the abbreviated types. Also, sized integer types like u32 should only be used when the variable must have a certain number of bits, so you shouldn't use u32 as a general purpose integer type.
I think I may need to redo this with u32. I'm still working on my read/write reg routines and it is not obvious that it would adjust right on a 64b machine. Just because you switch to a 64b machine, my sound hardware is still 32b.
--
Timur Tabi Linux kernel developer at Freescale
On Thu, Jul 24, 2008 at 12:05:59AM -0400, Jon Smirl wrote:
I think I may need to redo this with u32. I'm still working on my read/write reg routines and it is not obvious that it would adjust
I certainly agree with Timur that 'uint' and similar should be avoided for consistency with the rest of the code.
right on a 64b machine. Just because you switch to a 64b machine, my sound hardware is still 32b.
This is less important than it might seem - drivers generally serialise written values into wire format prior to writing them to the chip so the actual size the data comes in doesn't make much odds.
On Wed, Jul 23, 2008 at 07:36:35PM -0400, Jon Smirl wrote:
Clean up ASOC functions to support 32b values instead of 16b
As it stands there's also a mix of unsigned int and u32 in here which you've already said you'll address.
Could you please provide a more detailed changelog when you resubmit this? Explaining the mechanics of the change you've made would make it a bit easier to review - obviously, much of it is replacing unsigned short in the register parameters but there's other changes in there like switching from signed to unsigned types. It may be that it could usefully be split into multiple patches, each doing one mechanical change but possibly not.
<petty>Also, it's spelt "ASoC".</petty>
@@ -221,7 +221,7 @@ static int dapm_update_bits(struct snd_soc_dapm_widget *widget)
power = widget->power; if (widget->invert)
power = (power ? 0:1);
power = (power ? 0 : 1);old = snd_soc_read(codec, widget->reg); new = (old & ~(0x1 << widget->shift)) | (power << widget->shift);
This change is reasonable enough but unrelated to the rest of the patch...
@@ -755,10 +755,10 @@ static int dapm_mixer_update_power(struct snd_soc_dapm_widget *widget, found = 1; if (val) /* new connection */
path->connect = invert ? 0:1;
path->connect = invert ? 0 : 1;
path->connect = invert ? 1:0;
path->connect = invert ? 1 : 0;
Ditto.
participants (3)
-
Jon Smirl -
Mark Brown -
Timur Tabi