From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753284AbdLDDLk (ORCPT <rfc822;w@1wt.eu>);
        Sun, 3 Dec 2017 22:11:40 -0500
Received: from mga05.intel.com ([192.55.52.43]:39238 "EHLO mga05.intel.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1752802AbdLDDLj (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Sun, 3 Dec 2017 22:11:39 -0500
X-ExtLoop1: 1
X-IronPort-AV: E=Sophos;i="5.45,357,1508828400"; 
   d="scan'208";a="180916258"
Subject: Re: [alsa-devel] [PATCH v4 09/15] soundwire: Add slave status
 handling
To: Vinod Koul <vinod.koul@intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        LKML <linux-kernel@vger.kernel.org>,
        ALSA <alsa-devel@alsa-project.org>, Mark <broonie@kernel.org>,
        Takashi <tiwai@suse.de>, patches.audio@intel.com, alan@linux.intel.com,
        Charles Keepax <ckeepax@opensource.cirrus.com>,
        Sagar Dharia <sdharia@codeaurora.org>, srinivas.kandagatla@linaro.org,
        plai@codeaurora.org, Sudheer Papothi <spapothi@codeaurora.org>
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>
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Message-ID: <fb95d8f8-b6a5-6848-8b28-52e2c77a90bd@linux.intel.com>
Date: Sun, 3 Dec 2017 21:11:39 -0600
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:52.0)
 Gecko/20100101 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171203171139.GQ32417@localhost>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.