Re: [PATCH 2/4] ASoC: codecs: Add support for Richtek rt9123

[Mark Brown]

On Fri, Apr 04, 2025 at 10:22:12PM +0800, cy_huang@xxxxxxxxxxx wrote:

> +static int rt9123_enable_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol,
> +			       int event)
> +{

> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	snd_soc_component_write_field(comp, RT9123_REG_AMPCTRL, RT9123_MASK_AMPON, enable);
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);

What's going on with the runtime PM stuff here?  Especially for the DAPM
widget usually the ASoC core will be able to keep devices runtime PM
enabled so long as they are in use so I'd expect this not to have any
impact.  Why not just use a normal DAPM widget?

> +static int rt9123_xhandler_get(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *comp = snd_soc_kcontrol_component(kcontrol);
> +	struct device *dev = comp->dev;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	if (rt9123_kcontrol_name_comp(kcontrol, "SPK Gain Volume") == 0)
> +		ret = snd_soc_get_volsw(kcontrol, ucontrol);
> +	else
> +		ret = snd_soc_get_enum_double(kcontrol, ucontrol);

This is even more unusual - it'll runtime PM enable the device every
time we write to a control, even if the device is idle.  The driver does
implement a register cache so it's especially confusing, we'll power up
the device, resync the cache, write to the hardware then power the
device off again.  Usually you'd just use the standard operations and
then let the register writes get synced to the cache whenever it gets
enabled for actual use.  Again, why not just use standard controls?

> +static const struct snd_kcontrol_new rt9123_controls[] = {
> +	SOC_SINGLE_TLV("Master Volume", RT9123_REG_VOLGAIN, 2, 511, 1, dig_tlv),
> +	SOC_SINGLE_EXT_TLV("SPK Gain Volume", RT9123_REG_AMPCTRL, 0, 10, 0, rt9123_xhandler_get,
> +			   rt9123_xhandler_put, ana_tlv),

Speaker Volume.

> +static const struct regmap_config rt9123_regmap_config = {
> +	.name			= "rt9123",
> +	.reg_bits		= 8,
> +	.val_bits		= 16,
> +	.val_format_endian	= REGMAP_ENDIAN_BIG,
> +	.readable_reg		= rt9123_readable_reg,
> +	.writeable_reg		= rt9123_writeable_reg,
> +	.volatile_reg		= rt9123_volatile_reg,
> +	.cache_type		= REGCACHE_RBTREE,
> +	.num_reg_defaults_raw	= RT9123_REG_COMBOID + 1,
> +};

Generally _MAPLE is a better cache type for most devices - unless you
have a strong reason to use _RBTREE it's preferred.

> +	/* Trigger RG reset before regmap init cache */
> +	ret = i2c_smbus_write_word_data(i2c, RT9123_REG_AMPCTRL, RT9123_MASK_SWRST);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to trigger RG reset\n");
> +
> +	regmap = devm_regmap_init_i2c(i2c, &rt9123_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap), "Failed to init regmap\n");
> +
> +	ret = regmap_read(regmap, RT9123_REG_COMBOID, &venid);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read vendor-id\n");
> +
> +	if ((venid & RT9123_MASK_VENID) != RT9123_FIXED_VENID)
> +		return dev_err_probe(dev, -ENODEV, "Incorrect vendor-id 0x%04x\n", venid);

It seems nicer to verify the device ID before doing the reset in case
anything went wrong.  Who knows what some other device did with the
reset?

Attachment: signature.asc
Description: PGP signature