| Message ID | 20201202093322.77114-4-stephan@gerhold.net |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
On Wed, Dec 2, 2020 at 10:33 AM Stephan Gerhold <stephan@gerhold.net> wrote: > BMG160 needs VDD and VDDIO regulators that might need to be explicitly > enabled. Add some rudimentary support to obtain and enable these > regulators during probe() and disable them during remove() > or on the error path. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> Reviewed-by: Linus Walleij <linus.walleij@linar.org> Yours, Linus Walleij
On Wed, 2 Dec 2020 10:33:22 +0100 Stephan Gerhold <stephan@gerhold.net> wrote: > BMG160 needs VDD and VDDIO regulators that might need to be explicitly > enabled. Add some rudimentary support to obtain and enable these > regulators during probe() and disable them during remove() > or on the error path. > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> Hi Stephan, This one is a bit tricky due to the extensive use of devm_ managed cleanup. Normally I'd be very fussy about ensuring remove order is precise reverse of probe, but in this driver it isn't quite already, due to that chip_init being before the interrupt allocation. Having said that I'd rather not make it worse. Would you mind using automated clean up of the regulator_enable as well via devm_add_action_or_reset() call? As a side note, should we not have more cleanup of chip_init() in error paths, specifically putting the device into it's suspended mode? Obviously nothing to do with your patch... Thanks, Jonathan > --- > drivers/iio/gyro/bmg160_core.c | 38 +++++++++++++++++++++++++++------- > 1 file changed, 31 insertions(+), 7 deletions(-) > > diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c > index 2d5015801a75..4baa4169c5a2 100644 > --- a/drivers/iio/gyro/bmg160_core.c > +++ b/drivers/iio/gyro/bmg160_core.c > @@ -19,6 +19,7 @@ > #include <linux/iio/trigger_consumer.h> > #include <linux/iio/triggered_buffer.h> > #include <linux/regmap.h> > +#include <linux/regulator/consumer.h> > #include "bmg160.h" > > #define BMG160_IRQ_NAME "bmg160_event" > @@ -92,6 +93,7 @@ > > struct bmg160_data { > struct regmap *regmap; > + struct regulator_bulk_data regulators[2]; > struct iio_trigger *dready_trig; > struct iio_trigger *motion_trig; > struct iio_mount_matrix orientation; > @@ -1077,14 +1079,28 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, > data->irq = irq; > data->regmap = regmap; > > + data->regulators[0].supply = "vdd"; > + data->regulators[1].supply = "vddio"; > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators), > + data->regulators); > + if (ret) > + return dev_err_probe(dev, ret, "Failed to get regulators\n"); > + > ret = iio_read_mount_matrix(dev, "mount-matrix", > &data->orientation); > if (ret) > return ret; Why not put regulator get and enable together? > > + ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators), > + data->regulators); > + if (ret < 0) { > + dev_err(dev, "Failed to enable regulators: %d\n", ret); > + return ret; > + } > + If you were to use devm_add_action_or_reset() and a trivial wrapper the disable would be automated, simplifying the error handling etc. > ret = bmg160_chip_init(data); > if (ret < 0) > - return ret; > + goto err_regulator_disable; > > mutex_init(&data->mutex); > > @@ -1107,28 +1123,32 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, > BMG160_IRQ_NAME, > indio_dev); > if (ret) > - return ret; > + goto err_regulator_disable; > > data->dready_trig = devm_iio_trigger_alloc(dev, > "%s-dev%d", > indio_dev->name, > indio_dev->id); > - if (!data->dready_trig) > - return -ENOMEM; > + if (!data->dready_trig) { > + ret = -ENOMEM; > + goto err_regulator_disable; > + } > > data->motion_trig = devm_iio_trigger_alloc(dev, > "%s-any-motion-dev%d", > indio_dev->name, > indio_dev->id); > - if (!data->motion_trig) > - return -ENOMEM; > + if (!data->motion_trig) { > + ret = -ENOMEM; > + goto err_regulator_disable; > + } > > data->dready_trig->dev.parent = dev; > data->dready_trig->ops = &bmg160_trigger_ops; > iio_trigger_set_drvdata(data->dready_trig, indio_dev); > ret = iio_trigger_register(data->dready_trig); > if (ret) > - return ret; > + goto err_regulator_disable; > > data->motion_trig->dev.parent = dev; > data->motion_trig->ops = &bmg160_trigger_ops; > @@ -1174,6 +1194,8 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, > iio_trigger_unregister(data->dready_trig); > if (data->motion_trig) > iio_trigger_unregister(data->motion_trig); > +err_regulator_disable: > + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); > > return ret; > } > @@ -1200,6 +1222,8 @@ void bmg160_core_remove(struct device *dev) > mutex_lock(&data->mutex); > bmg160_set_mode(data, BMG160_MODE_DEEP_SUSPEND); > mutex_unlock(&data->mutex); > + > + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); > } > EXPORT_SYMBOL_GPL(bmg160_core_remove); >
Hi Jonathan, On Sat, Dec 05, 2020 at 03:38:48PM +0000, Jonathan Cameron wrote: > On Wed, 2 Dec 2020 10:33:22 +0100 > Stephan Gerhold <stephan@gerhold.net> wrote: > > > BMG160 needs VDD and VDDIO regulators that might need to be explicitly > > enabled. Add some rudimentary support to obtain and enable these > > regulators during probe() and disable them during remove() > > or on the error path. > > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net> > > This one is a bit tricky due to the extensive use of devm_ managed > cleanup. Normally I'd be very fussy about ensuring remove order > is precise reverse of probe, but in this driver it isn't quite > already, due to that chip_init being before the interrupt allocation. > > Having said that I'd rather not make it worse. Would you mind > using automated clean up of the regulator_enable as well via > devm_add_action_or_reset() call? > Good point, devm_add_action_or_reset() definitely looks better for this driver. I will send a v2 with just the bmg160 part shortly. > As a side note, should we not have more cleanup of chip_init() > in error paths, specifically putting the device into it's suspended > mode? Obviously nothing to do with your patch... > I'm not sure. I guess when bmg160_chip_init() fails there is some kind of communication problem or a problem with the chip. Chances are that putting it back into suspend mode would also fail. But I don't really know enough about the hardware to say more. :) I have also fixed your other comments below in v2. Thanks! Stephan > > > --- > > drivers/iio/gyro/bmg160_core.c | 38 +++++++++++++++++++++++++++------- > > 1 file changed, 31 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c > > index 2d5015801a75..4baa4169c5a2 100644 > > --- a/drivers/iio/gyro/bmg160_core.c > > +++ b/drivers/iio/gyro/bmg160_core.c > > @@ -19,6 +19,7 @@ > > #include <linux/iio/trigger_consumer.h> > > #include <linux/iio/triggered_buffer.h> > > #include <linux/regmap.h> > > +#include <linux/regulator/consumer.h> > > #include "bmg160.h" > > > > #define BMG160_IRQ_NAME "bmg160_event" > > @@ -92,6 +93,7 @@ > > > > struct bmg160_data { > > struct regmap *regmap; > > + struct regulator_bulk_data regulators[2]; > > struct iio_trigger *dready_trig; > > struct iio_trigger *motion_trig; > > struct iio_mount_matrix orientation; > > @@ -1077,14 +1079,28 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, > > data->irq = irq; > > data->regmap = regmap; > > > > + data->regulators[0].supply = "vdd"; > > + data->regulators[1].supply = "vddio"; > > + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators), > > + data->regulators); > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to get regulators\n"); > > + > > ret = iio_read_mount_matrix(dev, "mount-matrix", > > &data->orientation); > > if (ret) > > return ret; > > Why not put regulator get and enable together? > > > > > + ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators), > > + data->regulators); > > + if (ret < 0) { > > + dev_err(dev, "Failed to enable regulators: %d\n", ret); > > + return ret; > > + } > > + > > If you were to use devm_add_action_or_reset() and a trivial wrapper > the disable would be automated, simplifying the error handling etc. > > > ret = bmg160_chip_init(data); > > if (ret < 0) > > - return ret; > > + goto err_regulator_disable; > > > > mutex_init(&data->mutex); > > > > @@ -1107,28 +1123,32 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, > > BMG160_IRQ_NAME, > > indio_dev); > > if (ret) > > - return ret; > > + goto err_regulator_disable; > > > > data->dready_trig = devm_iio_trigger_alloc(dev, > > "%s-dev%d", > > indio_dev->name, > > indio_dev->id); > > - if (!data->dready_trig) > > - return -ENOMEM; > > + if (!data->dready_trig) { > > + ret = -ENOMEM; > > + goto err_regulator_disable; > > + } > > > > data->motion_trig = devm_iio_trigger_alloc(dev, > > "%s-any-motion-dev%d", > > indio_dev->name, > > indio_dev->id); > > - if (!data->motion_trig) > > - return -ENOMEM; > > + if (!data->motion_trig) { > > + ret = -ENOMEM; > > + goto err_regulator_disable; > > + } > > > > data->dready_trig->dev.parent = dev; > > data->dready_trig->ops = &bmg160_trigger_ops; > > iio_trigger_set_drvdata(data->dready_trig, indio_dev); > > ret = iio_trigger_register(data->dready_trig); > > if (ret) > > - return ret; > > + goto err_regulator_disable; > > > > data->motion_trig->dev.parent = dev; > > data->motion_trig->ops = &bmg160_trigger_ops; > > @@ -1174,6 +1194,8 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, > > iio_trigger_unregister(data->dready_trig); > > if (data->motion_trig) > > iio_trigger_unregister(data->motion_trig); > > +err_regulator_disable: > > + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); > > > > return ret; > > } > > @@ -1200,6 +1222,8 @@ void bmg160_core_remove(struct device *dev) > > mutex_lock(&data->mutex); > > bmg160_set_mode(data, BMG160_MODE_DEEP_SUSPEND); > > mutex_unlock(&data->mutex); > > + > > + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); > > } > > EXPORT_SYMBOL_GPL(bmg160_core_remove); > > >
diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c index 2d5015801a75..4baa4169c5a2 100644 --- a/drivers/iio/gyro/bmg160_core.c +++ b/drivers/iio/gyro/bmg160_core.c @@ -19,6 +19,7 @@ #include <linux/iio/trigger_consumer.h> #include <linux/iio/triggered_buffer.h> #include <linux/regmap.h> +#include <linux/regulator/consumer.h> #include "bmg160.h" #define BMG160_IRQ_NAME "bmg160_event" @@ -92,6 +93,7 @@ struct bmg160_data { struct regmap *regmap; + struct regulator_bulk_data regulators[2]; struct iio_trigger *dready_trig; struct iio_trigger *motion_trig; struct iio_mount_matrix orientation; @@ -1077,14 +1079,28 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, data->irq = irq; data->regmap = regmap; + data->regulators[0].supply = "vdd"; + data->regulators[1].supply = "vddio"; + ret = devm_regulator_bulk_get(dev, ARRAY_SIZE(data->regulators), + data->regulators); + if (ret) + return dev_err_probe(dev, ret, "Failed to get regulators\n"); + ret = iio_read_mount_matrix(dev, "mount-matrix", &data->orientation); if (ret) return ret; + ret = regulator_bulk_enable(ARRAY_SIZE(data->regulators), + data->regulators); + if (ret < 0) { + dev_err(dev, "Failed to enable regulators: %d\n", ret); + return ret; + } + ret = bmg160_chip_init(data); if (ret < 0) - return ret; + goto err_regulator_disable; mutex_init(&data->mutex); @@ -1107,28 +1123,32 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, BMG160_IRQ_NAME, indio_dev); if (ret) - return ret; + goto err_regulator_disable; data->dready_trig = devm_iio_trigger_alloc(dev, "%s-dev%d", indio_dev->name, indio_dev->id); - if (!data->dready_trig) - return -ENOMEM; + if (!data->dready_trig) { + ret = -ENOMEM; + goto err_regulator_disable; + } data->motion_trig = devm_iio_trigger_alloc(dev, "%s-any-motion-dev%d", indio_dev->name, indio_dev->id); - if (!data->motion_trig) - return -ENOMEM; + if (!data->motion_trig) { + ret = -ENOMEM; + goto err_regulator_disable; + } data->dready_trig->dev.parent = dev; data->dready_trig->ops = &bmg160_trigger_ops; iio_trigger_set_drvdata(data->dready_trig, indio_dev); ret = iio_trigger_register(data->dready_trig); if (ret) - return ret; + goto err_regulator_disable; data->motion_trig->dev.parent = dev; data->motion_trig->ops = &bmg160_trigger_ops; @@ -1174,6 +1194,8 @@ int bmg160_core_probe(struct device *dev, struct regmap *regmap, int irq, iio_trigger_unregister(data->dready_trig); if (data->motion_trig) iio_trigger_unregister(data->motion_trig); +err_regulator_disable: + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); return ret; } @@ -1200,6 +1222,8 @@ void bmg160_core_remove(struct device *dev) mutex_lock(&data->mutex); bmg160_set_mode(data, BMG160_MODE_DEEP_SUSPEND); mutex_unlock(&data->mutex); + + regulator_bulk_disable(ARRAY_SIZE(data->regulators), data->regulators); } EXPORT_SYMBOL_GPL(bmg160_core_remove);
BMG160 needs VDD and VDDIO regulators that might need to be explicitly enabled. Add some rudimentary support to obtain and enable these regulators during probe() and disable them during remove() or on the error path. Signed-off-by: Stephan Gerhold <stephan@gerhold.net> --- drivers/iio/gyro/bmg160_core.c | 38 +++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-)