Re: [PATCH 15/35] ASoC: Intel: Skylake: Use dsp loading functions directly

From: Cezary Rojewski <cezary.rojewski@intel.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	alsa-devel@alsa-project.org
Cc: broonie@kernel.org, tiwai@suse.com, lgirdwood@gmail.com
Subject: Re: [PATCH 15/35] ASoC: Intel: Skylake: Use dsp loading functions directly
Date: Sat, 24 Aug 2019 11:41:42 +0200	[thread overview]
Message-ID: <a22b7910-abeb-a8be-87a9-0e6e121d054d@intel.com> (raw)
In-Reply-To: <d334cfab-34c9-a75b-598a-8527ffc89f54@linux.intel.com>

On 2019-08-23 21:17, Pierre-Louis Bossart wrote:
> 
> 
> On 8/22/19 2:04 PM, Cezary Rojewski wrote:
>> None of skl_dsp_loader_ops are actually extended as any parameter that
>> could be "extended" is already part of given function's parameter list.
>> Rather than obfustace non-derived calls with ops and dereferences, make
> 
> A typo on obfuscate could be intentional?
> 
>> use of said operation directly. Takes part in remal of
> 
> removal?
> 

Ack on both.

>> skl_dsp_loader_ops structure.
>>
>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
>> ---
>>   sound/soc/intel/skylake/bxt-sst.c       | 18 +++++++++---------
>>   sound/soc/intel/skylake/cnl-sst.c       | 10 +++++-----
>>   sound/soc/intel/skylake/skl-messages.c  | 10 +++++-----
>>   sound/soc/intel/skylake/skl-sst-cldma.c | 10 +++++-----
>>   sound/soc/intel/skylake/skl-sst-dsp.h   |  9 +++++++++
>>   5 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/sound/soc/intel/skylake/bxt-sst.c 
>> b/sound/soc/intel/skylake/bxt-sst.c
>> index a8a2783f9b37..1ca4fba0f35f 100644
>> --- a/sound/soc/intel/skylake/bxt-sst.c
>> +++ b/sound/soc/intel/skylake/bxt-sst.c
>> @@ -60,7 +60,7 @@ bxt_load_library(struct sst_dsp *ctx, struct 
>> skl_lib_info *linfo, int lib_count)
>>           if (ret < 0)
>>               goto load_library_failed;
>> -        stream_tag = ctx->dsp_ops.prepare(ctx->dev, 0x40,
>> +        stream_tag = skl_dsp_prepare(ctx->dev, 0x40,
>>                       stripped_fw.size, &dmab);
> 
> fits on one line now?
> 

Will check, thanks.

>>           if (stream_tag <= 0) {
>>               dev_err(ctx->dev, "Lib prepare DMA err: %x\n",
>> @@ -72,14 +72,14 @@ bxt_load_library(struct sst_dsp *ctx, struct 
>> skl_lib_info *linfo, int lib_count)
>>           dma_id = stream_tag - 1;
>>           memcpy(dmab.area, stripped_fw.data, stripped_fw.size);
>> -        ctx->dsp_ops.trigger(ctx->dev, true, stream_tag);
>> +        skl_dsp_trigger(ctx->dev, true, stream_tag);
>>           ret = skl_sst_ipc_load_library(&skl->ipc, dma_id, i, true);
>>           if (ret < 0)
>>               dev_err(ctx->dev, "IPC Load Lib for %s fail: %d\n",
>>                       linfo[i].name, ret);
> 
> indent?
> 

Hmm looks like this has been here before but indeed can be corrected.

>> -        ctx->dsp_ops.trigger(ctx->dev, false, stream_tag);
>> -        ctx->dsp_ops.cleanup(ctx->dev, &dmab, stream_tag);
>> +        skl_dsp_trigger(ctx->dev, false, stream_tag);
>> +        skl_dsp_cleanup(ctx->dev, &dmab, stream_tag);
>>       }
>>       return ret;
>> @@ -100,7 +100,7 @@ static int sst_bxt_prepare_fw(struct sst_dsp *ctx,
>>   {
>>       int stream_tag, ret;
>> -    stream_tag = ctx->dsp_ops.prepare(ctx->dev, 0x40, fwsize, 
>> &ctx->dmab);
>> +    stream_tag = skl_dsp_prepare(ctx->dev, 0x40, fwsize, &ctx->dmab);
>>       if (stream_tag <= 0) {
>>           dev_err(ctx->dev, "Failed to prepare DMA FW loading err: %x\n",
>>                   stream_tag);
>> @@ -162,7 +162,7 @@ static int sst_bxt_prepare_fw(struct sst_dsp *ctx,
>>       return ret;
>>   base_fw_load_failed:
>> -    ctx->dsp_ops.cleanup(ctx->dev, &ctx->dmab, stream_tag);
>> +    skl_dsp_cleanup(ctx->dev, &ctx->dmab, stream_tag);
>>       skl_dsp_core_power_down(ctx, SKL_DSP_CORE_MASK(1));
>>       skl_dsp_disable_core(ctx, SKL_DSP_CORE0_MASK);
> 
> those macros look confusing. COREx_MASK or CORE_MASK(x), choose one.
> 

Huh, haven't touched any _CORE_MASK? Seeing that you are curious, let me 
do a little sneak peak: many macros are no longer with us and soon no 
longer on upstream! That includes some files too : D
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel