RE: [PATCH] ASoC: rt1318: Add RT1318 audio amplifier driver

[Jack Yu <jack.yu@realtek.com>]

> -----Original Message-----
> From: Mark Brown <broonie@kernel.org>
> Sent: Tuesday, June 4, 2024 8:38 PM
> To: Jack Yu <jack.yu@realtek.com>
> Cc: lgirdwood@gmail.com; alsa-devel@alsa-project.org; lars@metafoo.de;
> Flove(HsinFu) <flove@realtek.com>; Oder Chiou <oder_chiou@realtek.com>;
> Shuming [范書銘] <shumingf@realtek.com>; Derek [方德義]
> <derek.fang@realtek.com>
> Subject: Re: [PATCH] ASoC: rt1318: Add RT1318 audio amplifier driver
> 
> On Tue, Jun 04, 2024 at 06:17:02AM +0000, Jack Yu wrote:
> 
> > +static int rt1318_init_put(struct snd_kcontrol *kcontrol,
> > +		struct snd_ctl_elem_value *ucontrol) {
> > +	struct snd_soc_component *component =
> snd_soc_kcontrol_component(kcontrol);
> > +	struct rt1318_priv *rt1318 =
> > +snd_soc_component_get_drvdata(component);
> > +
> > +	rt1318->rt1318_init = ucontrol->value.integer.value[0];
> > +
> > +	if (rt1318->rt1318_init)
> > +		rt1318_reg_init(component);
> > +
> > +	return 0;
> > +}
> 
> So this control resets the CODEC - what's the story here?  Really it should be a
> boolean, and if you run mixer-test it'll tell you you're not returning 1 for value
> changes - please run mixer-test, there are a number of other errors that it will
> detect here.
> 
I will remove this control since it's only for internal testing purpose.

> > +static int rt1318_dvol_put(struct snd_kcontrol *kcontrol,
> > +		struct snd_ctl_elem_value *ucontrol) {
> > +	struct snd_soc_component *component =
> snd_soc_kcontrol_component(kcontrol);
> > +	struct rt1318_priv *rt1318 =
> > +snd_soc_component_get_drvdata(component);
> > +
> > +	rt1318->rt1318_dvol = ucontrol->value.integer.value[0];
> > +
> > +	if (rt1318->rt1318_dvol <= RT1318_DVOL_STEP) {
> > +		regmap_write(rt1318->regmap, RT1318_DA_VOL_L_8,
> rt1318->rt1318_dvol >> 8);
> > +		regmap_write(rt1318->regmap, RT1318_DA_VOL_L_1_7,
> rt1318->rt1318_dvol & 0xff);
> > +		regmap_write(rt1318->regmap, RT1318_DA_VOL_R_8,
> rt1318->rt1318_dvol >> 8);
> > +		regmap_write(rt1318->regmap, RT1318_DA_VOL_R_1_7,
> > +rt1318->rt1318_dvol & 0xff);
> 
> This will happily accept negative values which you probably don't want.

I'll do modification regarding to it.

> 
> > +	} else {
> > +		rt1318->rt1318_dvol = RT1318_DVOL_STEP;
> > +		dev_err(component->dev, "Unsupported volume gain\n");
> 
> The driver shouldn't allow userspace to spam the kernel log like this, it can be
> used to mask issues.
> 
I'll remove this "else" condition.

> > +static const struct snd_kcontrol_new rt1318_snd_controls[] = {
> > +	SOC_SINGLE_EXT("rt1318 digital volume", SND_SOC_NOPM, 0, 383, 0,
> > +		rt1318_dvol_get, rt1318_dvol_put),
> 
> No need to include the part number in controls, users aren't going to care.
> The general style for ALSA controls is capitalised too.
Will remove part number.