* [PATCH] ASoC: tegra: Add master volume/mute control support
@ 2021-10-25 11:06 ` Sameer Pujar
0 siblings, 0 replies; 14+ messages in thread
From: Sameer Pujar @ 2021-10-25 11:06 UTC (permalink / raw)
To: broonie, lgirdwood, perex, tiwai
Cc: jonathanh, thierry.reding, alsa-devel, linux-tegra, linux-kernel,
Sameer Pujar
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(-)
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)
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH] ASoC: tegra: Add master volume/mute control support
@ 2021-10-25 11:06 ` Sameer Pujar
0 siblings, 0 replies; 14+ messages in thread
From: Sameer Pujar @ 2021-10-25 11:06 UTC (permalink / raw)
To: broonie, lgirdwood, perex, tiwai
Cc: alsa-devel, Sameer Pujar, linux-kernel, jonathanh,
thierry.reding, linux-tegra
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(-)
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)
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
2021-10-25 11:06 ` Sameer Pujar
@ 2021-10-25 12:58 ` Jaroslav Kysela
-1 siblings, 0 replies; 14+ messages in thread
From: Jaroslav Kysela @ 2021-10-25 12:58 UTC (permalink / raw)
To: Sameer Pujar, broonie, lgirdwood, tiwai
Cc: jonathanh, thierry.reding, alsa-devel, linux-tegra, linux-kernel
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
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
@ 2021-10-25 12:58 ` Jaroslav Kysela
0 siblings, 0 replies; 14+ messages in thread
From: Jaroslav Kysela @ 2021-10-25 12:58 UTC (permalink / raw)
To: Sameer Pujar, broonie, lgirdwood, tiwai
Cc: linux-tegra, alsa-devel, thierry.reding, linux-kernel, jonathanh
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
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
2021-10-25 12:58 ` Jaroslav Kysela
@ 2021-10-26 6:23 ` Sameer Pujar
-1 siblings, 0 replies; 14+ messages in thread
From: Sameer Pujar @ 2021-10-26 6:23 UTC (permalink / raw)
To: Jaroslav Kysela, broonie, lgirdwood, tiwai
Cc: jonathanh, thierry.reding, alsa-devel, linux-tegra, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
@ 2021-10-26 6:23 ` Sameer Pujar
0 siblings, 0 replies; 14+ messages in thread
From: Sameer Pujar @ 2021-10-26 6:23 UTC (permalink / raw)
To: Jaroslav Kysela, broonie, lgirdwood, tiwai
Cc: linux-tegra, alsa-devel, thierry.reding, linux-kernel, jonathanh
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
2021-10-26 6:23 ` Sameer Pujar
@ 2021-10-29 15:08 ` Sameer Pujar
-1 siblings, 0 replies; 14+ messages in thread
From: Sameer Pujar @ 2021-10-29 15:08 UTC (permalink / raw)
To: Jaroslav Kysela, broonie, lgirdwood, tiwai
Cc: jonathanh, thierry.reding, alsa-devel, linux-tegra, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
@ 2021-10-29 15:08 ` Sameer Pujar
0 siblings, 0 replies; 14+ messages in thread
From: Sameer Pujar @ 2021-10-29 15:08 UTC (permalink / raw)
To: Jaroslav Kysela, broonie, lgirdwood, tiwai
Cc: linux-tegra, alsa-devel, thierry.reding, linux-kernel, jonathanh
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
2021-10-29 15:08 ` Sameer Pujar
@ 2021-10-29 15:22 ` Mark Brown
-1 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2021-10-29 15:22 UTC (permalink / raw)
To: Sameer Pujar
Cc: Jaroslav Kysela, lgirdwood, tiwai, jonathanh, thierry.reding,
alsa-devel, linux-tegra, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
@ 2021-10-29 15:22 ` Mark Brown
0 siblings, 0 replies; 14+ messages in thread
From: Mark Brown @ 2021-10-29 15:22 UTC (permalink / raw)
To: Sameer Pujar
Cc: alsa-devel, lgirdwood, linux-kernel, tiwai, jonathanh,
thierry.reding, linux-tegra
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
2021-10-29 15:08 ` Sameer Pujar
@ 2021-10-29 15:26 ` Jaroslav Kysela
-1 siblings, 0 replies; 14+ messages in thread
From: Jaroslav Kysela @ 2021-10-29 15:26 UTC (permalink / raw)
To: Sameer Pujar, broonie, lgirdwood, tiwai
Cc: jonathanh, thierry.reding, alsa-devel, linux-tegra, linux-kernel
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
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
@ 2021-10-29 15:26 ` Jaroslav Kysela
0 siblings, 0 replies; 14+ messages in thread
From: Jaroslav Kysela @ 2021-10-29 15:26 UTC (permalink / raw)
To: Sameer Pujar, broonie, lgirdwood, tiwai
Cc: linux-tegra, alsa-devel, thierry.reding, linux-kernel, jonathanh
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
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
2021-10-29 15:26 ` Jaroslav Kysela
@ 2021-11-03 5:12 ` Sameer Pujar
-1 siblings, 0 replies; 14+ messages in thread
From: Sameer Pujar @ 2021-11-03 5:12 UTC (permalink / raw)
To: Jaroslav Kysela, broonie, lgirdwood, tiwai
Cc: jonathanh, thierry.reding, alsa-devel, linux-tegra, linux-kernel
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] ASoC: tegra: Add master volume/mute control support
@ 2021-11-03 5:12 ` Sameer Pujar
0 siblings, 0 replies; 14+ messages in thread
From: Sameer Pujar @ 2021-11-03 5:12 UTC (permalink / raw)
To: Jaroslav Kysela, broonie, lgirdwood, tiwai
Cc: linux-tegra, alsa-devel, thierry.reding, linux-kernel, jonathanh
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-11-03 5:14 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25 11:06 [PATCH] ASoC: tegra: Add master volume/mute control support Sameer Pujar
2021-10-25 11:06 ` Sameer Pujar
2021-10-25 12:58 ` Jaroslav Kysela
2021-10-25 12:58 ` Jaroslav Kysela
2021-10-26 6:23 ` Sameer Pujar
2021-10-26 6:23 ` Sameer Pujar
2021-10-29 15:08 ` Sameer Pujar
2021-10-29 15:08 ` Sameer Pujar
2021-10-29 15:22 ` Mark Brown
2021-10-29 15:22 ` Mark Brown
2021-10-29 15:26 ` Jaroslav Kysela
2021-10-29 15:26 ` Jaroslav Kysela
2021-11-03 5:12 ` Sameer Pujar
2021-11-03 5:12 ` Sameer Pujar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.