Re: [PATCH 4/4] ASoC: SDCA: Add basic SDCA function driver

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

> +static int class_function_sdw_add_peripheral(struct snd_pcm_substream *substream,
> +					     struct snd_pcm_hw_params *params,
> +					     struct snd_soc_dai *dai)
> +{
> +	struct class_function_drv *drv = snd_soc_component_get_drvdata(dai->component);
> +	struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> +	struct sdw_slave *sdw = dev_to_sdw_dev(drv->dev->parent);
> +	struct sdw_stream_config sconfig = {0};
> +	struct sdw_port_config pconfig = {0};
> +	int ret;
> +
> +	if (!sdw_stream)
> +		return -EINVAL;
> +
> +	snd_sdw_params_to_config(substream, params, &sconfig, &pconfig);
> +
> +	ret = sdca_asoc_get_port(drv->dev, drv->regmap, drv->function, dai);
> +	if (ret < 0)
> +		return ret;
> +
> +	pconfig.num = ret;

You should probably document the assumption that each function has access to separate ports. 

IIRC there could be cases where ports are *shared* between functions due to hardware restrictions. I remember telling the SDCA committee that either functions were independent or they were not, and the answer is that they are independent except when they aren't. It's probably ok to assume a simple case first to make progress, but with a clear documentation of the known limits.

Oh and for some functions multiple ports are required to make use of different lanes, e.g. for the digital links in the NDAI functions.

> +
> +	ret = sdw_stream_add_slave(sdw, &sconfig, &pconfig, 1, sdw_stream);
> +	if (ret) {
> +		dev_err(drv->dev, "failed to add sdw stream: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return sdca_asoc_hw_params(drv->dev, drv->regmap, drv->function,
> +				   substream, params, dai);
> +}
> +
> +static int class_function_sdw_remove_peripheral(struct snd_pcm_substream *substream,
> +						struct snd_soc_dai *dai)
> +{
> +	struct class_function_drv *drv = snd_soc_component_get_drvdata(dai->component);
> +	struct sdw_stream_runtime *sdw_stream = snd_soc_dai_get_dma_data(dai, substream);
> +	struct sdw_slave *sdw = dev_to_sdw_dev(drv->dev->parent);
> +
> +	if (!sdw_stream)
> +		return -EINVAL;
> +
> +	return sdw_stream_remove_slave(sdw, sdw_stream);
> +}
> +
> +static int class_function_sdw_set_stream(struct snd_soc_dai *dai, void *sdw_stream,
> +					 int direction)
> +{
> +	snd_soc_dai_dma_data_set(dai, direction, sdw_stream);
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_dai_ops class_function_sdw_ops = {
> +	.startup	= class_function_startup,
> +	.shutdown	= sdca_asoc_free_constraints,
> +	.set_stream	= class_function_sdw_set_stream,
> +	.hw_params	= class_function_sdw_add_peripheral,
> +	.hw_free	= class_function_sdw_remove_peripheral,
> +};
> +
> +static int class_function_component_probe(struct snd_soc_component *component)
> +{
> +	struct class_function_drv *drv = snd_soc_component_get_drvdata(component);
> +	struct sdca_class_drv *core = drv->core;
> +
> +	return sdca_irq_populate(drv->function, component, core->irq_info);
> +}
> +
> +static const struct snd_soc_component_driver class_function_component_drv = {
> +	.probe			= class_function_component_probe,
> +	.endianness		= 1,
> +};
> +
> +static int class_function_boot(struct class_function_drv *drv)
> +{
> +	unsigned int reg = SDW_SDCA_CTL(drv->function->desc->adr,
> +					SDCA_ENTITY_TYPE_ENTITY_0,
> +					SDCA_CTL_ENTITY_0_FUNCTION_STATUS, 0);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(drv->regmap, reg, &val);
> +	if (ret < 0) {
> +		dev_err(drv->dev, "failed to read function status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!(val & SDCA_CTL_ENTITY_0_FUNCTION_HAS_BEEN_RESET)) {
> +		dev_dbg(drv->dev, "reset function device\n");
> +
> +		ret = sdca_reset_function(drv->dev, drv->function, drv->regmap);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (val & SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION) {
> +		dev_dbg(drv->dev, "write initialisation\n");
> +
> +		ret = sdca_regmap_write_init(drv->dev, drv->core->dev_regmap,
> +					     drv->function);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_write(drv->regmap, reg,
> +				   SDCA_CTL_ENTITY_0_FUNCTION_NEEDS_INITIALIZATION);
> +		if (ret < 0) {
> +			dev_err(drv->dev,
> +				"failed to clear function init status: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
> +	/* Start FDL process */
> +	ret = sdca_irq_populate_early(drv->dev, drv->regmap, drv->function,
> +				      drv->core->irq_info);
> +	if (ret)
> +		return ret;
> +
> +	ret = sdca_fdl_sync(drv->dev, drv->function, drv->core->irq_info);
> +	if (ret)
> +		return ret;
> +
> +	ret = sdca_regmap_write_defaults(drv->dev, drv->regmap, drv->function);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(drv->regmap, reg, 0xFF);
> +	if (ret < 0) {
> +		dev_err(drv->dev, "failed to clear function status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int class_function_probe(struct auxiliary_device *auxdev,
> +				const struct auxiliary_device_id *aux_dev_id)
> +{
> +	struct device *dev = &auxdev->dev;
> +	struct sdca_class_drv *core = dev_get_drvdata(dev->parent);
> +	struct sdca_device_data *data = &core->sdw->sdca_data;
> +	struct sdca_function_desc *desc;
> +	struct snd_soc_component_driver *cmp_drv;
> +	struct snd_soc_dai_driver *dais;
> +	struct class_function_drv *drv;
> +	struct regmap_sdw_mbq_cfg *mbq_config;
> +	struct regmap_config *config;
> +	struct reg_default *defaults;
> +	int ndefaults;
> +	int num_dais;
> +	int ret;
> +	int i;
> +
> +	drv = devm_kzalloc(dev, sizeof(*drv), GFP_KERNEL);
> +	if (!drv)
> +		return -ENOMEM;
> +
> +	cmp_drv = devm_kmemdup(dev, &class_function_component_drv, sizeof(*cmp_drv),
> +			       GFP_KERNEL);
> +	if (!cmp_drv)
> +		return -ENOMEM;
> +
> +	config = devm_kmemdup(dev, &class_function_regmap_config, sizeof(*config),
> +			      GFP_KERNEL);
> +	if (!config)
> +		return -ENOMEM;
> +
> +	mbq_config = devm_kmemdup(dev, &class_function_mbq_config, sizeof(*mbq_config),
> +				  GFP_KERNEL);
> +	if (!mbq_config)
> +		return -ENOMEM;
> +
> +	drv->dev = dev;
> +	drv->core = core;
> +
> +	for (i = 0; i < data->num_functions; i++) {
> +		desc = &data->function[i];
> +
> +		if (desc->type == aux_dev_id->driver_data)
> +			break;
> +	}
> +	if (i == core->sdw->sdca_data.num_functions) {
> +		dev_err(dev, "failed to locate function\n");
> +		return -EINVAL;
> +	}
> +
> +	drv->function = &core->functions[i];
> +
> +	ret = sdca_parse_function(dev, core->sdw, desc, drv->function);
> +	if (ret)
> +		return ret;
> +
> +	ndefaults = sdca_regmap_count_constants(dev, drv->function);
> +	if (ndefaults < 0)
> +		return ndefaults;
> +
> +	defaults = devm_kcalloc(dev, ndefaults, sizeof(*defaults), GFP_KERNEL);
> +	if (!defaults)
> +		return -ENOMEM;
> +
> +	ret = sdca_regmap_populate_constants(dev, drv->function, defaults);
> +	if (ret < 0)
> +		return ret;
> +
> +	regcache_sort_defaults(defaults, ndefaults);
> +
> +	auxiliary_set_drvdata(auxdev, drv);
> +
> +	config->reg_defaults = defaults;
> +	config->num_reg_defaults = ndefaults;
> +	config->lock_arg = &core->regmap_lock;
> +
> +	if (drv->function->busy_max_delay) {
> +		mbq_config->timeout_us = drv->function->busy_max_delay;
> +		mbq_config->retry_us = umax(drv->function->busy_max_delay / 10,
> +					    mbq_config->retry_us);
> +	}
> +
> +	drv->regmap = devm_regmap_init_sdw_mbq_cfg(dev, core->sdw, config, mbq_config);
> +	if (IS_ERR(drv->regmap))
> +		return dev_err_probe(dev, PTR_ERR(drv->regmap),
> +				     "failed to create regmap");
> +
> +	ret = sdca_asoc_populate_component(dev, drv->function, cmp_drv,
> +					   &dais, &num_dais,
> +					   &class_function_sdw_ops);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_set_autosuspend_delay(dev, 200);

why 200?

> +	pm_runtime_use_autosuspend(dev);
> +	pm_runtime_set_active(dev);
> +	pm_runtime_get_noresume(dev);
same comment, this is the same sequence as for the parent and I don't quite follow the pm_runtime flow.

> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = class_function_boot(drv);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_snd_soc_register_component(dev, cmp_drv, dais, num_dais);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to register component\n");
> +
> +	dev_err(dev, "%s: %pfwP: probe completed\n", __func__, auxdev->dev.fwnode);

why is this an error? the error case is handled earlier.

> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return 0;
> +}
> +
> +static int class_function_codec_runtime_suspend(struct device *dev)
> +{
> +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> +	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
> +
> +	/*
> +	 * Whilst the driver doesn't power the chip down here, going into
> +	 * runtime suspend lets the SoundWire bus power down, which means
> +	 * the driver can't communicate with the device any more.
> +	 */

actually this is not completely correct. The function driver can communicate IF the parent is active. This is copy-paste from the device level but it's not how the hardware works.

> +	regcache_cache_only(drv->regmap, true);

I would also add a comment that all the power management for PE/GEs is handled by DAPM when streams become active/suspended. So there isn't a need to deal with PE/GEs at this level.

> +
> +	return 0;
> +}
> +
> +static int class_function_codec_runtime_resume(struct device *dev)
> +{
> +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> +	struct class_function_drv *drv = auxiliary_get_drvdata(auxdev);
> +	int ret;
> +
> +	regcache_cache_only(drv->regmap, false);
> +
> +	regcache_mark_dirty(drv->regmap);

does the order matter?

> +
> +	ret = regcache_sync(drv->regmap);
> +	if (ret) {
> +		dev_err(drv->dev, "failed to restore register cache: %d\n", ret);
> +		goto err;
> +	}
> +
> +	return 0;
> +
> +err:
> +	regcache_cache_only(drv->regmap, true);
> +
> +	return ret;
> +}
> +
> +static const struct dev_pm_ops class_function_pm_ops = {
> +	RUNTIME_PM_OPS(class_function_codec_runtime_suspend,
> +		       class_function_codec_runtime_resume, NULL)
> +};
> +
> +static const struct auxiliary_device_id class_function_id_table[] = {
> +	{
> +		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_SMART_AMP_NAME,
> +		.driver_data = SDCA_FUNCTION_TYPE_SMART_AMP,
> +	},
> +	{
> +		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_SMART_MIC_NAME,
> +		.driver_data = SDCA_FUNCTION_TYPE_SMART_MIC,
> +	},
> +	{
> +		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_UAJ_NAME,
> +		.driver_data = SDCA_FUNCTION_TYPE_UAJ,
> +	},
> +	{
> +		.name = "snd_soc_sdca." SDCA_FUNCTION_TYPE_HID_NAME,
> +		.driver_data = SDCA_FUNCTION_TYPE_HID,
> +	},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(auxiliary, class_function_id_table);
> +
> +static struct auxiliary_driver class_function_drv = {
> +	.driver = {
> +		.name		= "sdca_function",
> +		.pm		= pm_ptr(&class_function_pm_ops),
> +	},
> +
> +	.probe		= class_function_probe,
> +	.id_table	= class_function_id_table
> +};
> +module_auxiliary_driver(class_function_drv);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("SDCA Class Function Driver");
> +MODULE_IMPORT_NS("SND_SOC_SDCA");