[PATCH 1/2] ASoC: rt5682s: Add driver for ALC5682I-VS codec

[Mark Brown]

On Thu, Aug 26, 2021 at 04:09:37PM +0800, derek.fang at 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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20210826/8298731f/attachment.sig>