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:
signature.asc
Description: PGP signature
