RE: [PATCH v5 01/13] ASoC: Intel: Add catpt device

["Rojewski, Cezary" <cezary.rojewski@intel.com>]

On 2020-09-16 5:24 PM, Andy Shevchenko wrote:
> On Tue, Sep 15, 2020 at 06:29:32PM +0200, Cezary Rojewski wrote:
>> Declare base structures, registers and device routines for the catpt
>> solution. Catpt deprecates and is a direct replacement for
>> sound/soc/intel/haswell. Supports Lynxpoint and Wildcat Point both.
> 
> Few nit-picks below. Overall looks good, FWIW,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
...

>> +#include <linux/acpi.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
> 
>> +#include <linux/pci.h>
> 
> Perhaps sorted?
> 
Ack. fixed in v6.

>> +#include <sound/soc.h>
>> +#include <sound/soc-acpi.h>
>> +#include <sound/soc-acpi-intel-match.h>
>> +#include "core.h"
>> +#include "registers.h"
>> +
>> +#define CREATE_TRACE_POINTS
>> +#include "trace.h"
>> +
>> +static int __maybe_unused catpt_suspend(struct device *dev)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dev);
>> +	struct dma_chan *chan;
>> +	int ret;
>> +
>> +	chan = catpt_dma_request_config_chan(cdev);
>> +	if (IS_ERR(chan))
>> +		return PTR_ERR(chan);
>> +
>> +	memset(&cdev->dx_ctx, 0, sizeof(cdev->dx_ctx));
>> +	ret = catpt_ipc_enter_dxstate(cdev, CATPT_DX_STATE_D3, &cdev->dx_ctx);
>> +	if (ret) {
>> +		ret = CATPT_IPC_ERROR(ret);
>> +		goto exit;
>> +	}
>> +
>> +	ret = catpt_dsp_stall(cdev, true);
>> +	if (ret)
>> +		goto exit;
>> +
>> +	ret = catpt_store_memdumps(cdev, chan);
>> +	if (ret) {
>> +		dev_err(cdev->dev, "store memdumps failed: %d\n", ret);
>> +		goto exit;
>> +	}
>> +
>> +	ret = catpt_store_module_states(cdev, chan);
>> +	if (ret) {
>> +		dev_err(cdev->dev, "store module states failed: %d\n", ret);
>> +		goto exit;
>> +	}
>> +
>> +	ret = catpt_store_streams_context(cdev, chan);
>> +	if (ret) {
>> +		dev_err(cdev->dev, "store streams ctx failed: %d\n", ret);
>> +		goto exit;
>> +	}
> 
>> +exit:
> 
> I would rather name it as 'out_dma_release' or so to explain what's going to be
> done.
> 

I find more descriptive labels inviting reader into: "this is an error
path" thinking and that's why I prefer to stick with simple 'exit'. If
you think that's not a way to go, can change this.

>> +	dma_release_channel(chan);
>> +	if (ret)
>> +		return ret;
>> +	return cdev->spec->power_down(cdev);
>> +}
>> +
>> +static int __maybe_unused catpt_resume(struct device *dev)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dev);
>> +	int ret, i;
>> +
>> +	ret = cdev->spec->power_up(cdev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (!module_is_live(dev->driver->owner)) {
>> +		dev_info(dev, "module unloading, skipping fw boot\n");
>> +		return 0;
>> +	}
>> +
>> +	ret = catpt_boot_firmware(cdev, true);
>> +	if (ret) {
>> +		dev_err(cdev->dev, "boot firmware failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* reconfigure SSP devices after dx transition */
> 
> Dx ?
> 

Reworded in v6, thanks.