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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
