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

[Sa, Nuno]

On Mon, 2019-09-30 at 16:11 +0100, Mark Brown wrote:
> 
> 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.

Ok, got it. Looking at `_regulator_get` I can see that a dummy
regulator is provided, when `NORMAL_GET` is used, even if one was not
given. So I think I get now the usage of
`devm_regulator_get_optional`.  As you said, supply voltages are not
optional, the optional _get_ should be used for things that are really
optional like some parts that can use external vs internal vref.

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