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
