On 30/06/16 20:30, Jonathan Cameron wrote:
> 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@xxxxxxxxxx>
>> Signed-off-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> 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.
I'm in an odd mood this morning so will fix this up (mostly to try
and avoid having to see a v5 ;)
Jonathan
Applied this one with the changes mentioned below. Please check the
result as I may well have messed it up.
The next patch is going to be 'fun'.
>
> 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);
>>
>
> --
> 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
>
--
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
