Re: [PATCH 2/2] ASoC: codec: tlv320adc3xxx: New codec driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3176 bytes --]

On Mon, Oct 04, 2021 at 06:21:10PM +0200, Ricard Wanderlof wrote:
> On Mon, 4 Oct 2021, Mark Brown wrote:
> > On Mon, Oct 04, 2021 at 11:19:21AM +0200, Ricard Wanderlof wrote:

> > > +++ b/sound/soc/codecs/tlv320adc3xxx.c
> > > @@ -0,0 +1,1239 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Based on sound/soc/codecs/tlv320aic3x.c by  Vladimir Barinov

> > Please make the entire comment a C++ one so things look more
> > intentional.

> Ok, I'll change this. I was trying to follow the style of existing 
> drivers, as the majority seem to have C style header comments even though 
> the SPDX line uses C++. I'm now guessing this is for legacy reasons 
> (minimizing changes in existing drivers when the SPDX line was added)?

A lot of SPDX stuff was done mechanically.

> > > +static bool adc3xxx_volatile_reg(struct device *dev, unsigned int reg)
> > > +{
> > > +	switch (reg) {
> > > +	case PAGE_SELECT: /* required by regmap implementation */

> > This is not required by regmap.

> Ok, I'll remove it. The regmap code was taken from tlv320aic3x.c which has 
> a similar paging structure, but I see now that other drivers don't have 
> this, so I guess it's not necessary for tlv320aic3x.c either and is there 
> either erroneously or because it once was necessary?

It's probably an error in either the description of the pages or just
the driver.

> > > +static const char * const micbias_voltage[] = { "off", "2V", "2.5V", 
> > > "AVDD" };

> > This should be configured in the DT, it's going to be a property of the
> > electrical design of the system.

> I can very well imagine that this something that should be runtime 
> userspace configurable. In fact where I work we have had products where 
> the bias voltage (for an externally connected microphone) could be 
> configured by the end user, (although not for this specific driver quite 
> honestly, we have had the need for hardware engineers to change it runtime 
> during circuit verification though).

> Would it be ok to have this configurable both in the DT as well as using a 
> control? Or should it be implemented in another way, such as a number of 
> pre set voltages that are selected between using a control?

It seems like a lot less work to just not have the runtime control and
let someone who needs it figure out how to represent it to userspace.
Something that's basically a backdoor for validation doesn't seem
persuasive, validation often wants to do things we actively wish to
prevent at runtime.

> > > +static const struct snd_kcontrol_new adc3xxx_snd_controls[] = {
> > > +	SOC_DOUBLE_R_TLV("PGA Gain Volume", LEFT_APGA_CTRL, RIGHT_APGA_CTRL,
> > > +			 0, 80, 0, pga_tlv),

> > s/Gain //

> But this would mean that the resulting control will be exposed as 
> "PGA" when viewed in amixer as an scontrol which seems less than intutive, 
> but perhaps there's nothing that can be done about that (other than 
> perhaps expanding PGA to Programmable Gain Amplifier perhaps)?

The TLV information should mean that UIs can figure out to represent it
as a volume control which should be clear enough.

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