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

[Ricard Wanderlof]

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