From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751871AbdKUBOV (ORCPT ); Mon, 20 Nov 2017 20:14:21 -0500 Received: from mail-pf0-f193.google.com ([209.85.192.193]:33738 "EHLO mail-pf0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751195AbdKUBOT (ORCPT ); Mon, 20 Nov 2017 20:14:19 -0500 X-Google-Smtp-Source: AGs4zMZe1hIelBbA84hpQxGrD438ffZNzAkN1saivbkGBtY6RppgWC3Cr7DzZud14ZQMCkcO1lK32w== Date: Mon, 20 Nov 2017 17:14:17 -0800 From: Nicolin Chen To: "Maciej S. Szmigiero" Cc: Timur Tabi , Xiubo Li , alsa-devel@alsa-project.org, linux-kernel , Takashi Iwai , Liam Girdwood , Mark Brown , Fabio Estevam , linuxppc-dev@lists.ozlabs.org Subject: Re: [alsa-devel] [PATCH] ASoC: fsl_ssi: call _fsl_ssi_set_dai_fmt() just once in AC'97 mode Message-ID: <20171121011416.GD14136@Asurada-Nvidia> References: <81ad2e7e-e070-c38e-b318-9607b4558ee7@maciej.szmigiero.name> <20171121000042.GB14136@Asurada-Nvidia> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > 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().