On Wed, Oct 29, 2014 at 12:34 AM, Mark Brown broonie@kernel.org wrote:
On Tue, Oct 28, 2014 at 09:11:34PM +0300, Max Filippov wrote:
On Tue, Oct 28, 2014 at 8:38 PM, Mark Brown broonie@kernel.org wrote:
You *really* need to explain how it's supposed to work - right now it's not at all obvious, like I say the fact that this is a rarely used idiom is not helping. For example when we tear down the stream we just assign the pointer in _stop() but don't bother with a sync until the stream is closed - why?
Because we can't wait in stop and syncing is not time critical, we can do it any time before the stream becomes invalid.
To be clear: the important part is that someone reading the code can understand what's going on.
Ok, I'll change it.
hw_params callback can change MCLK rate, so it has to disable and enable the clock anyway, and since enable can fail it does not guarantee that the clock will be left in the same state. Or should I adjust MCLK rate w/o disabling the clock?
So yet again: why not just enable the clock only when the device is in use? If it's being configured it stands to reason that the device isn't actively in use...
Mark, I don't get it, sorry ): My clock synthesizer is I2C controlled, so I can't prepare/unprepare it in the trigger callback. When should I do it?
Runtime PM is the normal way of doing it.
Ok, thanks.
The level field in the control register is 4 bit wide, so the allowed range of level is 0..15. FIFO size is 8192 entries, level = 1 corresponds to FIFO size / 2, level = 14 -- to FIFO size of 0. I guess this function won't get period_size of 0?
So if the IP gets changed and the code gets blown up this could well explode then... which doesn't seem entirely unlikely considering this is a FPGA platform so presumably this is easy to update. To repeat this is about clarity and the code looking like it's probably hiding bugs as much as if the code actually works if you really sit down and study it.
The calculation does not depend on the actual hardware, but on the constant definitions in the same file. They need to be updated if the hardware changes. I'll try to rewrite it in a cleaner way.
Right, my point is that if someone changes the hardware they'll just update the constants and then things will break.
Ok, I've rewritten it in a safer manner.