Re: [PATCH 04/10 v4] iio: pressure: bmp280: support supply regulators

[Jonathan Cameron <jic23@kernel.org>]

On 30/06/16 02:48, Linus Walleij wrote:
> The BMP085/BMP180/BMP280 is supplied with two power sources:
> VDDA (analog power) and VDDD (digital power). As these may come
> from regulators (as on the APQ8060 Dragonboard) we need the driver
> to attempt to fetch and enable these regulators.
> 
> We FAIL if we cannot: boards should either define:
> - Proper regulators if present
> - Define fixed regulators if power is hardwired to the component
> - Rely on dummy regulators (will be present on all DT systems and
>   any boardfile system that calls regulator_has_full_constraints().
> 
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Goes to show how well I read this earlier. Coincidentally it
doesn't apply cleanly due to a fix that's made it into the togreg
branch via a fast forward merge.

This one introduces a race, by potentially turning off the
regulators before we have removed the various interfaces that
allow for reading from the iio device.

You need to change the devm_iio_device_register into the non
devm version and unwind it in the remove function.

Jonathan
> ---
> ChangeLog v3->v4:
> - Fix missing "regulator" string in enablement message.
> - Fix BMP058->BMP085 in commit message
> ChangeLog v2->v3:
> - Encode a start-up time for each sensor type (from the datasheets)
>   this is necessary when coldstarting the sensor with a regulator
>   to be sure it is on before we try to use it.
> - Introduce a try/catch-style set of goto clauses in the probe
>   function to make sure the regulators are properly disabled.
> ChangeLog v1->v2:
> - Make the regulators non-optional: an optional supply is one you
>   can choose not to supply ELECTRICALLY as in an internal charge
>   pump taking its place or so.
> - Bail out if not present and add some notices to the commit that
>   boards need to think about their regulator usage.
> - Make sure we disable the regulators, and introduce a .remove()
>   call to do that if/when the module is removed.
> ---
>  drivers/iio/pressure/bmp280.c | 64 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 60 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/pressure/bmp280.c b/drivers/iio/pressure/bmp280.c
> index 1618968f8889..5245bc598187 100644
> --- a/drivers/iio/pressure/bmp280.c
> +++ b/drivers/iio/pressure/bmp280.c
> @@ -24,6 +24,7 @@
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
>  #include <linux/gpio/consumer.h>
> +#include <linux/regulator/consumer.h>
>  
>  /* BMP280 specific registers */
>  #define BMP280_REG_HUMIDITY_LSB		0xFE
> @@ -124,6 +125,9 @@ struct bmp280_data {
>  	struct mutex lock;
>  	struct regmap *regmap;
>  	const struct bmp280_chip_info *chip_info;
> +	struct regulator *vddd;
> +	struct regulator *vdda;
> +	unsigned int start_up_time; /* in milliseconds */
>  
>  	/* log of base 2 of oversampling rate */
>  	u8 oversampling_press;
> @@ -1047,12 +1051,14 @@ static int bmp280_probe(struct i2c_client *client,
>  		data->chip_info = &bmp180_chip_info;
>  		data->oversampling_press = ilog2(8);
>  		data->oversampling_temp = ilog2(1);
> +		data->start_up_time = 10;
>  		break;
>  	case BMP280_CHIP_ID:
>  		indio_dev->num_channels = 2;
>  		data->chip_info = &bmp280_chip_info;
>  		data->oversampling_press = ilog2(16);
>  		data->oversampling_temp = ilog2(2);
> +		data->start_up_time = 2;
>  		break;
>  	case BME280_CHIP_ID:
>  		indio_dev->num_channels = 3;
> @@ -1060,11 +1066,37 @@ static int bmp280_probe(struct i2c_client *client,
>  		data->oversampling_press = ilog2(16);
>  		data->oversampling_humid = ilog2(16);
>  		data->oversampling_temp = ilog2(2);
> +		data->start_up_time = 2;
>  		break;
>  	default:
>  		return -EINVAL;
>  	}
>  
> +	/* Bring up regulators */
> +	data->vddd = devm_regulator_get(&client->dev, "vddd");
> +	if (IS_ERR(data->vddd)) {
> +		dev_err(&client->dev, "failed to get VDDD regulator\n");
> +		return PTR_ERR(data->vddd);
> +	}
> +	ret = regulator_enable(data->vddd);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable VDDD regulator\n");
> +		return ret;
> +	}
> +	data->vdda = devm_regulator_get(&client->dev, "vdda");
> +	if (IS_ERR(data->vdda)) {
> +		dev_err(&client->dev, "failed to get VDDA regulator\n");
> +		ret = PTR_ERR(data->vddd);
> +		goto out_disable_vddd;
> +	}
> +	ret = regulator_enable(data->vdda);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to enable VDDA regulator\n");
> +		goto out_disable_vddd;
> +	}
> +	/* Wait to make sure we started up properly */
> +	mdelay(data->start_up_time);
> +
>  	/* Bring chip out of reset if there is an assigned GPIO line */
>  	gpiod = devm_gpiod_get(&client->dev, "reset", GPIOD_OUT_HIGH);
>  	/* Deassert the signal */
> @@ -1077,7 +1109,8 @@ static int bmp280_probe(struct i2c_client *client,
>  					data->chip_info->regmap_config);
>  	if (IS_ERR(data->regmap)) {
>  		dev_err(&client->dev, "failed to allocate register map\n");
> -		return PTR_ERR(data->regmap);
> +		ret = PTR_ERR(data->regmap);
> +		goto out_disable_vdda;
>  	}
>  
>  	ret = regmap_read(data->regmap, BMP280_REG_ID, &chip_id);
> @@ -1086,14 +1119,36 @@ static int bmp280_probe(struct i2c_client *client,
>  	if (chip_id != id->driver_data) {
>  		dev_err(&client->dev, "bad chip id.  expected %x got %x\n",
>  			BMP280_CHIP_ID, chip_id);
> -		return -EINVAL;
> +		ret = -EINVAL;
> +		goto out_disable_vdda;
>  	}
>  
>  	ret = data->chip_info->chip_config(data);
>  	if (ret < 0)
> -		return ret;
> +		goto out_disable_vdda;
> +
> +	i2c_set_clientdata(client, data);
>  
> -	return devm_iio_device_register(&client->dev, indio_dev);
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret)
> +		goto out_disable_vdda;
> +
> +	return 0;
> +
> +out_disable_vdda:
> +	regulator_disable(data->vdda);
> +out_disable_vddd:
> +	regulator_disable(data->vddd);
> +	return ret;
> +}
> +
> +static int bmp280_remove(struct i2c_client *client)
> +{
> +	struct bmp280_data *data = i2c_get_clientdata(client);
> +
> +	regulator_disable(data->vdda);
> +	regulator_disable(data->vddd);
Race condition as the userspace and in kernel interfaces to
use the device haven't been removed, but we've turned off the
lights...
> +	return 0;
>  }
>  
>  static const struct acpi_device_id bmp280_acpi_match[] = {
> @@ -1134,6 +1189,7 @@ static struct i2c_driver bmp280_driver = {
>  		.of_match_table = of_match_ptr(bmp280_of_match),
>  	},
>  	.probe		= bmp280_probe,
> +	.remove		= bmp280_remove,
>  	.id_table	= bmp280_id,
>  };
>  module_i2c_driver(bmp280_driver);
>