On Thu, Aug 26, 2021 at 04:09:37PM +0800, derek.fang@xxxxxxxxxxx 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?
Attachment:
signature.asc
Description: PGP signature
