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: Mon, 4 Dec 2017 08:51:00 +0530 Message-ID: <20171204032059.GY32417@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by alsa0.perex.cz (Postfix) with ESMTP id 3018F26710B for ; Mon, 4 Dec 2017 04:17:24 +0100 (CET) Content-Disposition: inline In-Reply-To: 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: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 -- ~Vinod