[alsa-devel] [PATCH v9 2/2] ASoC: codecs: add wsa881x amplifier support

[Mark Brown]

On Thu, Dec 19, 2019 at 10:03:28AM +0000, Srinivas Kandagatla wrote:

> +#define WSA881X_PA_GAIN_TLV(xname, reg, shift, max, invert, tlv_array) \
> +{	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, .name = xname, \
> +	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
> +		 SNDRV_CTL_ELEM_ACCESS_READWRITE,\
> +	.tlv.p = (tlv_array), \
> +	.info = snd_soc_info_volsw, .get = snd_soc_get_volsw,\
> +	.put = wsa881x_put_pa_gain, \
> +	.private_value = SOC_SINGLE_VALUE(reg, shift, max, invert, 0) }

The get operation isn't going to work here...

> +
> +static struct reg_default wsa881x_defaults[] = {
> +	{WSA881X_CHIP_ID0, 0x00},

Spaces around the { } please.

> +	unsigned int val = 0;
> +
> +	regmap_read(rm, WSA881X_CHIP_ID1, &wsa881x->version);
> +	regmap_register_patch(wsa881x->regmap, wsa881x_rev_2_0,
> +			      ARRAY_SIZE(wsa881x_rev_2_0));
> +	/* Enable software reset output from soundwire slave */
> +	regmap_update_bits(rm, WSA881X_SWR_RESET_EN, 0x07, 0x07);
> +	/* Bring out of analog reset */
> +	regmap_update_bits(rm, WSA881X_CDC_RST_CTL, 0x02, 0x02);
> +	/* Bring out of digital reset */
> +	regmap_update_bits(rm, WSA881X_CDC_RST_CTL, 0x01, 0x01);

Please add some blank lines before the comments for legiblity.

> +static int wsa881x_put_pa_gain(struct snd_kcontrol *kc,
> +			       struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *comp = snd_soc_kcontrol_component(kc);
> +	struct wsa881x_priv *wsa881x = snd_soc_component_get_drvdata(comp);
> +	struct soc_mixer_control *mc =
> +			(struct soc_mixer_control *)kc->private_value;
> +	int max = mc->max;
> +	unsigned int mask = (1 << fls(max)) - 1;
> +
> +	/*
> +	 * program actual register just before compander enable and ensure hw
> +	 * sequence is followed
> +	 */
> +	wsa881x->pa_gain = (max - ucontrol->value.integer.value[0]) & mask;
> +
> +	return 0;
> +}

...this doesn't actually write the value to the register map but we're
using the standard get operation to read the value so the readback will
not show the new value until it happens to get written out to the chip.
This will also silently ignore volume updates until whatever event
triggers the write, I'd expect at least an error if we can't do a write
while the relevant part of the chip is active.

> +			usleep_range(1000, 1010);
> +			wsa881x_ramp_pa_gain(comp, min_gain, max_gain);

The ramp function has exactly one user, may as well just inline it.
-------------- 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/20191219/a9812057/attachment-0001.sig>