On 03/12/2016 12:04 PM, Jonathan Cameron wrote:
On 09/03/16 11:34, Gregor Boirie wrote:
When not compiled-in, regulator layer will return a NULL pointer when
trying to get a reference to any regulator using devm_regulator_get(). As
IS_ERR() does not consider this an error, the ms5611 probing operation will
try to enable a NULL regulator, which will invariably cause a kernel
crash.
This patch fixes this situation by using devm_regulator_get_optional()
instead of devm_regulator_get().
Fixes: 3145229f9191 ("iio:pressure:ms5611: power regulator support")
Signed-off-by: Gregor Boirie <gregor.boirie@xxxxxxxxxx>
I've been thinking a bit more about these cases and think we are missusing
the optional regulators. If regulator support is present and a non optional
register is requested then it will feed a dummy regulator back.
The intent of this is to handle the case where an non optional regulator is
needed, but not specified (typically because it is always on).
My intent was:
* if regulator support not built-in, keep going ;
* else :
* if regulator not declared (platform or device tree), keep going ;
* else :
* if regulator not properly declared, error ;
* else enable it, then go one initializing.
So, I suppose dummy regulators should be allowed to support all
possible cases (use devm_regulator_get()).
Have I missed something ?
So the question is how should we handle the regulators not built in case.
If you do call devm_regulator_get you'll get a null pointer.
Without regulators built in regulator_enable will be fine (returns 0)
as does regulator disable.
So I'm unclear on where the crash is coming from.
Probably from the fact that I use an up to date IIO subsystem ported onto
an old kernel (3.10.x), which rather complicates testing with regard to
other subsystem dependencies.
Also interesting is this driver enables the regulator but never disables
it (which it should).
Right. I thought regulator_put() implicitly disabled the regulator, which
is wrong...
I was clearly half asleep when reviewing this.
Sorry!
Well, I wasn't that awake either. Sorry for the noise.
Grégor.
Jonathan
---
drivers/iio/pressure/ms5611_core.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/pressure/ms5611_core.c b/drivers/iio/pressure/ms5611_core.c
index 37dbc04..a2a871b 100644
--- a/drivers/iio/pressure/ms5611_core.c
+++ b/drivers/iio/pressure/ms5611_core.c
@@ -387,8 +387,8 @@ static const struct iio_info ms5611_info = {
static int ms5611_init(struct iio_dev *indio_dev)
{
int ret;
- struct regulator *vdd = devm_regulator_get(indio_dev->dev.parent,
- "vdd");
+ struct regulator *vdd =
+ devm_regulator_get_optional(indio_dev->dev.parent, "vdd");
/* Enable attached regulator if any. */
if (!IS_ERR(vdd)) {
@@ -398,6 +398,10 @@ static int ms5611_init(struct iio_dev *indio_dev)
"failed to enable Vdd supply: %d\n", ret);
return ret;
}
+ } else {
+ ret = PTR_ERR(vdd);
+ if (ret != -ENODEV)
+ return ret;
}
ret = ms5611_reset(indio_dev);
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
