diff mbox series

[04/14] Removed the usage control and add the power switch on DAPM route.

Message ID 20230106091543.2440-5-kiseok.jo@irondevice.com (mailing list archive)
State New, archived
Headers show
Series ASoC: Add a driver the Iron Device SMA1303 AMP | expand

Commit Message

Ki-Seok Jo Jan. 6, 2023, 9:15 a.m. UTC 
Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com>
Reported-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/codecs/sma1303.c | 79 ++++----------------------------------
 1 file changed, 8 insertions(+), 71 deletions(-)

Comments

Mark Brown Jan. 6, 2023, 1:59 p.m. UTC | #1 
On Fri, Jan 06, 2023 at 06:15:33PM +0900, Kiseok Jo wrote:
> Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com>
> Reported-by: Mark Brown <broonie@kernel.org>
> ---
>  sound/soc/codecs/sma1303.c | 79 ++++----------------------------------
>  1 file changed, 8 insertions(+), 71 deletions(-)

This and most of your other followup patches should be part of patch 1 -
normally you shouldn't introduce problems then fix them as part of the
same patch series, this makes it very hard to review things since people
will see the problems earlier on and perhaps not get as far as seeing
the fix.  Once the code is merged changes need to be incremental but
until then you should fix the patch itself.
Ki-Seok Jo Jan. 9, 2023, 9:33 a.m. UTC | #2 
> On Fri, Jan 06, 2023 at 06:15:33PM +0900, Kiseok Jo wrote:
> > Signed-off-by: Kiseok Jo <kiseok.jo@irondevice.com>
> > Reported-by: Mark Brown <broonie@kernel.org>
> > ---
> >  sound/soc/codecs/sma1303.c | 79 
> > ++++----------------------------------
> >  1 file changed, 8 insertions(+), 71 deletions(-)

> This and most of your other followup patches should be part of patch 1 - normally you shouldn't introduce problems then fix them as part of the same patch series, this makes it very hard to review things since people will see the problems earlier on and perhaps not get as far as seeing the fix.  Once the code is merged changes need to be incremental but until then you should fix the patch itself.

Thank you for your reply.

I misunderstood what you say. I haven't registered the new driver code on the kernel.
So I think it's not new patch. So I resend the mail updating patch version applied your feedback.
(Because this patch's topic is 'Add driver')

If the driver is registered and there are any changes, I'll make a new patch for each changes.
And the patches doesn't make same patch series.

I already resent the patch mail for registering a new driver, please check if there is a problem.

Thank you.

Best regards,
Kiseok Jo
diff mbox series

Patch

diff --git a/sound/soc/codecs/sma1303.c b/sound/soc/codecs/sma1303.c
index 8bd59a481f2d..f78cd2daad61 100644
--- a/sound/soc/codecs/sma1303.c
+++ b/sound/soc/codecs/sma1303.c
@@ -69,7 +69,6 @@  struct sma1303_priv {
 	struct mutex lock;
 	struct delayed_work check_fault_work;
 	bool amp_power_status;
-	bool usage_status;
 	int num_of_pll_matches;
 	int retry_cnt;
 	unsigned int sys_clk_id;
@@ -375,66 +374,6 @@  static int bytes_ext_put(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
-static int amp_usage_status_get(struct snd_kcontrol *kcontrol,
-		struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *component =
-		snd_soc_kcontrol_component(kcontrol);
-	struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
-	int ret = -1;
-
-	if (component == NULL) {
-		pr_err("%s:component is NULL\n", __func__);
-		return ret;
-	}
-	if (sma1303 == NULL) {
-		pr_err("%s:sma1303 is NULL\n", __func__);
-		return ret;
-	}
-	ucontrol->value.integer.value[0] = sma1303->usage_status;
-
-	if (sma1303->usage_status)
-		dev_info(component->dev, "Amplifier Power Control Enabled\n");
-	else
-		dev_info(component->dev, "Amplifier Power Control Disabled\n");
-
-	return 0;
-}
-
-static int amp_usage_status_put(struct snd_kcontrol *kcontrol,
-		struct snd_ctl_elem_value *ucontrol)
-{
-	struct snd_soc_component *component =
-		snd_soc_kcontrol_component(kcontrol);
-	struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
-	int ret = -1;
-
-	if (component == NULL) {
-		pr_err("%s:component is NULL\n", __func__);
-		return ret;
-	}
-	if (sma1303 == NULL) {
-		pr_err("%s:sma1303 is NULL\n", __func__);
-		return ret;
-	}
-
-	if ((sma1303->usage_status
-			!= ucontrol->value.integer.value[0])
-			&& (!ucontrol->value.integer.value[0])) {
-		dev_info(component->dev, "%s\n", "Force AMP Power Down");
-		ret = sma1303_shutdown(component);
-		if (ret < 0) {
-			ucontrol->value.integer.value[0]
-			       = sma1303->usage_status;
-			return ret;
-		}
-
-	}
-	sma1303->usage_status = ucontrol->value.integer.value[0];
-
-	return 0;
-}
-
 static const char * const sma1303_amp_mode_text[] = {
 	"1 Chip", "Mono on 2 chips", "Left in 2 chips", "Right in 2chips"};
 
@@ -928,10 +867,10 @@  static SOC_VALUE_ENUM_SINGLE_DECL(sma1303_sdo_source_enum,
 		sma1303_sdo_source_values);
 static const struct snd_kcontrol_new sma1303_sdo_source_mux =
 	SOC_DAPM_ENUM("SDO Source", sma1303_sdo_source_enum);
+static const struct snd_kcontrol_new sma1303_enable_control =
+	SOC_DAPM_SINGLE_VIRT("Switch", 1);
 
 static const struct snd_kcontrol_new sma1303_snd_controls[] = {
-SOC_SINGLE_EXT("Amplifier Usage", SND_SOC_NOPM, 0, 1, 0,
-	amp_usage_status_get, amp_usage_status_put),
 SOC_ENUM_EXT("Amplifier Mode", sma1303_amp_mode_enum,
 	sma1303_amp_mode_get, sma1303_amp_mode_put),
 SOC_ENUM_EXT("Outport config", sma1303_outport_config_enum,
@@ -1048,14 +987,12 @@  static int sma1303_dac_event(struct snd_soc_dapm_widget *w,
 {
 	struct snd_soc_component *component =
 		snd_soc_dapm_to_component(w->dapm);
-	struct sma1303_priv *sma1303 = snd_soc_component_get_drvdata(component);
 
 	switch (event) {
 	case SND_SOC_DAPM_PRE_PMU:
 		dev_info(component->dev, "%s : PRE_PMU\n", __func__);
 
-		if (sma1303->usage_status)
-			sma1303_startup(component);
+		sma1303_startup(component);
 
 		break;
 
@@ -1130,11 +1067,13 @@  SND_SOC_DAPM_ADC_E("DAC_FEEDBACK", "Capture", SND_SOC_NOPM, 0, 0,
 SND_SOC_DAPM_OUTPUT("SPK"),
 SND_SOC_DAPM_INPUT("SDO"),
 SND_SOC_DAPM_MUX("SDO Source", SND_SOC_NOPM, 0, 0, &sma1303_sdo_source_mux),
+SND_SOC_DAPM_SWITCH("AMP Enable", SND_SOC_NOPM, 0, 1, &sma1303_enable_control),
 };
 
 static const struct snd_soc_dapm_route sma1303_audio_map[] = {
 {"DAC", NULL, "CLK_SUPPLY"},
-{"SPK", NULL, "DAC"},
+{"AMP Enable", "Switch", "DAC"},
+{"SPK", NULL, "AMP Enable"},
 {"SDO Source", "Disable", "SDO"},
 {"SDO Source", "Format_C", "SDO"},
 {"SDO Source", "Mixer_Out", "SDO"},
@@ -1214,9 +1153,8 @@  static int sma1303_dai_hw_params_amp(struct snd_pcm_substream *substream,
 
 	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
 
-		if (sma1303->usage_status &&
-			(sma1303->sys_clk_id == SMA1303_PLL_CLKIN_MCLK
-			|| sma1303->sys_clk_id == SMA1303_PLL_CLKIN_BCLK)) {
+		if (sma1303->sys_clk_id == SMA1303_PLL_CLKIN_MCLK
+			|| sma1303->sys_clk_id == SMA1303_PLL_CLKIN_BCLK) {
 
 			if (sma1303->last_bclk != bclk) {
 				if (sma1303->amp_power_status) {
@@ -2039,7 +1977,6 @@  static int sma1303_i2c_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, sma1303);
 
 	sma1303->amp_mode = ONE_CHIP_SOLUTION;
-	sma1303->usage_status = true;
 	sma1303->amp_power_status = false;
 	sma1303->check_fault_status = true;
 	sma1303->pll_matches = sma1303_pll_matches;