Thanks for your prompt review Mark!
On Mon, 4 Oct 2021, Mark Brown wrote:
On Mon, Oct 04, 2021 at 11:19:21AM +0200, Ricard Wanderlof wrote:
There's an awful lot of code in here that just doesn't really look like a normal Linux driver or follow conventions for ASoC, just from a quick visual overview without actually reading anything it's fairly clear the driver needs quite a bit of a refresh for mainline.
To a large extent this is because I've retained as much of the original driver as possible, only changing things that seemed absolutely necessary. However your remark makes it clear that this is a less than successful strategy so I'll make a general overhaul based on your remarks, and resubmit.
A few specific questions below, however:
+++ 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)?
+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?
+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?
+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)?
/* Switch on NADC Divider */snd_soc_component_update_bits(component, ADC_NADC,ENABLE_NADC, ENABLE_NADC);/* Switch on MADC Divider */snd_soc_component_update_bits(component, ADC_MADC,ENABLE_MADC, ENABLE_MADC);Why are these not managed through DAPM?
The simple answer is that the driver was originally written like this and I saw no reason to change it.
- adc3xxx->mclk = devm_clk_get(dev, NULL);
- if (IS_ERR(adc3xxx->mclk)) {
if (PTR_ERR(adc3xxx->mclk) != -ENOENT)return PTR_ERR(adc3xxx->mclk);/* Make a note that there is no mclk specified. */adc3xxx->mclk = NULL;Does the device work without a MCLK?
Yes, if so configured it can use the BCLK as the source clock for the clock generation using the internal PLL.
/Ricard