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?