Re: [PATCH 4/6] ASoC: SOF: Intel: byt: Refactor fw ready / mem windows creation

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

On 8/7/19 2:07 PM, Cezary Rojewski wrote:
> On 2019-08-07 17:02, Pierre-Louis Bossart wrote:
>> From: Daniel Baluta <daniel.baluta@nxp.com>
> 
>> So we are basically moving code from intel/byt.c to loader.c keeping
>> in mind that mbox_offset is a per platform constant so we need to
>> use newly introduced snd_sof_dsp_get_mailbox_offset /
>> snd_sof_dsp_get_window_offset in order to get the correct
>> mbox offset / window offset value.
> 
> You've already explained your goal. These details are unnecessary.

They don't hurt and help explain the approach.

> 
>>
>> Also, bar is a per platform constant so we use snd_sof_dsp_get_bar_index
>> instead of the hardcoded BYT_DSP_BAR.
>>
>> Signed-off-by: Daniel Baluta <daniel.baluta@nxp.com>
>> Signed-off-by: Pierre-Louis Bossart 
>> <pierre-louis.bossart@linux.intel.com>
>> ---
>>   sound/soc/sof/intel/byt.c | 164 +++++--------------------------------
>>   sound/soc/sof/loader.c    | 168 ++++++++++++++++++++++++++++++++++++++
>>   sound/soc/sof/sof-priv.h  |   2 +
>>   3 files changed, 189 insertions(+), 145 deletions(-)
> 
> Hmm, even the commit message mentions two steps, not one. Splitting this 
> commit into two - introduction of new generic functions and byt 
> alignment towards the newly added approach - seems reasonable. Bdw & hda 
> followups already make good examples.

The last two just remove the duplicate code and align on using the 
common helpers.
In the initial step we still need to move the code from baytrail to the 
common function. Doing it in two steps doesn't bring much added value 
IMO. To preserve git bisect support, you'd need to add a new common 
code, then remove the baytrail one in a follow-up patch. It'd make it 
less self-explanatory where this new code comes from.

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel