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.