[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-iio
Subject:    Re: [PATCH 1/2] iio: gyro: mpu3050: Use devm_ to set up buffer
From:       Jonathan Cameron <jic23 () kernel ! org>
Date:       2020-11-30 20:51:50
Message-ID: 20201130205150.052fb114 () archlinux
[Download RAW message or body]

On Mon, 30 Nov 2020 13:59:14 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

> This makes use of devm_iio_triggered_buffer_setup() to
> save some minor overhead.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Hi Linus,

I'm very fussy about mixing devm and other cleanup.  Unless everything
that happens after this point is also managed, I'm not happy to see
an individual function made managed.  It may be safe, but
if fails the 'obviously safe' test.

Something odd going on here though.  We are currently removing the
buffer before we unregister the userspace interfaces to it.
That's not a good idea.  The remove order seems reverse from
what it should be...

Jonathan

> ---
>  drivers/iio/gyro/mpu3050-core.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/gyro/mpu3050-core.c b/drivers/iio/gyro/mpu3050-core.c
> index 00e58060968c..0d0850945d3a 100644
> --- a/drivers/iio/gyro/mpu3050-core.c
> +++ b/drivers/iio/gyro/mpu3050-core.c
> @@ -1203,9 +1203,10 @@ int mpu3050_common_probe(struct device *dev,
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  	indio_dev->name = name;
>  
> -	ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
> -					 mpu3050_trigger_handler,
> -					 &mpu3050_buffer_setup_ops);
> +	ret = devm_iio_triggered_buffer_setup(dev,
> +					indio_dev, iio_pollfunc_store_time,
> +					mpu3050_trigger_handler,
> +					&mpu3050_buffer_setup_ops);
>  	if (ret) {
>  		dev_err(dev, "triggered buffer setup failed\n");
>  		goto err_power_down;
> @@ -1214,7 +1215,7 @@ int mpu3050_common_probe(struct device *dev,
>  	ret = iio_device_register(indio_dev);
>  	if (ret) {
>  		dev_err(dev, "device register failed\n");
> -		goto err_cleanup_buffer;
> +		goto err_power_down;
>  	}
>  
>  	dev_set_drvdata(dev, indio_dev);
> @@ -1241,8 +1242,6 @@ int mpu3050_common_probe(struct device *dev,
>  
>  	return 0;
>  
> -err_cleanup_buffer:
> -	iio_triggered_buffer_cleanup(indio_dev);
>  err_power_down:
>  	mpu3050_power_down(mpu3050);
>  
> @@ -1258,7 +1257,6 @@ int mpu3050_common_remove(struct device *dev)
>  	pm_runtime_get_sync(dev);
>  	pm_runtime_put_noidle(dev);
>  	pm_runtime_disable(dev);
> -	iio_triggered_buffer_cleanup(indio_dev);
>  	if (mpu3050->irq)
>  		free_irq(mpu3050->irq, mpu3050);
>  	iio_device_unregister(indio_dev);

[prev in list] [next in list] [prev in thread] [next in thread]