Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5363c7b7-7a5b-490c-445b-fb7ccd693c63@linux.intel.com>
Date:   Fri, 12 May 2023 11:34:44 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.10.0
Subject: Re: [PATCH 01/10] soundwire: bus: Allow SoundWire peripherals to
 register IRQ handlers
Content-Language: en-US
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     broonie@kernel.org, lee@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        tglx@linutronix.de, maz@kernel.org, linus.walleij@linaro.org,
        vkoul@kernel.org, lgirdwood@gmail.com,
        yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
        alsa-devel@alsa-project.org, patches@opensource.cirrus.com,
        devicetree@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230512122838.243002-1-ckeepax@opensource.cirrus.com>
 <20230512122838.243002-2-ckeepax@opensource.cirrus.com>
 <0471f085-14bf-c159-9b92-62983af6c19a@linux.intel.com>
 <20230512160224.GK68926@ediswmail.ad.cirrus.com>
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20230512160224.GK68926@ediswmail.ad.cirrus.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/12/23 11:02, Charles Keepax wrote:
> On Fri, May 12, 2023 at 08:45:51AM -0500, Pierre-Louis Bossart wrote:
>>> @@ -1711,6 +1739,9 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
>>>  				struct device *dev = &slave->dev;
>>>  				struct sdw_driver *drv = drv_to_sdw_driver(dev->driver);
>>>  
>>> +				if (slave->prop.use_domain_irq && slave->irq)
>>> +					handle_nested_irq(slave->irq);
>>> +
>>
>> I am a bit lost here, I can understand that alerts would be handled by a
>> dedicated handler, but here the code continues and will call the
>> existing interrupt_callback.
>>
>> Is this intentional? I wonder if there's a risk with two entities
>> dealing with the same event and programming the same registers.
>> Shouldn't there be some sort of 'either or' rule?
>>
> 
> I guess there is a risk of them "handling" the IRQ twice,
> although it is hard to see why you would write the driver that
> way. Also since they are sequencial the second would I guess
> just see that no IRQs are pending.
> 
> The intention for calling both is that it facilitates using
> the same IRQ handler for I2C and SoundWire. At least on the
> Cirrus devices there are a bunch of chip specific registers
> that need treated exactly the same on I2C and SoundWire, but
> then a couple of extra registers that need handled in the
> SoundWire case. This way the handling of those can be kept
> completely in the SoundWire part of the code and not ifdef-ed
> into the main IRQ path.

Sounds good to me, but it's worth adding a comment and improving the
commit message with design intent/rules since it's a common part in
drivers/soundwire/