On Thu, Dec 13, 2018 at 4:31 PM Mark Brown <broonie@xxxxxxxxxx> wrote:
>
> On Thu, Dec 13, 2018 at 10:20:47AM +0000, Cosmin Samoila wrote:
> > On Mi, 2018-12-12 at 18:14 +0000, Mark Brown wrote:
>
> > > > +static const char * const micfil_hwvad_rate[] = {
> > > > + "48KHz", "44.1KHz",
> > > > +};
>
> > > > +static const int micfil_hwvad_rate_ints[] = {
> > > > + 48000, 44100,
> > > > +};
> > > Can the driver not determine this automatically from sample rates?
>
> > I think I could add "48000", "44100" instead of "48KHz", "44.1KHz" and
> > use kstrtol()
>
> No, you're missing the point - why not set the sample rate based on the
> sample rate the interface is running at?
Hmm, definitely we somehow miss the point here. So what happens
if we only do Voice Activity Detection (VAD) ?
This means that there is no sample rate yet configured, so for this reason
we get the sampling rate as above via ALSA kcontrol interface.
My feeling is that somehow we will need to create maybe another type
of stream (VAD?).
This is a thing that we need to think. Suggestions are welcomed!
>
> > > > +static const char * const micfil_clk_src_texts[] = {
> > > > + "Auto", "AudioPLL1", "AudioPLL2", "ExtClk3",
> > > > +};
>
> > > Is this something that should be user selectable or is it going to be
> > > controlled by the board design?
>
> > User should be able to select the clock source since, in theory,
> > hardware could support any custom rate as long as you can provide the
> > clock on extclk.
>
> What I'm saying is that this should not be selectable by the user at
> runtime. It's not like the user is going to open their system and start
> soldering links onto the board or anything.
Of course, so would this be a better option to select it from dts?
>
> > > What happens if this gets changed while a stream is active - the user
> > > will think they changed the configuration but it won't take effect
> > > until
> > > the next stream is started AFAICT?
>
> > If the stream is active, the configuration will indeed take efect on
> > the next stream but this is the desired behavior. If we would want to
> > do the config on the fly, we should use sync functions that also resets
> > the pdm module which is not what we intend.
> > User must first configure the module then start the recording since
> > this seems to be the natural flow.
>
> If the user can't reconfigure the stream while it's running the user
> shouldn't be able to reconfigure the stream while it's running - we
> should block the change if it can't be implemented. Then the user can
> make the change while the interface is idle and and
>
> > > Are you *sure* you need to and want to use atomic_set() here and that
> > > there's no race conditions resulting from trying to use an atomic
> > > variable? It's much simpler and clearer to use mutexes, if for some
> > > reason atomic variables make sense then the reasoning needs to be
> > > clearly documented as they're quite tricky and easy to break or
> > > misunderstand.
We can go back to mutexes no problem, but we thought that atomic vars
are lighter.
>
> > We want to keep track of the recording state and the hwvad state since
> > recording and voice detection can work in parallel. The main reason why
> > we want to keep track is because the recording and hwvad share the same
> > clock and we should not touch the clock when one or another is on.
> > Another restriction is that we want to make sure we use the same rate
> > for recording and voice detection when doing it in parallel.
> > This was the only solution we found viable at that time and it worked
> > in any supported scenarios but we are open for suggestions if the
> > functionality is kept and code will have a better quality.
>
> This explains what the data is but not why you have chosen to use
> atomics to do this; what other concurrency primitives were considered,
> why were they rejected and what's the analysis that shows how the use of
> atomics is safe?
We first used mutexes but I suggested switching to atomic vars because
AFAIK they are lighter than mutexes.
