On Fri, May 12, 2023 at 11:34:44AM -0500, Pierre-Louis Bossart wrote:
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/
Yeah no issues with updating the commit message to explain that in more detail.
Thanks, Charles