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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

> +static irqreturn_t function_status_handler(int irq, void *data)
> +{
> +	struct sdca_interrupt *interrupt = data;
> +	struct device *dev = interrupt->component->dev;
> +	unsigned int reg, val;
> +	unsigned long status;
> +	unsigned int mask;
> +	int ret;
> +
> +	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
> +			   interrupt->control->sel, 0);
> +
> +	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.

> +	}
> +
> +	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.

> +		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.

> +		}
> +	}
> +
> +	ret = regmap_write(interrupt->component->regmap, reg, val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to clear function status: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t detected_mode_handler(int irq, void *data)
> +{
> +	struct sdca_interrupt *interrupt = data;
> +	struct snd_soc_component *component = interrupt->component;
> +	struct device *dev = component->dev;
> +	struct snd_soc_card *card = component->card;
> +	struct rw_semaphore *rwsem = &card->snd_card->controls_rwsem;
> +	struct snd_kcontrol *kctl = interrupt->priv;
> +	struct snd_ctl_elem_value ucontrol;
> +	struct soc_enum *soc_enum;
> +	unsigned int reg, val;
> +	int ret;
> +
> +	if (!kctl) {
> +		const char *name __free(kfree) = kasprintf(GFP_KERNEL, "%s %s",
> +							   interrupt->entity->label,
> +							   SDCA_CTL_SELECTED_MODE_NAME);
> +
> +		if (!name)
> +			return -ENOMEM;
> +
> +		kctl = snd_soc_component_get_kcontrol(component, name);
> +		if (!kctl) {
> +			dev_dbg(dev, "control not found: %s\n", name);
> +			return IRQ_NONE;
> +		}
> +
> +		interrupt->priv = kctl;
> +	}
> +
> +	soc_enum = (struct soc_enum *)kctl->private_value;
> +
> +	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?

> +	ret = regmap_read(component->regmap, reg, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to read detected mode: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	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.

> +		regcache_drop_region(component->regmap, reg, reg);
> +
> +		ret = regmap_read(component->regmap, reg, &val);
> +		if (ret) {
> +			dev_err(dev, "failed to re-check selected mode: %d\n", ret);
> +			return IRQ_NONE;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	dev_dbg(dev, "%s: %#x\n", interrupt->name, val);
> +
> +	ucontrol.value.enumerated.item[0] = snd_soc_enum_val_to_item(soc_enum, val);
> +
> +	down_write(rwsem);
> +	ret = kctl->put(kctl, &ucontrol);
> +	up_write(rwsem);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to update selected mode: %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	snd_ctl_notify(card->snd_card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int sdca_irq_request_locked(struct device *dev,
>  				   struct sdca_interrupt_info *info,
>  				   int sdca_irq, const char *name,
> @@ -202,6 +339,7 @@ int sdca_irq_populate(struct sdca_function_data *function,
>  			struct sdca_control *control = &entity->controls[j];
>  			int irq = control->interrupt_position;
>  			struct sdca_interrupt *interrupt;
> +			irq_handler_t handler;
>  			const char *name;
>  			int ret;
>  
> @@ -226,8 +364,23 @@ int sdca_irq_populate(struct sdca_function_data *function,
>  			if (ret)
>  				return ret;
>  
> +			handler = base_handler;
> +
> +			switch (entity->type) {
> +			case SDCA_ENTITY_TYPE_ENTITY_0:
> +				if (control->sel == SDCA_CTL_ENTITY_0_FUNCTION_STATUS)
> +					handler = function_status_handler;
> +				break;
> +			case SDCA_ENTITY_TYPE_GE:
> +				if (control->sel == SDCA_CTL_GE_DETECTED_MODE)
> +					handler = detected_mode_handler;
> +				break;
> +			default:
> +				break;
> +			}
> +
>  			ret = sdca_irq_request_locked(dev, info, irq, interrupt->name,
> -						      base_handler, interrupt);
> +						      handler, interrupt);
>  			if (ret) {
>  				dev_err(dev, "failed to request irq %s: %d\n",
>  					name, ret);