On 21.11.2017 02:14, Nicolin Chen wrote: > On Tue, Nov 21, 2017 at 01:32:09AM +0100, Maciej S. Szmigiero wrote: >> On 21.11.2017 01:00, Nicolin Chen wrote: >>> On Mon, Nov 20, 2017 at 11:13:45PM +0100, Maciej S. Szmigiero wrote: >> (..) >>>> We need to make sure, however, that only proper channel slots are enabled >>>> at playback start time since some AC'97 CODECs (like VT1613) were observed >>>> requesting via SLOTREQ (and so enabling at SSI) spurious ones just after >>>> an AC'97 link is started but before the CODEC is configured by its driver. >>> >>> I don't really understand this part. Why do we need to *make sure* >>> and set SACCDIS and SACCEN again since they're initialized already> >>> Could you please elaborate a bit more? >> >> SACCDIS and SACCEN aren't just normal registers. >> They are a way to disable or enable bits in SACCST register - writing >> a '1' bit at some position in SACCDIS unsets the relevant bit in SACCST, >> writing a '1' bit at some position in SACCEN sets the relevant bit in >> SACCST. >> >> But a bit in SACCST can also be set (and so an AC'97 channel slot >> enabled) if a CODEC sets the relevant bit in its SLOTREQ request (it is >> enough if this bit was set in SLOTREQ just once, bits in SACCST are >> 'sticky'). > > This makes sense now. It could be mentioned a bit in the commit log > as well. Will do. >> If an extra slot gets enabled that's a disaster for playback because some >> of normal left or right channel samples are instead redirected to this >> extra slot. >> >> Unfortunately, the VT1613 codec on UDOO board (which is the only user of >> fsl_ssi driver in the AC'97 mode) is a bit broken and likes to (sometimes) >> set extra bits in its SLOTREQ request - I've confirmed this on two boards >> bought a few months apart directly from UDOO shop. >> >> A workaround implemented in fsl-asoc-card of setting an appropriate >> CODEC register so that slots 3/4 (the normal stereo playback slots) are >> used for S/PDIF seems to mostly fix this issue but since this codec is so >> untrustworthy then: > > If this move is also a workaround, probably it'd be better to have > an fsl_ssi_ac97_xxx_war() function with some comments/descriptions, > and include it in the config(). Since it doesn't hurt to set these > registers again, I am personally fine with that. But it needs to be > clear -- otherwise, people may try to remove it later with a change > like: Removing redundant SACCEN/SACCDIS settings since they're done > during probe(). > That's a good point, will do. Maciej _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
