[alsa-devel] [PATCH 2/5] ASoC : dwc : support dw i2s in AMD platform

[maruthi srinivas]

On Mon, Oct 5, 2015 at 9:11 PM, Mark Brown <broonie at kernel.org> wrote:
> On Fri, Sep 25, 2015 at 05:48:23PM -0400, Alex Deucher wrote:
>> From: Maruthi Srinivas Bayyavarapu <Maruthi.Bayyavarapu at amd.com>
>>
>> Vendor specific quirk was added to:
>> 1. Support AMD platform which has two dwc controllers with different
>>    base address for playback and capture. Also, I2S_COMP_PARAM_*
>>    registers offsets differed.
>
>> 2. Resume audio which was active before system suspend.
>>    After 'resume', dwc need to be reconfigured with params
>>    configured during 'hw_params' callback. With this, audio usecase
>>    continues from where it got stopped because of 'suspend'.
>
> This is hard to review since there are several different changes in
> here, one change per commit is muuch more helpful.  As is I'm not 100%
> sure exactly what each bit of the change is intended to do.  One example
> here is that a separate patch splitting pbase and cbase would be really
> helpful.

Ok, I will split into multiple patches and send again.

>
>> --- a/include/sound/designware_i2s.h
>> +++ b/include/sound/designware_i2s.h
>> @@ -44,6 +44,9 @@ struct i2s_platform_data {
>>       int channel;
>>       u32 snd_fmts;
>>       u32 snd_rates;
>> +     #define DW_I2S_VENDOR_AMD (1 << 0)
>> +     unsigned int quirks;
>
> I would expect to see a set of quirks with the AMD devices selecting
> those that apply to them rather than just a per vendor define - this
> is more generic and more maintainable long term.  AMD may require
> different sets of quirks with some future device and systems from other
> vendors may require some but not all of the quirks that AMD requires.

Agreed. I will send a revised patch.

>
>> +     if (dev->quirks & DW_I2S_VENDOR_AMD) {
>> +             comp1 = i2s_read_reg(dev->i2s_pbase, 0x124);
>> +             comp2 = i2s_read_reg(dev->i2s_pbase, 0x120);
>
> Please add defines for these registers.  Rather than having the quirk
> code throughout the driver it might be clearer to have the comp1 and
> comp2 register offsets stored in the driver data so you can just do the
> actual quirk once at probe time.

Agreed. I will send a revised patch.

>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>