On Wed, Oct 29, 2014 at 12:34 AM, Mark Brown <broonie@xxxxxxxxxx> 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@xxxxxxxxxx> 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. -- Thanks. -- Max -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
