On Fri, Sep 25, 2015 at 05:48:23PM -0400, Alex Deucher wrote:
From: Maruthi Srinivas Bayyavarapu Maruthi.Bayyavarapu@amd.com
Vendor specific quirk was added to:
- Support AMD platform which has two dwc controllers with different base address for playback and capture. Also, I2S_COMP_PARAM_* registers offsets differed.
- 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.
--- 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.
- 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.