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/
