Re: [RFC 2/2] ASoC: Add MICFIL SoC Digital Audio Interface driver.

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3935 bytes --]

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?

> > > +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.

> > 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 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?

> > > +	/* set default gain to max_gain */
> > > +	regmap_write(micfil->regmap, REG_MICFIL_OUT_CTRL,
> > > 0x77777777);
> > > +	for (i = 0; i < 8; i++)
> > > +		micfil->channel_gain[i] = 0xF;

> > I'm assuming the hardware has no defaults here but if we've got to
> > pick
> > a gain wouldn't a low gain be less likely to blast out someone's
> > eardrums than a maximum gain?

> Fair enough. We did this because the maximum output volume isn't that
> high but I guess we should set it to minimum and user should set volume
> via amixer.

The ideal thing is to just not set it at all and use whatever the
hardware designers picked.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]