On Sat, Sep 19, 2015 at 09:56:12AM -0700, Mark Brown wrote:
On Fri, Sep 11, 2015 at 12:45:00PM +0100, Mark Brown wrote:
On Thu, Sep 03, 2015 at 01:44:54PM +0530, Vinod Koul wrote:
I was wondering if you have any feedback on this series ?
Please don't send content free pings (as I'm sure I've mentioned before). I've not looked at this yet because it's another large, invasive patch series which you sent at the very end of the development cycle.
So, I've now looked at these, though not fully. To expand on the above a bit: these are big bits of very device specific code that's really hard to read and understand and when I do read it I'm seeing similar issues again and again both in terms of big picture stuff and the trivial details (including the inconsistencies in things like commenting style that I'm not specifically pulling up). I'm really worried that I'm often seeing fragments of things that may or may not work well together.
These Intel devices not only have a lot more code than most devices do, they also get a lot more exposure than most devices do to end users so I actually have to try to make some effort to understand what's going on and can't skimp on review even though I feel like I am. I'm really concerned about where we're heading here and how robustly the results are going to work.
This all really slows down review, both directly in requiring detailed checks of hard to follow the code and as a result of making almost all other code review seem much more productive.
As well as the low level code quality issues it would really help if rather than continually sending large blocks of functionality we were starting from something that was a minimally viable but functional system and then extending that. That should be a lot easier to follow.
Thanks Mark for the review,
I wouldn't have pinged but had been more than few weeks so was wondering if this got missed. I did realise you were travelling and merge window came up, but still thought Mark's typical turnaound is 1-2 weeks so was expecting something. I do understand this take time but no harm in checking if it is beyond usual window one expects.
Now on the code due to inherent nature of HW (we have HDA + DSP + I2S!!!) and SW scalability, we do need lots of code for basic audio. Btw _this_ patch series is the last one to get basic minimal audio out of SKL. After this we will add machine and then features...
We did decide to use DPCM, DAPM and topology here to solve manging the DSP using these frameworks. yes hard coding a simple mixer and two pipes could have made code look easy to follow but then we would be redoing those bits as we scale these to defferent designs.
And to provide what we are trying here I have attached the SKL Topology conf file to give a picture of topology we are trying to build as a reference.
On code issues, I will fix the style and wrappers issues you raised and other instances where this might be required.
Also where you feel something is missing please do ask. Sometimes things are perceived intuitive for us as we are doing this for quire some time but may not be so for fresh eyes :)
Thanks