| Message ID | 20181119005347.747-2-martin@martingkelly.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] iio: bmi160: snap timestamp closer to event | expand |
On Mon, Nov 19, 2018 at 2:55 AM Martin Kelly <martin@martingkelly.com> wrote: > > From: Martin Kelly <martin@martingkelly.com> > > Currently, we're using the devm version of some but not all functions. > Switch to the devm version of iio_triggered_buffer_setup and > iio_device_register to simplify the code a bit and decrease the chance of > bugs. Yes, it makes sense. Acked-by: Daniel Baluta <daniel.baluta@gmail.com> > > Signed-off-by: Martin Kelly <martin@martingkelly.com> > --- > drivers/iio/imu/bmi160/bmi160_core.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c > index 4d9d59d9e3a9..5e4efaed5e88 100644 > --- a/drivers/iio/imu/bmi160/bmi160_core.c > +++ b/drivers/iio/imu/bmi160/bmi160_core.c > @@ -577,18 +577,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap, > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &bmi160_info; > > - ret = iio_triggered_buffer_setup(indio_dev, NULL, > - bmi160_trigger_handler, NULL); > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > + bmi160_trigger_handler, NULL); > if (ret < 0) > goto uninit; > > - ret = iio_device_register(indio_dev); > + ret = devm_iio_device_register(dev, indio_dev); > if (ret < 0) > - goto buffer_cleanup; > + goto uninit; > > return 0; > -buffer_cleanup: > - iio_triggered_buffer_cleanup(indio_dev); > uninit: > bmi160_chip_uninit(data); > return ret; > @@ -600,8 +598,6 @@ void bmi160_core_remove(struct device *dev) > struct iio_dev *indio_dev = dev_get_drvdata(dev); > struct bmi160_data *data = iio_priv(indio_dev); > > - iio_device_unregister(indio_dev); > - iio_triggered_buffer_cleanup(indio_dev); > bmi160_chip_uninit(data); > } > EXPORT_SYMBOL_GPL(bmi160_core_remove); > -- > 2.11.0 >
On Mon, 19 Nov 2018 10:48:20 +0200 Daniel Baluta <daniel.baluta@intel.com> wrote: > On Mon, Nov 19, 2018 at 2:55 AM Martin Kelly <martin@martingkelly.com> wrote: > > > > From: Martin Kelly <martin@martingkelly.com> > > > > Currently, we're using the devm version of some but not all functions. > > Switch to the devm version of iio_triggered_buffer_setup and > > iio_device_register to simplify the code a bit and decrease the chance of > > bugs. > > Yes, it makes sense. > > Acked-by: Daniel Baluta <daniel.baluta@gmail.com> Here we disagree :). This results in the chip being uninitialized via bmi160_chip_uninit before we remove the user space interfaces. So the combination that is there right now was very deliberate and is correct - that's not to say it can't be improved upon. So if you do want to do this you need to ensure that operation automatically occurs in the correct point in the remove and error flows. devm_add_action_or_reset is your friend here as it'll let you call that uninit from the automatic unwinding path and hence at the correct point. Thanks, Jonathan > > > > > Signed-off-by: Martin Kelly <martin@martingkelly.com> > > --- > > drivers/iio/imu/bmi160/bmi160_core.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c > > index 4d9d59d9e3a9..5e4efaed5e88 100644 > > --- a/drivers/iio/imu/bmi160/bmi160_core.c > > +++ b/drivers/iio/imu/bmi160/bmi160_core.c > > @@ -577,18 +577,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap, > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->info = &bmi160_info; > > > > - ret = iio_triggered_buffer_setup(indio_dev, NULL, > > - bmi160_trigger_handler, NULL); > > + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, > > + bmi160_trigger_handler, NULL); > > if (ret < 0) > > goto uninit; > > > > - ret = iio_device_register(indio_dev); > > + ret = devm_iio_device_register(dev, indio_dev); > > if (ret < 0) > > - goto buffer_cleanup; > > + goto uninit; > > > > return 0; > > -buffer_cleanup: > > - iio_triggered_buffer_cleanup(indio_dev); > > uninit: > > bmi160_chip_uninit(data); > > return ret; > > @@ -600,8 +598,6 @@ void bmi160_core_remove(struct device *dev) > > struct iio_dev *indio_dev = dev_get_drvdata(dev); > > struct bmi160_data *data = iio_priv(indio_dev); > > > > - iio_device_unregister(indio_dev); > > - iio_triggered_buffer_cleanup(indio_dev); > > bmi160_chip_uninit(data); > > } > > EXPORT_SYMBOL_GPL(bmi160_core_remove); > > -- > > 2.11.0 > >
On 11/25/18 5:51 AM, Jonathan Cameron wrote: > On Mon, 19 Nov 2018 10:48:20 +0200 > Daniel Baluta <daniel.baluta@intel.com> wrote: > >> On Mon, Nov 19, 2018 at 2:55 AM Martin Kelly <martin@martingkelly.com> wrote: >>> >>> From: Martin Kelly <martin@martingkelly.com> >>> >>> Currently, we're using the devm version of some but not all functions. >>> Switch to the devm version of iio_triggered_buffer_setup and >>> iio_device_register to simplify the code a bit and decrease the chance of >>> bugs. >> >> Yes, it makes sense. >> >> Acked-by: Daniel Baluta <daniel.baluta@gmail.com> > Here we disagree :). This results in the chip being uninitialized > via bmi160_chip_uninit before we remove the user space interfaces. > So the combination that is there right now was very deliberate > and is correct - that's not to say it can't be improved upon. > > So if you do want to do this you need to ensure that operation > automatically occurs in the correct point in the remove and error > flows. > > devm_add_action_or_reset is your friend here as it'll let you call > that uninit from the automatic unwinding path and hence at the > correct point. > > Thanks, > > Jonathan > Thank you and good catch. I'll send a v2 using devm_add_action_or_reset. >> >>> >>> Signed-off-by: Martin Kelly <martin@martingkelly.com> >>> --- >>> drivers/iio/imu/bmi160/bmi160_core.c | 12 ++++-------- >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c >>> index 4d9d59d9e3a9..5e4efaed5e88 100644 >>> --- a/drivers/iio/imu/bmi160/bmi160_core.c >>> +++ b/drivers/iio/imu/bmi160/bmi160_core.c >>> @@ -577,18 +577,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap, >>> indio_dev->modes = INDIO_DIRECT_MODE; >>> indio_dev->info = &bmi160_info; >>> >>> - ret = iio_triggered_buffer_setup(indio_dev, NULL, >>> - bmi160_trigger_handler, NULL); >>> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, >>> + bmi160_trigger_handler, NULL); >>> if (ret < 0) >>> goto uninit; >>> >>> - ret = iio_device_register(indio_dev); >>> + ret = devm_iio_device_register(dev, indio_dev); >>> if (ret < 0) >>> - goto buffer_cleanup; >>> + goto uninit; >>> >>> return 0; >>> -buffer_cleanup: >>> - iio_triggered_buffer_cleanup(indio_dev); >>> uninit: >>> bmi160_chip_uninit(data); >>> return ret; >>> @@ -600,8 +598,6 @@ void bmi160_core_remove(struct device *dev) >>> struct iio_dev *indio_dev = dev_get_drvdata(dev); >>> struct bmi160_data *data = iio_priv(indio_dev); >>> >>> - iio_device_unregister(indio_dev); >>> - iio_triggered_buffer_cleanup(indio_dev); >>> bmi160_chip_uninit(data); >>> } >>> EXPORT_SYMBOL_GPL(bmi160_core_remove); >>> -- >>> 2.11.0 >>> >
diff --git a/drivers/iio/imu/bmi160/bmi160_core.c b/drivers/iio/imu/bmi160/bmi160_core.c index 4d9d59d9e3a9..5e4efaed5e88 100644 --- a/drivers/iio/imu/bmi160/bmi160_core.c +++ b/drivers/iio/imu/bmi160/bmi160_core.c @@ -577,18 +577,16 @@ int bmi160_core_probe(struct device *dev, struct regmap *regmap, indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &bmi160_info; - ret = iio_triggered_buffer_setup(indio_dev, NULL, - bmi160_trigger_handler, NULL); + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL, + bmi160_trigger_handler, NULL); if (ret < 0) goto uninit; - ret = iio_device_register(indio_dev); + ret = devm_iio_device_register(dev, indio_dev); if (ret < 0) - goto buffer_cleanup; + goto uninit; return 0; -buffer_cleanup: - iio_triggered_buffer_cleanup(indio_dev); uninit: bmi160_chip_uninit(data); return ret; @@ -600,8 +598,6 @@ void bmi160_core_remove(struct device *dev) struct iio_dev *indio_dev = dev_get_drvdata(dev); struct bmi160_data *data = iio_priv(indio_dev); - iio_device_unregister(indio_dev); - iio_triggered_buffer_cleanup(indio_dev); bmi160_chip_uninit(data); } EXPORT_SYMBOL_GPL(bmi160_core_remove);