Re: [PATCH v3 RESEND 14/19] ASoC: SDCA: Add FDL library for XU entities

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

> +config SND_SOC_SDCA_FDL
> +	bool "SDCA FDL (File DownLoad) support"
> +	depends on SND_SOC_SDCA
> +	default y
> +	help
> +	  This option enables support for the File Download using UMP,
> +	  typically used for downloading firmware to devices.

nit-pick: shouldn't this option be selected by device drivers who need this library? There should be guardrails to prevent the fallback helpers from being used silently.

> +int sdca_reset_function(struct device *dev, struct sdca_function_data *function,
> +			struct regmap *regmap)
> +{
> +	unsigned int reg = SDW_SDCA_CTL(function->desc->adr,
> +					SDCA_ENTITY_TYPE_ENTITY_0,
> +					SDCA_CTL_ENTITY_0_FUNCTION_ACTION, 0);
> +	unsigned int val, poll_us;
> +	int ret;
> +
> +	ret = regmap_write(regmap, reg, SDCA_CTL_ENTITY_0_RESET_FUNCTION_NOW);
> +	if (ret) // Allowed for function reset to not be implemented
> +		return 0;

nit-pick: maybe document why this is allowed. This doesn't seem very good in terms of design.

> +
> +	if (!function->reset_max_delay) {
> +		dev_err(dev, "No reset delay specified in DisCo\n");
> +		return -EINVAL;
> +	}

nit-pick: or maybe fallback to a reasonable default? Making the entire download dependent on Disco correctness isn't going to work long-term.

> +
> +	poll_us = umin(function->reset_max_delay >> 4, 1000);

add comment on what this >> 4 means.

> +
> +	ret = regmap_read_poll_timeout(regmap, reg, val, !val, poll_us,
> +				       function->reset_max_delay);
> +	if (ret) {
> +		dev_err(dev, "Failed waiting for function reset: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS(sdca_reset_function, "SND_SOC_SDCA");

> +/**
> + * sdca_fdl_process - Process the FDL state machine
> + * @interrupt: SDCA interrupt structure
> + *
> + * Based on section 13.2.5 Flow Diagram for File Download, Host side.
> + *
> + * Return: Zero on success or a negative error code.
> + */
> +int sdca_fdl_process(struct sdca_interrupt *interrupt)
> +{
> +	struct device *dev = interrupt->dev;
> +	struct sdca_entity_xu *xu = &interrupt->entity->xu;
> +	unsigned int reg, status;
> +	int response, ret;
> +
> +	ret = sdca_ump_get_owner_host(dev, interrupt->function_regmap,
> +				      interrupt->function, interrupt->entity,
> +				      interrupt->control);
> +	if (ret)
> +		goto reset_function;
> +
> +	reg = SDW_SDCA_CTL(interrupt->function->desc->adr, interrupt->entity->id,
> +			   SDCA_CTL_XU_FDL_STATUS, 0);
> +	ret = regmap_read(interrupt->function_regmap, reg, &status);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to read FDL status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	dev_dbg(dev, "FDL status: %#x\n", status);
> +
> +	ret = fdl_status_process(interrupt, status);
> +	if (ret < 0)
> +		goto reset_function;
> +
> +	response = ret;
> +
> +	dev_dbg(dev, "FDL response: %#x\n", response);
> +
> +	ret = regmap_write(interrupt->function_regmap, reg,
> +			   response | (status & ~SDCA_CTL_XU_FDLH_MASK));
> +	if (ret < 0) {
> +		dev_err(dev, "failed to set FDL status signal: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = sdca_ump_set_owner_device(dev, interrupt->function_regmap,
> +					interrupt->function, interrupt->entity,
> +					interrupt->control);
> +	if (ret)
> +		return ret;
> +
> +	switch (response) {
> +	case SDCA_CTL_XU_FDLH_RESET_ACK:
> +		dev_dbg(dev, "FDL request reset\n");
> +
> +		switch (xu->reset_mechanism) {
> +		default:
> +			dev_warn(dev, "Requested reset mechanism not implemented\n");
> +			fallthrough;

this fallthrough looks odd since it forces a check on the reset a second time ...

> +		case SDCA_XU_RESET_FUNCTION:
> +			goto reset_function;
> +		}
> +	default:
> +		return 0;
> +	}
> +
> +reset_function:

... inside this function call

> +	sdca_reset_function(dev, interrupt->function, interrupt->function_regmap);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(sdca_fdl_process, "SND_SOC_SDCA");