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

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

> +/**
> + * struct sdca_interrupt - contains information about a single SDCA interrupt
> + * @name: The name of the interrupt.
> + * @component: Pointer to the ASoC component owns the interrupt.
> + * @function: Pointer to the Function that the interrupt is associated with.
> + * @entity: Pointer to the Entity that the interrupt is associated with.
> + * @control: Pointer to the Control that the interrupt is associated with.
> + * @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.

> + */
> +struct sdca_interrupt {
> +	const char *name;
> +
> +	struct snd_soc_component *component;
> +	struct sdca_function_data *function;
> +	struct sdca_entity *entity;
> +	struct sdca_control *control;
> +
> +	bool externally_requested;
> +};
> +
> +/**
> + * struct sdca_interrupt_info - contains top-level SDCA interrupt information
> + * @irq_chip: regmap irq chip structure.
> + * @irq_data: regmap irq chip data structure.
> + * @irqs: Array of data for each individual IRQ.
> + * @irq_lock: Protects access to the list of sdca_interrupt structures.
> + */
> +struct sdca_interrupt_info {
> +	struct regmap_irq_chip irq_chip;
> +	struct regmap_irq_chip_data *irq_data;
> +
> +	struct sdca_interrupt irqs[SDCA_MAX_INTERRUPTS];
> +
> +	struct mutex irq_lock; /* Protect accesses to irqs list */

Can you elaborate on what might conflict?

The only thing I can think of in terms of required locking is access to the common SDCA interrupt registers, but that's different to the irq list.

> +static const struct regmap_irq_chip sdca_irq_chip = {
> +	.name = "sdca_irq",
> +
> +	.status_base = SDW_SCP_SDCA_INT1,
> +	.unmask_base = SDW_SCP_SDCA_INTMASK1,
> +	.ack_base = SDW_SCP_SDCA_INT1,
> +	.num_regs = 4,
> +
> +	.irqs = regmap_irqs,
> +	.num_irqs = SDCA_MAX_INTERRUPTS,
> +
> +	.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.

> +};
> +
> +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?

> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int sdca_irq_request_locked(struct device *dev,
> +				   struct sdca_interrupt_info *info,
> +				   int sdca_irq, const char *name,
> +				   irq_handler_t handler, void *data)
> +{
> +	int irq;
> +	int ret;
> +
> +	irq = regmap_irq_get_virq(info->irq_data, sdca_irq);
> +	if (irq < 0)
> +		return irq;
> +
> +	ret = devm_request_threaded_irq(dev, irq, NULL, handler,
> +					IRQF_ONESHOT, name, data);
> +	if (ret)
> +		return ret;
> +
> +	dev_dbg(dev, "requested irq %d for %s\n", irq, name);

might be worth adding the function and entity name?

> +
> +	return 0;
> +}
> +
> +/**
> + * sdca_request_irq - request an individual SDCA interrupt
> + * @dev: Pointer to the struct device against which things should be allocated.
> + * @interrupt_info: Pointer to the interrupt information structure.
> + * @sdca_irq: SDCA interrupt position.
> + * @name: Name to be given to the IRQ.
> + * @handler: A callback thread function to be called for the IRQ.
> + * @data: Private data pointer that will be passed to the handler.
> + *
> + * Typically this is handled internally by sdca_irq_populate, however if
> + * a device requires custom IRQ handling this can be called manually before
> + * calling sdca_irq_populate, which will then skip that IRQ whilst processing.
> + *
> + * Return: Zero on success, and a negative error code on failure.
> + */
> +int sdca_irq_request(struct device *dev, struct sdca_interrupt_info *info,
> +		     int sdca_irq, const char *name, irq_handler_t handler,
> +		     void *data)
> +{
> +	int ret;
> +
> +	if (sdca_irq < 0 || sdca_irq > SDCA_MAX_INTERRUPTS) {
> +		dev_err(dev, "bad irq request: %d\n", sdca_irq);
> +		return -EINVAL;
> +	}
> +
> +	guard(mutex)(&info->irq_lock);
> +
> +	ret = sdca_irq_request_locked(dev, info, sdca_irq, name, handler, data);
> +	if (ret) {
> +		dev_err(dev, "failed to request irq %s: %d\n", name, ret);
> +		return ret;
> +	}
> +
> +	info->irqs[sdca_irq].externally_requested = true;

this looks like generic/core code, not sure what's 'external' about this...

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_request, "SND_SOC_SDCA_IRQ");
> +
> +/**
> + * sdca_irq_data_populate - Populate common interrupt data
> + * @component: Pointer to the ASoC component for the Function.
> + * @function: Pointer to the SDCA Function.
> + * @entity: Pointer to the SDCA Entity.
> + * @control: Pointer to the SDCA Control.
> + * @interrupt: Pointer to the SDCA interrupt for this IRQ.
> + *
> + * Return: Zero on success, and a negative error code on failure.
> + */
> +int sdca_irq_data_populate(struct snd_soc_component *component,
> +			   struct sdca_function_data *function,
> +			   struct sdca_entity *entity,
> +			   struct sdca_control *control,
> +			   struct sdca_interrupt *interrupt)
> +{
> +	struct device *dev = component->dev;
> +	const char *name;
> +
> +	name = devm_kasprintf(dev, GFP_KERNEL, "%s %s %s", function->desc->name,
> +			      entity->label, control->label);
> +	if (!name)
> +		return -ENOMEM;
> +
> +	interrupt->name = name;
> +	interrupt->component = component;
> +	interrupt->function = function;
> +	interrupt->entity = entity;
> +	interrupt->control = control;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_data_populate, "SND_SOC_SDCA_IRQ");
> +
> +/**
> + * sdca_irq_populate - Request all the individual IRQs for an SDCA Function
> + * @function: Pointer to the SDCA Function.
> + * @component: Pointer to the ASoC component for the Function.
> + * @info: Pointer to the SDCA interrupt info for this device.
> + *
> + * Typically this would be called from the driver for a single SDCA Function.
> + *
> + * Return: Zero on success, and a negative error code on failure.
> + */
> +int sdca_irq_populate(struct sdca_function_data *function,
> +		      struct snd_soc_component *component,
> +		      struct sdca_interrupt_info *info)
> +{
> +	struct device *dev = component->dev;
> +	int i, j;
> +
> +	guard(mutex)(&info->irq_lock);
> +
> +	for (i = 0; i < function->num_entities; i++) {
> +		struct sdca_entity *entity = &function->entities[i];
> +
> +		for (j = 0; j < entity->num_controls; j++) {
> +			struct sdca_control *control = &entity->controls[j];
> +			int irq = control->interrupt_position;
> +			struct sdca_interrupt *interrupt;
> +			const char *name;
> +			int ret;
> +
> +			if (irq == SDCA_NO_INTERRUPT) {
> +				continue;
> +			} else if (irq < 0 || irq >= SDCA_MAX_INTERRUPTS) {
> +				dev_err(dev, "bad irq position: %d\n", irq);
> +				return -EINVAL;
> +			}
> +
> +			interrupt = &info->irqs[irq];
> +
> +			if (interrupt->externally_requested) {
> +				dev_dbg(dev,
> +					"skipping irq %d, externally requested\n",
> +					irq);
> +				continue;
> +			}
> +
> +			ret = sdca_irq_data_populate(component, function, entity,
> +						     control, interrupt);
> +			if (ret)
> +				return ret;
> +
> +			ret = sdca_irq_request_locked(dev, info, irq, interrupt->name,
> +						      base_handler, interrupt);
> +			if (ret) {
> +				dev_err(dev, "failed to request irq %s: %d\n",
> +					name, ret);
> +				return ret;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_populate, "SND_SOC_SDCA_IRQ");
> +
> +/**
> + * sdca_irq_allocate - allocate an SDCA interrupt structure for a device
> + * @dev: Device pointer against which things should be allocated.
> + * @regmap: regmap to be used for accessing the SDCA IRQ registers.
> + * @irq: The interrupt number.
> + *
> + * Typically this would be called from the top level driver for the whole
> + * SDCA device, as only a single instance is required across all Functions
> + * on the device.
> + *
> + * Return: A pointer to the allocated sdca_interrupt_info struct, or an
> + * error code.
> + */
> +struct sdca_interrupt_info *sdca_irq_allocate(struct device *dev,
> +					      struct regmap *regmap, int irq)
> +{
> +	struct sdca_interrupt_info *info;
> +	int ret;
> +
> +	info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return ERR_PTR(-ENOMEM);
> +
> +	info->irq_chip = sdca_irq_chip;
> +
> +	devm_mutex_init(dev, &info->irq_lock);
> +
> +	ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT, 0,
> +				       &info->irq_chip, &info->irq_data);
> +	if (ret) {
> +		dev_err(dev, "failed to register irq chip: %d\n", ret);
> +		return ERR_PTR(ret);
> +	}
> +
> +	dev_dbg(dev, "registered on irq %d\n", irq);

not sure how helpful this message might be, it'll be rather cryptic without additional context on which interrupt this code handles.

> +
> +	return info;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_irq_allocate, "SND_SOC_SDCA_IRQ");
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SDCA IRQ library");