From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vinod Koul Subject: Re: [PATCH v4 09/15] soundwire: Add slave status handling Date: Wed, 6 Dec 2017 15:14:48 +0530 Message-ID: <20171206094448.GP32417@localhost> References: <1512122177-2889-1-git-send-email-vinod.koul@intel.com> <1512122177-2889-10-git-send-email-vinod.koul@intel.com> <693f15b3-3751-2416-5d81-abd362464309@linux.intel.com> <20171203171139.GQ32417@localhost> <20171204032059.GY32417@localhost> <382cc7a0-a478-fb30-9ea4-22f828cd9093@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by alsa0.perex.cz (Postfix) with ESMTP id 08D3C26698E for ; Wed, 6 Dec 2017 10:41:11 +0100 (CET) Content-Disposition: inline In-Reply-To: <382cc7a0-a478-fb30-9ea4-22f828cd9093@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Pierre-Louis Bossart Cc: ALSA , Charles Keepax , Sudheer Papothi , Takashi , Greg Kroah-Hartman , plai@codeaurora.org, LKML , patches.audio@intel.com, Mark , srinivas.kandagatla@linaro.org, Sagar Dharia , alan@linux.intel.com List-Id: alsa-devel@alsa-project.org On Sun, Dec 03, 2017 at 09:52:48PM -0600, Pierre-Louis Bossart wrote: > On 12/3/17 9:21 PM, Vinod Koul wrote: > >On Sun, Dec 03, 2017 at 09:11:39PM -0600, Pierre-Louis Bossart wrote: > >>On 12/3/17 11:11 AM, Vinod Koul wrote: > >>>On Fri, Dec 01, 2017 at 05:52:03PM -0600, Pierre-Louis Bossart wrote: > >>> > >>>>>+ status = sdw_read(slave, SDW_DP0_INT); > >>>>>+ if (status < 0) { > >>>>>+ dev_err(slave->bus->dev, > >>>>>+ "SDW_DP0_INT read failed:%d", status); > >>>>>+ return status; > >>>>>+ } > >>>>>+ > >>>>>+ count++; > >>>>>+ > >>>>>+ /* we can get alerts while processing so keep retrying */ > >>>> > >>>>This is not incorrect, but this goes beyond what the spec requires. > >>>> > >>>>The additional read is to make sure some interrupts are not lost due to a > >>>>known race condition. It would be enough to mask the status read the second > >>>>time to only check if the interrupts sources which were cleared are still > >>>>signaling something. > >>>> > >>>>With the code as it is, you may catch *new* interrupt sources, which could > >>>>impact the arbitration/priority/policy in handling interrupts. It's not > >>>>necessarily bad, but you'd need to document whether you want to deal with > >>>>the race condition described in the MIPI spec or try to be smarter. > >>> > >>>This was based on your last comment, lets discuss more offline on this to > >>>see what else is required here. > >> > >>I am fine if you leave the code as is for now, it's not bad but can be > >>optimized. > > > >Not bad is not good here :) > > > >Okay I still havent grabbed my coffee, so help me out here. I am not sure I > >understand here, can you point me to the part of spec handling you were > >referring and what should be *ideally* done > > You first read the status, then clear the interrupts then re-read the > status. I'd be good enough in the second read to mask with the settings of > the first read. This is intended to detect alert sources that fired between > the last successful read and the write to clear interrupts (see Figure 92 in > the 1.1 spec) > > e.g. > > do { > status1= sdw_read() > deal with interrupts > status2 = sdw_read() > status2 &= status1; /* filter initial sources */ Sounds better, updated now, thanks -- ~Vinod