Re: [PATCH 1/2] ASOC: Add ADAU7118 8 Channel PDM-to-I2S/TDM Converter driver

From: "Sa, Nuno" <Nuno.Sa@analog.com>
To: "broonie@kernel.org" <broonie@kernel.org>
Cc: "mark.rutland@arm.com" <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"lars@metafoo.de" <lars@metafoo.de>,
	"tiwai@suse.com" <tiwai@suse.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>
Subject: Re: [PATCH 1/2] ASOC: Add ADAU7118 8 Channel PDM-to-I2S/TDM Converter driver
Date: Mon, 30 Sep 2019 09:44:00 +0000	[thread overview]
Message-ID: <6245f99f37c10dcec0a52344bab4b980f08e07da.camel@analog.com> (raw)
In-Reply-To: <20190926184318.GF2036@sirena.org.uk>

Hi Mark,

Thanks for the review. Some comments/doubts inline. This device was my
first contact with ASOC/sound devs so, I apologize if some of the
comments/doubts are completely wrong/trivial.

On Thu, 2019-09-26 at 11:43 -0700, Mark Brown wrote:
> 
> On Thu, Sep 26, 2019 at 09:17:06AM +0200, Nuno Sá wrote:
> 
> > --- /dev/null
> > +++ b/sound/soc/codecs/adau7118-hw.c
> > @@ -0,0 +1,43 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Analog Devices ADAU7118 8 channel PDM-to-I2S/TDM Converter
> > Standalone Hw
> > + * driver
> > + *
> > + * Copyright 2019 Analog Devices Inc.
> > + */
> 
> Please make the entire comment a C++ style one in the .c files so
> things look more intentional.
> 
> > +	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
> > +	case SND_SOC_DAIFMT_I2S:
> > +		ret = snd_soc_component_update_bits(dai->component,
> > +						    ADAU7118_REG_SPT_CT
> > RL1,
> > +						    ADAU7118_DATA_FMT_M
> > ASK,
> > +						    ADAU7118_DATA_FMT(0
> > ));
> > +		break;
> > +	case SND_SOC_DAIFMT_LEFT_J:
> > +		ret = snd_soc_component_update_bits(dai->component,
> > +						    ADAU7118_REG_SPT_CT
> > RL1,
> > +						    ADAU7118_DATA_FMT_M
> > ASK,
> > +						    ADAU7118_DATA_FMT(1
> > ));
> > +		break;
> > +	case SND_SOC_DAIFMT_RIGHT_J:
> > +		st->right_j = true;
> > +		break;
> 
> Don't we need to set any register values here?

The register set is done in adau7118_hw_params(). For right
justification the device can delay bclck by 8, 12 or 16. So, We need to
know the data_width to check if we can apply the configuration.

> > +
> > +	return ret < 0 ? ret : 0;
> > +}
> 
> Please don't use the ternery operator like this, it just makes
> things harder to read - write normal if conditional statements.

ack.

> > +	case SND_SOC_BIAS_STANDBY:
> > +		if (snd_soc_component_get_bias_level(component) ==
> > +							SND_SOC_BIAS_OF
> > F) {
> > +			if (!st->iovdd)
> > +				return 0;
> 
> This is broken, the device will always require power so it should
> always control the regulators.

The reason why I made this optional was to let the user assume that, in
some cases, the supply can be always present (and not controlled by the
kernel) and, in those cases, he would not have to care about giving
regulators nodes in devicetree. Furthermore, the driver would not have
to care about enabling/disabling  regulators. Is this not a valid
scenario? Or is it that, for this kind of devices it does not really
make sense (which I think it doesn't) to have them always powered, so
we just assume a regulator is needed (and in the unlikely scenario we
don't have one, the user just provides a fixed-regulator)?

> > +static int adau7118_suspend(struct snd_soc_component *component)
> > +{
> > +	return snd_soc_component_force_bias_level(component,
> > SND_SOC_BIAS_OFF);
> > +}
> > +
> > +static int adau7118_resume(struct snd_soc_component *component)
> > +{
> > +	return snd_soc_component_force_bias_level(component,
> > +						  SND_SOC_BIAS_STANDBY)
> > ;
> > +}
> 
> Let DAPM do this for you, there's no substantial delays on power
> on so you're probably best just setting idle_bias_off.

So, this means dropping resume/suspend and to not set idle_bias_on,
right?

> > +static int adau7118_regulator_setup(struct adau7118_data *st)
> > +{
> > +	int ret = 0;
> > +
> > +	st->iovdd = devm_regulator_get_optional(st->dev, "IOVDD");
> > +	if (!IS_ERR(st->iovdd)) {
> 
> Unless the device can operate with supplies physically absent it
> should not be requesting regulators as optional, this breaks your
> error handling especially with probe deferral which is a fairly
> common case.

Just for my understanding (most likely I'm missing something obvious),
why would I have issues with the error handling in probe deferral?

Thanks!
Nuno Sá
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel