From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753354AbdLDDR1 (ORCPT ); Sun, 3 Dec 2017 22:17:27 -0500 Received: from mga03.intel.com ([134.134.136.65]:32080 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752917AbdLDDRY (ORCPT ); Sun, 3 Dec 2017 22:17:24 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,357,1508828400"; d="scan'208";a="183616535" Date: Mon, 4 Dec 2017 08:51:00 +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: <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-Disposition: inline In-Reply-To: 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: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 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