From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754701AbdLFJl0 (ORCPT ); Wed, 6 Dec 2017 04:41:26 -0500 Received: from mga14.intel.com ([192.55.52.115]:10288 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754619AbdLFJlK (ORCPT ); Wed, 6 Dec 2017 04:41:10 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,367,1508828400"; d="scan'208";a="10070650" Date: Wed, 6 Dec 2017 15:14:48 +0530 From: Vinod Koul To: Pierre-Louis Bossart Cc: Greg Kroah-Hartman , LKML , ALSA , Mark , Takashi , patches.audio@intel.com, alan@linux.intel.com, Charles Keepax , Sagar Dharia , srinivas.kandagatla@linaro.org, plai@codeaurora.org, Sudheer Papothi Subject: Re: [alsa-devel] [PATCH v4 09/15] soundwire: Add slave status handling 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-Disposition: inline In-Reply-To: <382cc7a0-a478-fb30-9ea4-22f828cd9093@linux.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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