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
--
Ricard Wolf Wanderlof ricardw(at)axis.com
Axis Communications AB, Lund, Sweden www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30
