Re: [PATCH 7/7] ASoC: SDCA: Add some initial IRQ handlers

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

On Tue, Jun 10, 2025 at 11:07:00AM +0200, Pierre-Louis Bossart wrote:
> > +	ret = regmap_read(interrupt->component->regmap, reg, &val);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to read function status: %d\n", ret);
> > +		return IRQ_NONE;
> 
> usually when a read fails, the entire device is in the
> weeds. Not sure squelching the interrupts is wise, something
> more drastic should happen.

I am not sure I really want to get to far into complex error
handling at the moment. I think I would lean towards focusing on
getting the functional case working first. This is also pretty
much what all audio drivers do on register read errors log a
message, abort the current operation, and carry on. There isn't
great value in doing more unless one goes so far as to actually
attempt to reboot the device and recover, which I would definitely
put outside the scope of what we are trying to achieve here.

> > +	dev_dbg(dev, "function status: %#x\n", val);
> > +
> > +	status = val;
> > +	for_each_set_bit(mask, &status, BITS_PER_BYTE) {
> > +		mask = 1 << mask;
> > +
> > +		switch (mask) {
> > +		case SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION:
> > +			//FIXME: Add init writes
> > +			break;
> 
> between here...
> 
> > +		case SDCA_CTL_ENTITY_0_FUNCTION_FAULT:
> > +			dev_err(dev, "function fault\n");
> > +			break;
> > +		case SDCA_CTL_ENTITY_0_UMP_SEQUENCE_FAULT:
> > +			dev_err(dev, "ump sequence fault\n");
> > +			break;
> > +		case SDCA_CTL_ENTITY_0_DEVICE_NEWLY_ATTACHED:
> > +		case SDCA_CTL_ENTITY_0_INTS_DISABLED_ABNORMALLY:
> > +		case SDCA_CTL_ENTITY_0_STREAMING_STOPPED_ABNORMALLY:
> > +		case SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET:
> 
> ... and here, the status bit essentially mean the Function isn't
> working as expected. I really don't see the point of attempting
> to write a register below, we'd need something that essentially
> resets the SoundWire device to restart from a known position.

The FAULT ones definitely, hence why I log an error. The others
its less clear, definitely on first boot both NEWLY_ATTACHED
and RESET are likely going to be set. The two ABNORMALLY's are
also a bit sketchy, if you look in Table 198 these can be set
after any reset. Basically where one gets to is  all of these
can basically happen during normal operation.

One could probably get into more complex trying to filter out
post reset events and catch later ones, but again I am hesitant
to get to far into complex error fielding like that. It seems
like it would make more sense to tackle those problems once we
have some situations where people are actually hitting problems
then the requirements might start to become a little more clear.

At most right now I would probably go for a debug in here, but it
feels redundant as there is the line above logging the function
status value. Also if a device wants to respond to these with
more strenuous measures it could override the function status
IRQ and taking device specific action.

> > +		case SDCA_CTL_ENTITY_0_FUNCTION_BUSY:
> > +			break;
> 
> Conversely this one seems harmless, FUNCTION_BUSY only impacts
> specific SDCA registers and this status cannot have any bearing
> on how to deal with interrupts.

Yeah this should be pretty harmless... well unless its a device
that function busy's for random reasons (ie. reasons other than a
deferred register operation), but that is not something we are
currently adding support for.

> > +	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
> > +			   interrupt->control->sel, 0);
> 
> Humm, I have to walk back what I wrote above, if FUNCTION_BUSY
> is set the read below can be problematic, no?

In the current register operation function busy model, this will
always be fine as this operation will block on the regmap lock
which should be held by the previous register operation that is
causing the function busy.

> > +	switch (val) {
> > +	case SDCA_DETECTED_MODE_DETECTION_IN_PROGRESS:
> > +	case SDCA_DETECTED_MODE_JACK_UNKNOWN:
> > +		reg = SDW_SDCA_CTL(interrupt->function->desc->adr,
> > +				   interrupt->entity->id,
> > +				   SDCA_CTL_GE_SELECTED_MODE, 0);
> > +
> > +		/*
> > +		 * Selected mode is not typically a volatile register, but
> > +		 * force a read from the hardware in the case detected mode
> > +		 * is unknown to see what the device selected as a "safe"
> > +		 * option.
> > +		 */
> 
> I am not sure this explanation is correct. I was my
> understanding that when there's a jack interrupt, both
> the DETECTED and SELECTED values are set by hardware, but
> SELECTED can be overridden by higher-level software or user
> interaction. DETECTED is RO, SELECTED is R/W IIRC.

That is basically what I am trying to say in the comment. Normal
proceedure from the class side is to read the DETECTED and write
that into the SELECTED, even though the hardware will have likely
already done that. But it is slightly unusual to have a register
that is Class Access and RW but still written by the device. RW
registers are usually cached, here we need to drop that cache to
ensure we read the value which the firmware has tampered with,
because we if DETECTED was showing unknown we want to know what
"safe" value the firmware picked for SELECTED rather than our
cached value.

Thanks,
Charles