[alsa-devel] [PATCH 2/2] ASoC: sgtl5000: clean up sgtl5000_enable_regulators()

[Shawn Guo]

On Fri, Dec 13, 2013 at 10:29:43AM +0000, Li.Xiubo at freescale.com wrote:
> Hi Shawn,
> 
> > -	ret = regulator_bulk_get(codec->dev, ARRAY_SIZE(sgtl5000->supplies),
> > -				sgtl5000->supplies);
> > -	if (!ret)
> > -		external_vddd = 1;
> > -	else {
> > +	/* External VDDD only works before revision 0x11 */
> > +	if (sgtl5000->revision < 0x11) {
> > +		vddd = regulator_get_optional(codec->dev, "VDDD");
> > +		if (IS_ERR(vddd)) {
> > +			/* See if it's just not registered yet */
> > +			if (PTR_ERR(vddd) == -EPROBE_DEFER)
> > +				return -EPROBE_DEFER;
> > +		} else {
> > +			external_vddd = 1;
> > +			regulator_put(vddd);
> > +		}
> 
> Shouldn't we handle other errors ?

We do not really care other errors.  In case that an external VDDD is
present on hardware design but regulator_get_optional("VDDD") still
fails for some other reason, we should turn to use the internal LDO
anyway rather than simply failing out.

> We can see that from the regulator_get_optional()-->_regulator_get(), there
> are many other errors maybe returned. Not considering other error cases, If
> the dt is used here, only ERR_PTR(-ENODEV) will be returned if the external
> VDDD is absent, and only this case the ERR_PTR(_ENODEV) error will be returned,
> Or if dt is not used and there has no external VDDD was found, the
> ERR_PTR(-EPROBE_DEFER) will be returned.
> 
> So, if the external VDDD is not used, only ERR_PTR(-ENODEV) and ERR_PTR(-EPROBE_DEFER)
> maybe returned ignoring other errors.
> 
> How about the following ? 
> -----------------
> +	if (IS_ERR(vddd)) {
> +		/* See if it's just not registered yet */
> +		if (PTR_ERR(vddd) != -ENODEV)
> +			return PTR_ERR(vddd);

Just for example, if PTR_ERR(vddd) is -EBUSY here, instead of failing
out, why do not we switch to internal LDO for continuing the probe?

Shawn

> +	} else {
> +		external_vddd = 1;
> +		regulator_put(vddd);
> +	}
> -----------------