| Message ID | B9E728AE-223C-4BA7-961A-ED107AF03407@latencyzero.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Rick Mann, On Tue, Sep 29, 2015 at 12:38 PM, Rick Mann <rmann@latencyzero.com> wrote: > Hi. I came across some bugs in sound/soc/codecs/tlv320aic3x.c, wherein it writes non-zero values to reserved registers on the tlv320aic3104 (I found it in 4.1.4, but it's still in 4.3-rc3). I have an untested proposed patch. But I don't know anything about how to report it or offer it. > > What should I do? Thanks. Follow this: https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=refs/tags/v4.3-rc3 > For reference: > > $ git diff -- sound/soc/codecs/tlv320aic3x.c > diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c > index 51c4713..79c0ca0 100644 > --- a/sound/soc/codecs/tlv320aic3x.c > +++ b/sound/soc/codecs/tlv320aic3x.c > @@ -1509,14 +1509,24 @@ static int aic3x_init(struct snd_soc_codec *codec) > snd_soc_write(codec, PGAL_2_LLOPM_VOL, DEFAULT_VOL); > snd_soc_write(codec, PGAR_2_RLOPM_VOL, DEFAULT_VOL); > > - /* Line2 to HP Bypass default volume, disconnect from Output Mixer */ > - snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, DEFAULT_VOL); > - snd_soc_write(codec, LINE2R_2_HPROUT_VOL, DEFAULT_VOL); > - snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, DEFAULT_VOL); > - snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, DEFAULT_VOL); > - /* Line2 Line Out default volume, disconnect from Output Mixer */ > - snd_soc_write(codec, LINE2L_2_LLOPM_VOL, DEFAULT_VOL); > - snd_soc_write(codec, LINE2R_2_RLOPM_VOL, DEFAULT_VOL); > + if (aic3x->model == AIC3X_MODEL_3104) { > + /* On tlv320aic3014, these registers are reserved and must be written 0 */ > + snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, 0); > + snd_soc_write(codec, LINE2R_2_HPROUT_VOL, 0); > + snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, 0); > + snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, 0); > + snd_soc_write(codec, LINE2L_2_LLOPM_VOL, 0); > + snd_soc_write(codec, LINE2R_2_RLOPM_VOL, 0); Nothing should be done for these registers on 3104, not even setting them to 0. The datasheet says "Do not write to this register.". > + } else { > + /* Line2 to HP Bypass default volume, disconnect from Output Mixer */ > + snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, DEFAULT_VOL); > + snd_soc_write(codec, LINE2R_2_HPROUT_VOL, DEFAULT_VOL); > + snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, DEFAULT_VOL); > + snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, DEFAULT_VOL); > + /* Line2 Line Out default volume, disconnect from Output Mixer */ > + snd_soc_write(codec, LINE2L_2_LLOPM_VOL, DEFAULT_VOL); > + snd_soc_write(codec, LINE2R_2_RLOPM_VOL, DEFAULT_VOL); > + } Correct. > > switch (aic3x->model) { > case AIC3X_MODEL_3X: > > Best regards, Benoît
> On Sep 29, 2015, at 11:47 , Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com> wrote: > > Dear Rick Mann, > > On Tue, Sep 29, 2015 at 12:38 PM, Rick Mann <rmann@latencyzero.com> wrote: >> Hi. I came across some bugs in sound/soc/codecs/tlv320aic3x.c, wherein it writes non-zero values to reserved registers on the tlv320aic3104 (I found it in 4.1.4, but it's still in 4.3-rc3). I have an untested proposed patch. But I don't know anything about how to report it or offer it. >> >> What should I do? Thanks. > > Follow this: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=refs/tags/v4.3-rc3 Thanks, will do. > > Nothing should be done for these registers on 3104, not even setting > them to 0. The datasheet says "Do not write to this register.". Indeed, it does say that. I'll change the code. However, it also says not to write certain bits within a register, which is impossible (e.g. Table 88, Register 86, bit D2, "Reserved. Do not write to this register bit."). > >> + } else { >> + /* Line2 to HP Bypass default volume, disconnect from Output Mixer */ >> + snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, DEFAULT_VOL); >> + snd_soc_write(codec, LINE2R_2_HPROUT_VOL, DEFAULT_VOL); >> + snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, DEFAULT_VOL); >> + snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, DEFAULT_VOL); >> + /* Line2 Line Out default volume, disconnect from Output Mixer */ >> + snd_soc_write(codec, LINE2L_2_LLOPM_VOL, DEFAULT_VOL); >> + snd_soc_write(codec, LINE2R_2_RLOPM_VOL, DEFAULT_VOL); >> + } > > Correct. Thanks!
Dear Rick Mann, On Tue, Sep 29, 2015 at 9:21 PM, Rick Mann <rmann@latencyzero.com> wrote: >> On Sep 29, 2015, at 11:47 , Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com> wrote: >> On Tue, Sep 29, 2015 at 12:38 PM, Rick Mann <rmann@latencyzero.com> wrote: >> Nothing should be done for these registers on 3104, not even setting >> them to 0. The datasheet says "Do not write to this register.". > > Indeed, it does say that. I'll change the code. > > However, it also says not to write certain bits within a register, which is impossible (e.g. Table 88, Register 86, bit D2, "Reserved. Do not write to this register bit."). This is just a bug resulting from a copy/paste in the datasheet, but the idea is clear: do not touch reserved items when you can avoid doing so. This bit is even read-only besides being reserved, so writing it can't be harmful. The general rule for similar bits that are writable and reserved is either to write them to 0 or to leave them unchanged when writing the other bits of the register, depending on the chip and registers. Best regards, Benoît
Against what repo/branch should I submit this patch? broonie/sound/asoc-v4.3-rc2? > On Sep 29, 2015, at 11:47 , Benoît Thébaudeau <benoit.thebaudeau.dev@gmail.com> wrote: > > Dear Rick Mann, > > On Tue, Sep 29, 2015 at 12:38 PM, Rick Mann <rmann@latencyzero.com> wrote: >> Hi. I came across some bugs in sound/soc/codecs/tlv320aic3x.c, wherein it writes non-zero values to reserved registers on the tlv320aic3104 (I found it in 4.1.4, but it's still in 4.3-rc3). I have an untested proposed patch. But I don't know anything about how to report it or offer it. >> >> What should I do? Thanks. > > Follow this: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=refs/tags/v4.3-rc3 > >> For reference: >> >> $ git diff -- sound/soc/codecs/tlv320aic3x.c >> diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c >> index 51c4713..79c0ca0 100644 >> --- a/sound/soc/codecs/tlv320aic3x.c >> +++ b/sound/soc/codecs/tlv320aic3x.c >> @@ -1509,14 +1509,24 @@ static int aic3x_init(struct snd_soc_codec *codec) >> snd_soc_write(codec, PGAL_2_LLOPM_VOL, DEFAULT_VOL); >> snd_soc_write(codec, PGAR_2_RLOPM_VOL, DEFAULT_VOL); >> >> - /* Line2 to HP Bypass default volume, disconnect from Output Mixer */ >> - snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, DEFAULT_VOL); >> - snd_soc_write(codec, LINE2R_2_HPROUT_VOL, DEFAULT_VOL); >> - snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, DEFAULT_VOL); >> - snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, DEFAULT_VOL); >> - /* Line2 Line Out default volume, disconnect from Output Mixer */ >> - snd_soc_write(codec, LINE2L_2_LLOPM_VOL, DEFAULT_VOL); >> - snd_soc_write(codec, LINE2R_2_RLOPM_VOL, DEFAULT_VOL); >> + if (aic3x->model == AIC3X_MODEL_3104) { >> + /* On tlv320aic3014, these registers are reserved and must be written 0 */ >> + snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, 0); >> + snd_soc_write(codec, LINE2R_2_HPROUT_VOL, 0); >> + snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, 0); >> + snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, 0); >> + snd_soc_write(codec, LINE2L_2_LLOPM_VOL, 0); >> + snd_soc_write(codec, LINE2R_2_RLOPM_VOL, 0); > > Nothing should be done for these registers on 3104, not even setting > them to 0. The datasheet says "Do not write to this register.". > >> + } else { >> + /* Line2 to HP Bypass default volume, disconnect from Output Mixer */ >> + snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, DEFAULT_VOL); >> + snd_soc_write(codec, LINE2R_2_HPROUT_VOL, DEFAULT_VOL); >> + snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, DEFAULT_VOL); >> + snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, DEFAULT_VOL); >> + /* Line2 Line Out default volume, disconnect from Output Mixer */ >> + snd_soc_write(codec, LINE2L_2_LLOPM_VOL, DEFAULT_VOL); >> + snd_soc_write(codec, LINE2R_2_RLOPM_VOL, DEFAULT_VOL); >> + } > > Correct. > >> >> switch (aic3x->model) { >> case AIC3X_MODEL_3X: >> >> > > Best regards, > Benoît > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Dear Rick Mann,
On Wed, Sep 30, 2015 at 3:27 AM, Rick Mann <rmann@latencyzero.com> wrote:
> Against what repo/branch should I submit this patch? broonie/sound/asoc-v4.3-rc2?
The affected lines of this driver are probably the same in all the
latest trees, so it does not matter much. This branch seems to be the
best fit:
https://git.kernel.org/cgit/linux/kernel/git/broonie/sound.git/log/?h=for-next
Best regards,
Benoît
P.S.: Top-posting is highly discouraged on the mailing lists.
diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c index 51c4713..79c0ca0 100644 --- a/sound/soc/codecs/tlv320aic3x.c +++ b/sound/soc/codecs/tlv320aic3x.c @@ -1509,14 +1509,24 @@ static int aic3x_init(struct snd_soc_codec *codec) snd_soc_write(codec, PGAL_2_LLOPM_VOL, DEFAULT_VOL); snd_soc_write(codec, PGAR_2_RLOPM_VOL, DEFAULT_VOL); - /* Line2 to HP Bypass default volume, disconnect from Output Mixer */ - snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, DEFAULT_VOL); - snd_soc_write(codec, LINE2R_2_HPROUT_VOL, DEFAULT_VOL); - snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, DEFAULT_VOL); - snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, DEFAULT_VOL); - /* Line2 Line Out default volume, disconnect from Output Mixer */ - snd_soc_write(codec, LINE2L_2_LLOPM_VOL, DEFAULT_VOL); - snd_soc_write(codec, LINE2R_2_RLOPM_VOL, DEFAULT_VOL); + if (aic3x->model == AIC3X_MODEL_3104) { + /* On tlv320aic3014, these registers are reserved and must be written 0 */ + snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, 0); + snd_soc_write(codec, LINE2R_2_HPROUT_VOL, 0); + snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, 0); + snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, 0); + snd_soc_write(codec, LINE2L_2_LLOPM_VOL, 0); + snd_soc_write(codec, LINE2R_2_RLOPM_VOL, 0); + } else { + /* Line2 to HP Bypass default volume, disconnect from Output Mixer */ + snd_soc_write(codec, LINE2L_2_HPLOUT_VOL, DEFAULT_VOL); + snd_soc_write(codec, LINE2R_2_HPROUT_VOL, DEFAULT_VOL); + snd_soc_write(codec, LINE2L_2_HPLCOM_VOL, DEFAULT_VOL); + snd_soc_write(codec, LINE2R_2_HPRCOM_VOL, DEFAULT_VOL); + /* Line2 Line Out default volume, disconnect from Output Mixer */ + snd_soc_write(codec, LINE2L_2_LLOPM_VOL, DEFAULT_VOL); + snd_soc_write(codec, LINE2R_2_RLOPM_VOL, DEFAULT_VOL); + } switch (aic3x->model) { case AIC3X_MODEL_3X: