On Tue, 17 Jul 2018 04:41:56 -0400 Brian Masney <[email protected]> wrote:
> This patch adds support for the regulator framework to the tsl2772 > driver. Driver was tested using a LG Nexus 5 (hammerhead) phone with > the two regulators and on a Raspberry Pi 2 without any regulators > controlling the power to the sensor. > > Signed-off-by: Brian Masney <[email protected]> There is an ordering issue in probe and hence the tear down. Fixing it is unfortunately going to make the rest of the handling more complex... Jonathan > --- > .../devicetree/bindings/iio/light/tsl2772.txt | 4 + > drivers/iio/light/tsl2772.c | 82 ++++++++++++++++++- > 2 files changed, 85 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/iio/light/tsl2772.txt > b/Documentation/devicetree/bindings/iio/light/tsl2772.txt > index ab553d52b9fc..bfde8b2c8790 100644 > --- a/Documentation/devicetree/bindings/iio/light/tsl2772.txt > +++ b/Documentation/devicetree/bindings/iio/light/tsl2772.txt > @@ -21,6 +21,8 @@ Optional properties: > TSL2772_DIODE_BOTH. > - amstaos,prox_power - must be TSL2772_100_mA, TSL2772_50_mA, > TSL2772_25_mA, > or TSL2772_13_mA. > + - vdd-supply: phandle to the regulator that provides power to the sensor. > + - vddio-supply: phandle to the regulator that provides power to the bus. > - interrupt-parent: should be the phandle for the interrupt controller > - interrupts: the sole interrupt generated by the device > > @@ -35,5 +37,7 @@ tsl2772@39 { > compatible = "amstaos,tsl2772"; > reg = <0x39>; > interrupts-extended = <&msmgpio 61 IRQ_TYPE_EDGE_FALLING>; > + vdd-supply = <&pm8941_l17>; > + vddio-supply = <&pm8941_lvs1>; > amstaos,prox_diode = <TSL2772_DIODE0>; > }; > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c > index 0be57f2ecb02..f6a27f8b3ca7 100644 > --- a/drivers/iio/light/tsl2772.c > +++ b/drivers/iio/light/tsl2772.c > @@ -20,6 +20,7 @@ > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > #include <linux/platform_data/tsl2772.h> > +#include <linux/regulator/consumer.h> > > /* Cal defs */ > #define PROX_STAT_CAL 0 > @@ -146,6 +147,9 @@ struct tsl2772_chip { > struct mutex prox_mutex; > struct mutex als_mutex; > struct i2c_client *client; > + struct regulator *vdd_supply; > + struct regulator *vddio_supply; > + bool regulators_enabled; > u16 prox_data; > struct tsl2772_als_info als_cur_info; > struct tsl2772_settings settings; > @@ -609,6 +613,46 @@ static int tsl2772_als_calibrate(struct iio_dev > *indio_dev) > return ret; > } > > +static int tsl2772_enable_regulators(struct tsl2772_chip *chip) > +{ > + int ret; > + > + ret = regulator_enable(chip->vddio_supply); > + if (ret < 0) { > + dev_err(&chip->client->dev, "Failed to enable regulator: %d\n", > + ret); > + return ret; > + } > + > + ret = regulator_enable(chip->vdd_supply); > + if (ret < 0) { > + regulator_disable(chip->vddio_supply); > + dev_err(&chip->client->dev, "Failed to enable regulator: %d\n", > + ret); > + return ret; > + } > + > + chip->regulators_enabled = true; > + > + return 0; > +} > + > +static void tsl2772_disable_regulators(struct tsl2772_chip *chip) > +{ > + if (!chip->regulators_enabled) > + return; > + > + regulator_disable(chip->vdd_supply); > + regulator_disable(chip->vddio_supply); > + > + chip->regulators_enabled = false; hmm. this gets fiddly so I can see why you are using this flag to avoid missbalanced enables / disables. > +} > + > +static void tsl2772_disable_regulators_action(void *_data) > +{ > + tsl2772_disable_regulators(_data); > +} > + > static int tsl2772_chip_on(struct iio_dev *indio_dev) > { > struct tsl2772_chip *chip = iio_priv(indio_dev); > @@ -666,6 +710,10 @@ static int tsl2772_chip_on(struct iio_dev *indio_dev) > chip->als_gain_time_scale = als_time_us * > tsl2772_als_gain[chip->settings.als_gain]; > > + ret = tsl2772_enable_regulators(chip); > + if (ret < 0) > + return ret; > + > /* > * TSL2772 Specific power-on / adc enable sequence > * Power on the device 1st. > @@ -724,10 +772,13 @@ static int tsl2772_chip_on(struct iio_dev *indio_dev) > static int tsl2772_chip_off(struct iio_dev *indio_dev) > { > struct tsl2772_chip *chip = iio_priv(indio_dev); > + int ret; > > /* turn device off */ > chip->tsl2772_chip_status = TSL2772_CHIP_SUSPENDED; > - return tsl2772_write_control_reg(chip, 0x00); > + ret = tsl2772_write_control_reg(chip, 0x00); > + tsl2772_disable_regulators(chip); Blank line here would be nice. > + return ret; > } > > /** > @@ -1666,6 +1717,35 @@ static int tsl2772_probe(struct i2c_client *clientp, > chip->client = clientp; > i2c_set_clientdata(clientp, indio_dev); > > + chip->vddio_supply = devm_regulator_get(&clientp->dev, "vddio"); > + if (IS_ERR(chip->vddio_supply)) { > + if (PTR_ERR(chip->vddio_supply) != -EPROBE_DEFER) > + dev_err(&clientp->dev, > + "Failed to get vddio regulator %d\n", > + (int)PTR_ERR(chip->vddio_supply)); > + > + return PTR_ERR(chip->vddio_supply); > + } > + > + chip->vdd_supply = devm_regulator_get(&clientp->dev, "vdd"); > + if (IS_ERR(chip->vdd_supply)) { > + if (PTR_ERR(chip->vdd_supply) != -EPROBE_DEFER) > + dev_err(&clientp->dev, > + "Failed to get vdd regulator %d\n", > + (int)PTR_ERR(chip->vdd_supply)); > + > + return PTR_ERR(chip->vdd_supply); > + } > + > + ret = devm_add_action(&clientp->dev, tsl2772_disable_regulators_action, > + chip); Can't do them together. you need to think about where you might get a failure. What if it is after the first enable but before the second? In that case you've just left a regulator on as we haven't done this setup yet. > + if (ret < 0) { > + tsl2772_disable_regulators_action(chip); > + dev_err(&clientp->dev, "Failed to setup regulator cleanup > action %d\n", > + ret); > + return ret; > + } > + > ret = i2c_smbus_read_byte_data(chip->client, > TSL2772_CMD_REG | TSL2772_CHIPID); > if (ret < 0)
