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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2767 bytes --]

On Mon, Sep 30, 2019 at 09:44:00AM +0000, Sa, Nuno wrote:
> On Thu, 2019-09-26 at 11:43 -0700, Mark Brown wrote:

> > > +	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.

OK.

> > > +	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

Have you tried doing that?  Notice how the regulator API subtitutes in a
dummy regulator for you and the driver works fine without custom code.

> 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

It's not a valid scenario in driver code - the driver shouldn't be
randomly ignoring errors and hoping the errors were deliberate rather
than indiciations of real problems.

> > > +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?

Right.  Like I say it looks like your power up path is fast enough for
this.

> > > +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?

Actually it does look like you handle this correctly further down, the
normal pattern would have been to have the error handling inside the if
here and not indent the rest of the success path so I misread it.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel