| Message ID | 1635159976-17355-1-git-send-email-spujar@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ASoC: tegra: Add master volume/mute control support | expand |
On 25. 10. 21 13:06, Sameer Pujar wrote: > @@ -150,11 +186,22 @@ static int tegra210_mvc_put_mute(struct snd_kcontrol *kcontrol, ... > > return 1; It's a bit unrelated comment to this change, but it may be worth to verify all kcontrol put callbacks in the tegra code. Ensure that value 1 is returned only when something was really changed in hardware. The tegra210_i2s_put_control() has opposite problem for example - returns always 0 which means that the change notifications are not send to subscribed applications. Jaroslav
On 10/25/2021 6:28 PM, Jaroslav Kysela wrote: > On 25. 10. 21 13:06, Sameer Pujar wrote: > >> @@ -150,11 +186,22 @@ static int tegra210_mvc_put_mute(struct >> snd_kcontrol *kcontrol, > > ... >> >> return 1; > > It's a bit unrelated comment to this change, but it may be worth to > verify all > kcontrol put callbacks in the tegra code. Ensure that value 1 is > returned only > when something was really changed in hardware. > > The tegra210_i2s_put_control() has opposite problem for example - returns > always 0 which means that the change notifications are not send to > subscribed > applications. > Thanks Jaroslav for review. I will keep a note to review put() calls in Tegra drivers and send fixes accordingly.
On 10/26/2021 11:53 AM, Sameer Pujar wrote: > > > On 10/25/2021 6:28 PM, Jaroslav Kysela wrote: >> On 25. 10. 21 13:06, Sameer Pujar wrote: >> >>> @@ -150,11 +186,22 @@ static int tegra210_mvc_put_mute(struct >>> snd_kcontrol *kcontrol, >> >> ... >>> >>> return 1; >> >> It's a bit unrelated comment to this change, but it may be worth to >> verify all >> kcontrol put callbacks in the tegra code. Ensure that value 1 is >> returned only >> when something was really changed in hardware. There are cases when the mixer control update is not immediately written to HW, instead the update is ACKed (stored in variable) and writen to HW at a later point of time. Do these cases qualify for "return 1" as well? >> >> The tegra210_i2s_put_control() has opposite problem for example - >> returns >> always 0 which means that the change notifications are not send to >> subscribed >> applications. >> > > Thanks Jaroslav for review. I will keep a note to review put() calls > in Tegra drivers and send fixes accordingly.
On Fri, Oct 29, 2021 at 08:38:54PM +0530, Sameer Pujar wrote: > On 10/26/2021 11:53 AM, Sameer Pujar wrote: > > On 10/25/2021 6:28 PM, Jaroslav Kysela wrote: > > > It's a bit unrelated comment to this change, but it may be worth to > > > verify all > > > kcontrol put callbacks in the tegra code. Ensure that value 1 is > > > returned only > > > when something was really changed in hardware. > There are cases when the mixer control update is not immediately written to > HW, instead the update is ACKed (stored in variable) and writen to HW at a > later point of time. Do these cases qualify for "return 1" as well? What matters is the user visible effect. It doesn't matter when the change gets written to the hardware, the important thing is that an applicaton will read back a new value and users will observe whatver change the control change caused.
On 29. 10. 21 17:08, Sameer Pujar wrote: > > > On 10/26/2021 11:53 AM, Sameer Pujar wrote: >> >> >> On 10/25/2021 6:28 PM, Jaroslav Kysela wrote: >>> On 25. 10. 21 13:06, Sameer Pujar wrote: >>> >>>> @@ -150,11 +186,22 @@ static int tegra210_mvc_put_mute(struct >>>> snd_kcontrol *kcontrol, >>> >>> ... >>>> >>>> return 1; >>> >>> It's a bit unrelated comment to this change, but it may be worth to >>> verify all >>> kcontrol put callbacks in the tegra code. Ensure that value 1 is >>> returned only >>> when something was really changed in hardware. > > There are cases when the mixer control update is not immediately written > to HW, instead the update is ACKed (stored in variable) and writen to HW > at a later point of time. Do these cases qualify for "return 1" as well? Yes - assuming that the get callback returns the cached value. The get/put implementation should be consistent from the caller view. The driver implementation (delayed write) is a separate thing. Jaroslav
On 10/29/2021 8:56 PM, Jaroslav Kysela wrote: > External email: Use caution opening links or attachments > > > On 29. 10. 21 17:08, Sameer Pujar wrote: >> >> >> On 10/26/2021 11:53 AM, Sameer Pujar wrote: >>> >>> >>> On 10/25/2021 6:28 PM, Jaroslav Kysela wrote: >>>> On 25. 10. 21 13:06, Sameer Pujar wrote: >>>> >>>>> @@ -150,11 +186,22 @@ static int tegra210_mvc_put_mute(struct >>>>> snd_kcontrol *kcontrol, >>>> >>>> ... >>>>> >>>>> return 1; >>>> >>>> It's a bit unrelated comment to this change, but it may be worth to >>>> verify all >>>> kcontrol put callbacks in the tegra code. Ensure that value 1 is >>>> returned only >>>> when something was really changed in hardware. >> >> There are cases when the mixer control update is not immediately written >> to HW, instead the update is ACKed (stored in variable) and writen to HW >> at a later point of time. Do these cases qualify for "return 1" as well? > > Yes - assuming that the get callback returns the cached value. The > get/put > implementation should be consistent from the caller view. The driver > implementation (delayed write) is a separate thing. Thanks Jaroslav and Mark. I have now sent a separate series to fix Tegra210 (and later) drivers.
diff --git a/sound/soc/tegra/tegra210_mvc.c b/sound/soc/tegra/tegra210_mvc.c index 7b9c700..40cd21a 100644 --- a/sound/soc/tegra/tegra210_mvc.c +++ b/sound/soc/tegra/tegra210_mvc.c @@ -123,7 +123,42 @@ static int tegra210_mvc_get_mute(struct snd_kcontrol *kcontrol, mute_mask = (val >> TEGRA210_MVC_MUTE_SHIFT) & TEGRA210_MUTE_MASK_EN; - ucontrol->value.integer.value[0] = mute_mask; + if (strstr(kcontrol->id.name, "Per Chan Mute Mask")) { + /* + * If per channel control is enabled, then return + * exact mute/unmute setting of all channels. + * + * Else report setting based on CH0 bit to reflect + * the correct HW state. + */ + if (val & TEGRA210_MVC_PER_CHAN_CTRL_EN) { + ucontrol->value.integer.value[0] = mute_mask; + } else { + if (mute_mask & TEGRA210_MVC_CH0_MUTE_EN) + ucontrol->value.integer.value[0] = + TEGRA210_MUTE_MASK_EN; + else + ucontrol->value.integer.value[0] = 0; + } + } else { + /* + * If per channel control is disabled, then return + * master mute/unmute setting based on CH0 bit. + * + * Else report settings based on state of all + * channels. + */ + if (!(val & TEGRA210_MVC_PER_CHAN_CTRL_EN)) { + ucontrol->value.integer.value[0] = + mute_mask & TEGRA210_MVC_CH0_MUTE_EN; + } else { + if (mute_mask == TEGRA210_MUTE_MASK_EN) + ucontrol->value.integer.value[0] = + TEGRA210_MVC_CH0_MUTE_EN; + else + ucontrol->value.integer.value[0] = 0; + } + } return 0; } @@ -136,6 +171,7 @@ static int tegra210_mvc_put_mute(struct snd_kcontrol *kcontrol, struct snd_soc_component *cmpnt = snd_soc_kcontrol_component(kcontrol); struct tegra210_mvc *mvc = snd_soc_component_get_drvdata(cmpnt); unsigned int value; + u32 reg_mask; u8 mute_mask; int err; @@ -150,11 +186,22 @@ static int tegra210_mvc_put_mute(struct snd_kcontrol *kcontrol, mute_mask = ucontrol->value.integer.value[0]; - err = regmap_update_bits(mvc->regmap, mc->reg, - TEGRA210_MVC_MUTE_MASK, - mute_mask << TEGRA210_MVC_MUTE_SHIFT); - if (err < 0) - goto end; + if (strstr(kcontrol->id.name, "Per Chan Mute Mask")) { + regmap_update_bits(mvc->regmap, TEGRA210_MVC_CTRL, + TEGRA210_MVC_PER_CHAN_CTRL_EN_MASK, + TEGRA210_MVC_PER_CHAN_CTRL_EN); + + reg_mask = TEGRA210_MVC_MUTE_MASK; + } else { + regmap_update_bits(mvc->regmap, TEGRA210_MVC_CTRL, + TEGRA210_MVC_PER_CHAN_CTRL_EN_MASK, + 0); + + reg_mask = TEGRA210_MVC_CH0_MUTE_MASK; + } + + regmap_update_bits(mvc->regmap, mc->reg, reg_mask, + mute_mask << TEGRA210_MVC_MUTE_SHIFT); return 1; @@ -212,11 +259,31 @@ static int tegra210_mvc_put_vol(struct snd_kcontrol *kcontrol, ucontrol->value.integer.value[0]); /* Configure init volume same as target volume */ - regmap_write(mvc->regmap, - TEGRA210_MVC_REG_OFFSET(TEGRA210_MVC_INIT_VOL, chan), - mvc->volume[chan]); + if (strstr(kcontrol->id.name, "Channel")) { + regmap_update_bits(mvc->regmap, TEGRA210_MVC_CTRL, + TEGRA210_MVC_PER_CHAN_CTRL_EN_MASK, + TEGRA210_MVC_PER_CHAN_CTRL_EN); + + regmap_write(mvc->regmap, + TEGRA210_MVC_REG_OFFSET(TEGRA210_MVC_INIT_VOL, chan), + mvc->volume[chan]); + + regmap_write(mvc->regmap, reg, mvc->volume[chan]); + } else { + int i; + + regmap_update_bits(mvc->regmap, TEGRA210_MVC_CTRL, + TEGRA210_MVC_PER_CHAN_CTRL_EN_MASK, + 0); + + for (i = 1; i < TEGRA210_MVC_MAX_CHAN_COUNT; i++) + mvc->volume[i] = mvc->volume[0]; - regmap_write(mvc->regmap, reg, mvc->volume[chan]); + regmap_write(mvc->regmap, TEGRA210_MVC_INIT_VOL, + mvc->volume[0]); + + regmap_write(mvc->regmap, reg, mvc->volume[0]); + } regmap_update_bits(mvc->regmap, TEGRA210_MVC_SWITCH, TEGRA210_MVC_VOLUME_SWITCH_MASK, @@ -422,6 +489,14 @@ static const struct snd_kcontrol_new tegra210_mvc_vol_ctrl[] = { TEGRA210_MVC_CTRL, 0, TEGRA210_MUTE_MASK_EN, 0, tegra210_mvc_get_mute, tegra210_mvc_put_mute), + /* Master volume */ + SOC_SINGLE_EXT("Volume", TEGRA210_MVC_TARGET_VOL, 0, 16000, 0, + tegra210_mvc_get_vol, tegra210_mvc_put_vol), + + /* Master mute */ + SOC_SINGLE_EXT("Mute", TEGRA210_MVC_CTRL, 0, 1, 0, + tegra210_mvc_get_mute, tegra210_mvc_put_mute), + SOC_ENUM_EXT("Curve Type", tegra210_mvc_curve_type_ctrl, tegra210_mvc_get_curve_type, tegra210_mvc_put_curve_type), }; diff --git a/sound/soc/tegra/tegra210_mvc.h b/sound/soc/tegra/tegra210_mvc.h index def29c4..7f2567e 100644 --- a/sound/soc/tegra/tegra210_mvc.h +++ b/sound/soc/tegra/tegra210_mvc.h @@ -59,6 +59,8 @@ #define TEGRA210_MUTE_MASK_EN 0xff #define TEGRA210_MVC_MUTE_MASK (TEGRA210_MUTE_MASK_EN << TEGRA210_MVC_MUTE_SHIFT) #define TEGRA210_MVC_MUTE_EN (TEGRA210_MUTE_MASK_EN << TEGRA210_MVC_MUTE_SHIFT) +#define TEGRA210_MVC_CH0_MUTE_EN 1 +#define TEGRA210_MVC_CH0_MUTE_MASK (TEGRA210_MVC_CH0_MUTE_EN << TEGRA210_MVC_MUTE_SHIFT) #define TEGRA210_MVC_PER_CHAN_CTRL_EN_SHIFT 30 #define TEGRA210_MVC_PER_CHAN_CTRL_EN_MASK (1 << TEGRA210_MVC_PER_CHAN_CTRL_EN_SHIFT)
The MVC module has a per channel control bit, based on which it decides to apply channel specific volume/mute settings. When per channel control bit is enabled (which is the default HW configuration), all MVC channel volume/mute can be independently controlled. If the control is disabled, channel-0 volume/mute setting is applied by HW to all remaining channels. Thus add support to leverage this HW feature by exposing master controls for volume/mute. With this, now there are per channel and master volume/mute controls. Users need to just use controls which are suitable for their applications. The per channel control enable/disable is mananged in driver and hidden from users, so that they need to just worry about respective volume/mute controls. Signed-off-by: Sameer Pujar <spujar@nvidia.com> --- sound/soc/tegra/tegra210_mvc.c | 95 +++++++++++++++++++++++++++++++++++++----- sound/soc/tegra/tegra210_mvc.h | 2 + 2 files changed, 87 insertions(+), 10 deletions(-)