lkml.org 
[lkml]   [2014]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform
    From
    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.

    --
    Thanks.
    -- Max


    \
     
     \ /
      Last update: 2014-10-29 16:01    [W:3.058 / U:1.356 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site