On Thu, Aug 26, 2021 at 04:09:37PM +0800, derek.fang@realtek.com wrote: This looks good, a few small mostly style comments below but nothing substantial:
+static int rt5682s_sar_power_mode(struct snd_soc_component *component, + int mode, int jd_step) +{ + struct rt5682s_priv *rt5682s = + snd_soc_component_get_drvdata(component); + + mutex_lock(&rt5682s->sar_mutex); + + switch (mode) { + case SAR_PWR_SAVING: + snd_soc_component_update_bits(component, RT5682S_CBJ_CTRL_3, + RT5682S_CBJ_IN_BUF_MASK, RT5682S_CBJ_IN_BUF_DIS);
+ default: + break; + }
Shouldn't there be some sort of warning or error if we get an invalid mode here?
+static void rt5682s_enable_push_button_irq(struct snd_soc_component *component, + bool enable) +{ + if (enable) { + snd_soc_component_update_bits(component, RT5682S_SAR_IL_CMD_13, + RT5682S_SAR_SOUR_MASK, RT5682S_SAR_SOUR_BTN); + snd_soc_component_write(component, RT5682S_IL_CMD_1, 0x0040); + snd_soc_component_update_bits(component, RT5682S_4BTN_IL_CMD_2, + RT5682S_4BTN_IL_MASK | RT5682S_4BTN_IL_RST_MASK, + RT5682S_4BTN_IL_EN | RT5682S_4BTN_IL_NOR); + snd_soc_component_update_bits(component, RT5682S_IRQ_CTRL_3, + RT5682S_IL_IRQ_MASK, RT5682S_IL_IRQ_EN); + } else {
Why not just have separate enable and disable functions, there's no shared code here and it avoids the boolean argument which tends to be unclear?
+static const char * const rt5682s_sar_mode[] = { + "Normal", "Saving" +}; + +static SOC_ENUM_SINGLE_DECL(rt5682s_sar_mode_enum, 0, 0, + rt5682s_sar_mode);
Might be easier to make this a "SAR Power Saving Switch"? Doesn't really matter.
+static int rt5682s_probe(struct snd_soc_component *component) +{ + struct rt5682s_priv *rt5682s = snd_soc_component_get_drvdata(component); + struct snd_soc_dapm_context *dapm = &component->dapm; + +#ifdef CONFIG_COMMON_CLK + int ret; +#endif + rt5682s->component = component; + +#ifdef CONFIG_COMMON_CLK + /* Check if MCLK provided */ + rt5682s->mclk = devm_clk_get(component->dev, "mclk");
Perhaps factor the clock init out into a _probe_clks() function and then have a stub function rather than the two ifdefs?