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

[Srinivas Kandagatla]

Thanks for the review,

On 19/12/2019 13:04, Mark Brown wrote:
> 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...
> 
I agree, I will add some error reporting if it fails to set the 
requested volume.
>> +
>> +static struct reg_default wsa881x_defaults[] = {
>> +	{WSA881X_CHIP_ID0, 0x00},
> 
> Spaces around the { } please.
> 
Also will fix such instances.

>> +	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.
Sure will fix all such instances in the code.

> 
>> +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.

I will add some error prints in case it fails to program the requested 
volume.

> 
>> +			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.
makes sense!

--srini

>