Re: [PATCH 6/7] ASoC: SDCA: Generic interrupt support

[Charles Keepax <ckeepax@opensource.cirrus.com>]

On Tue, Jun 10, 2025 at 07:55:23PM +0200, Pierre-Louis Bossart wrote:
> On 6/10/25 12:21, Charles Keepax wrote:
> > On Tue, Jun 10, 2025 at 10:52:30AM +0200, Pierre-Louis Bossart wrote:

Apologies for the delay, I was sure I responded to this but I
must have done one of those writing the email then failing to
send it tricks.

> >>> + * @externally_requested: Internal flag used to check if something has already
> >>> + * requested the interrupt.
> >>
> >> I am not following what 'externally' and 'something'
> >> refer to. Each interrupt is allocated to a specific
> >> function/entity/control, this hints at another agent getting in
> >> the way but that's not really how this is supposed to work.
> >>
> >> This deserves additional clarifications IMHO.
> > 
> > I can update the commit message/comment a bit, but the idea
> > is mostly we are creating an SDCA library here, so there are
> > two possible IRQ flows, the SDCA framework can handle the IRQ
> > internally, ie. using the handlers present in sdca_interrupts.c or
> > the client driver can register a handler for the IRQ directly to
> > do additional magic. This flag lets the core know that was done.
> 
> I would have expected a base behavior to deal with the interrupt
> itself, and optionally a vendor-specific extension for additional
> magic. I don't really get the notion of 'two possible flows'.

There really isn't a lot of difference between the two:

  1) Internal: the handler is all class compliant the core just
     registers it for you:
      - device driver calls sdca_irq_allocate to register the
        root IRQ.
      - function driver calls sdca_irq_populate to register all
        the handlers.
  2) External: the handler is not class compliant the end driver
     should register its own handler to do whatever magic:
      - device driver calls sdca_irq_allocate to register the
        root IRQ.
      - function driver calls sdca_irq_request for the IRQs that
        need custom handling.
      - function driver calls sdca_irq_populate to register any
        remaining handlers, this can be skipped if all IRQs were
        registered through sdca_irq_request.

Its really just a cutting down on boiler plate code thing, one
could always make the client driver register all IRQs through
sdca_irq_request and then you wouldn't need the external flag,
but its nice for compliant devices if you can just have a single
register all IRQs call.

> >>> +	.runtime_pm = true,
> >>
> >> can you clarify what this last initialization does? runtime_pm
> >> is far from obvious for SDCA with the different levels between
> >> the SoundWire bus (enumeration, clock stop, register access,
> >> etc) and Function management.
> > 
> > Means the regmap IRQ handler will do a PM runtime get whilst
> > handling the IRQ, which will ensure the both the device and the
> > controller are powered up, which will be necessary for
> > communicating with the device.
> 
> this is kind of odd, the device needs to already be up before
> you have an SDCA interrupt, no?
> The only case where the device and controller would be in a
> low-power state would be in the clock-stop, but that's different
> to the SDCA interrupt, it's the SoundWire 'keep data line high'
> mechanism.

I mean yes the device is probably already up when the IRQ is
first processed by the SoundWire. In general its just good
practice to be holding a runtime reference whilst interacting
with the part, stop things turning off whilst you do so.

> >>> +static irqreturn_t base_handler(int irq, void *data)
> >>> +{
> >>> +	struct sdca_interrupt *interrupt = data;
> >>> +	struct device *dev = interrupt->component->dev;
> >>> +
> >>> +	dev_warn(dev, "%s irq without full handling\n", interrupt->name);
> >>
> >> is this saying this function is really a placeholder for
> >> something else?
> > 
> > Yeah basically, this will be assigned to any IRQ that doesn't
> > have an implemented handler. For now that is most things, but
> > even in the end once the core is more flushed out this will likely
> > persist as IRQs are control specific in SDCA. This will always be
> > necessary for controls that don't have any spec mandated handling
> > but could get an IRQ for implementation specific purposes and
> > would have a custom handler registered by the client driver. It
> > is really a default handler to warn the user/system implementor
> > that some code is likely missing for a part.
> 
> ok, not sure a dev_warn is required though?

It is useful information to the user, it typically indicates the
part is doing something that has no software support. I would be
happy to downgrade to an info or dbg if you felt strongly, but
personally I think a warn feels right here.

> >>> +	info->irqs[sdca_irq].externally_requested = true;
> >>
> >> this looks like generic/core code, not sure what's 'external'
> >> about this...
> > 
> > The key point here is that handler is passed into the function,
> > that is the destinction between an internal and external here,
> > internal uses a built in handler, external gets handed a handler
> > by the client driver.
> 
> I am not sure I follow this one, you could have a common
> mechanism followed by a vendor-specific one. Why would there be
> a choice to make? IIRC the mechanism I implemented in the early
> stages didn't have this architectural notion of internal/external,
> just a vendor-specific callback once the interrupt was cleared.
> 

The regmap IRQ always clears the interrupt, so that is the same
as your previous implementation. The handler registered here is
analogous to your vendor-specific callback whether it is
internal or external. The extra layer here is just a convience,
the core can register all the IRQ handlers for you if you don't
need any non-class compliant handling. Just saves a bunch of
boiler plate in the drivers registering all the IRQs.

Thanks,
Charles