iio and regulator api usage

iio and regulator api usage

[Gregor Boirie]

Hi all,

After Jonathan's ms5611 review, I digged a bit deeper into regulator API.
I must confess it looks rather confusing to me, especially with regard to
optional regulator support.

Most often, IIO drivers implement the following scheme at probing time to
enable an optional regulator:
</code>
/* At probing time */
void mydriver_power_enable(struct iio_dev *indio_dev)
{
     struct mydriver_priv *priv = iio_priv(indio_dev);

     /* Regulators not mandatory, but if requested we should enable them. */
     priv->vdd = devm_regulator_get_optional(indio_dev->dev.parent, "vdd");
     if (!IS_ERR(priv->vdd)) {
         int err = regulator_enable(priv->vdd);
         if (err != 0)
             dev_warn(&indio_dev->dev,
                  "Failed to enable specified Vdd supply\n");
     }

     /* Keep going */
     ...
}

/* At removal time */
void mydriver_power_disable(struct iio_dev *indio_dev)
{
     struct mydriver_priv *priv = iio_priv(indio_dev);

     ...
     if (!IS_ERR(priv->vdd))
         regulator_disable(priv->vdd);
     ...
}
<code/>

A problem may arise with this piece of code: when 
devm_regulator_get_optional
returns -EPROBE_DEFER, driver will go on initializing with no regulator 
instead
of deferring their probing operations at a later time as it should (to wait
regulator initialization completion).
Although related to real troubles, other error codes are also ignored 
(EPERM,
EBUSY, ENOMEM...), which is even worse.
 From my understanding, optional regulator usage should be enforced when 
present
and ignored if not there. The only error code which may be ignored should be
ENODEV when the regulator is optional.
This is clearly not implemented this way in the above example.

You can see such code in:
drivers/adc/max1363.c
drivers/common/st_sensors/st_sensors_core.c
drivers/dac/ad5686.c

The same seems to happen when using the alternate devm_regulator_get for 
optional
usage purposes as in:
drivers/iio/amplifiers/ad8366.c
drivers/iio/dac/ad5624r_spi.c
and others...

So here is my simple question: how should I implement optional regulator 
support ?

Thanks for sharing your time and knowledge. Regards,
Grégor.

Re: iio and regulator api usage

[Lars-Peter Clausen]

On 03/15/2016 10:29 AM, Gregor Boirie wrote:
> Hi all,
> 
> After Jonathan's ms5611 review, I digged a bit deeper into regulator API.
> I must confess it looks rather confusing to me, especially with regard to
> optional regulator support.
> 
> Most often, IIO drivers implement the following scheme at probing time to
> enable an optional regulator:
> </code>
> /* At probing time */
> void mydriver_power_enable(struct iio_dev *indio_dev)
> {
>     struct mydriver_priv *priv = iio_priv(indio_dev);
> 
>     /* Regulators not mandatory, but if requested we should enable them. */
>     priv->vdd = devm_regulator_get_optional(indio_dev->dev.parent, "vdd");
>     if (!IS_ERR(priv->vdd)) {
>         int err = regulator_enable(priv->vdd);
>         if (err != 0)
>             dev_warn(&indio_dev->dev,
>                  "Failed to enable specified Vdd supply\n");
>     }
> 
>     /* Keep going */
>     ...
> }
> 
> /* At removal time */
> void mydriver_power_disable(struct iio_dev *indio_dev)
> {
>     struct mydriver_priv *priv = iio_priv(indio_dev);
> 
>     ...
>     if (!IS_ERR(priv->vdd))
>         regulator_disable(priv->vdd);
>     ...
> }
> <code/>
> 
> A problem may arise with this piece of code: when devm_regulator_get_optional
> returns -EPROBE_DEFER, driver will go on initializing with no regulator instead
> of deferring their probing operations at a later time as it should (to wait
> regulator initialization completion).
> Although related to real troubles, other error codes are also ignored (EPERM,
> EBUSY, ENOMEM...), which is even worse.
> From my understanding, optional regulator usage should be enforced when present
> and ignored if not there. The only error code which may be ignored should be
> ENODEV when the regulator is optional.
> This is clearly not implemented this way in the above example.
> 
> You can see such code in:
> drivers/adc/max1363.c
> drivers/common/st_sensors/st_sensors_core.c
> drivers/dac/ad5686.c
> 
> The same seems to happen when using the alternate devm_regulator_get for
> optional
> usage purposes as in:
> drivers/iio/amplifiers/ad8366.c
> drivers/iio/dac/ad5624r_spi.c
> and others...
> 
> So here is my simple question: how should I implement optional regulator
> support ?

The only error code that should be ignored is ENODEV. Everything else should
be propagated further up the stack.

- Lars