| Message ID | 20190403070325.1077-4-andrew.smirnov@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i.MX7D ADC improvements | expand |
On Wed, 3 Apr 2019 00:03:22 -0700 Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > Use devm_iio_device_register() and drop explicit call to > iio_device_unregister(). > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > Cc: Jonathan Cameron <jic23@kernel.org> > Cc: Hartmut Knaack <knaack.h@gmx.de> > Cc: Lars-Peter Clausen <lars@metafoo.de> > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net> > Cc: Chris Healy <cphealy@gmail.com> > Cc: linux-iio@vger.kernel.org > Cc: linux-kernel@vger.kernel.org No to this one. The thing to think about is the resulting order of the unwinding that happens in remove. Do take a look at the code flow, but in short what happens is: 1. driver.remove() 2. Devm release functions run in the opposite order to they were called during setup. The upshot of the change you just made here is that we turn the power off to the device before we remove the userspace interfaces, giving potentially interesting failures.. There are two options to avoid this: 1. Make everything use devm_ calls (often using devm_add_action_or_reset for the ones that don't have their own versions). 2. Stop using devm managed functions at the first non devm_ call that needs unwinding. From that point onwards in probe / remove you have to do everything manually. Jonathan > --- > drivers/iio/adc/imx7d_adc.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c > index 72cfe9834bcb..9a46838ec7cf 100644 > --- a/drivers/iio/adc/imx7d_adc.c > +++ b/drivers/iio/adc/imx7d_adc.c > @@ -517,7 +517,7 @@ static int imx7d_adc_probe(struct platform_device *pdev) > imx7d_adc_feature_config(info); > imx7d_adc_hw_init(info); > > - ret = iio_device_register(indio_dev); > + ret = devm_iio_device_register(dev, indio_dev); > if (ret) { > imx7d_adc_power_down(info); > dev_err(&pdev->dev, "Couldn't register the device.\n"); > @@ -539,8 +539,6 @@ static int imx7d_adc_remove(struct platform_device *pdev) > struct iio_dev *indio_dev = platform_get_drvdata(pdev); > struct imx7d_adc *info = iio_priv(indio_dev); > > - iio_device_unregister(indio_dev); > - > imx7d_adc_power_down(info); > > clk_disable_unprepare(info->clk);
On Sun, Apr 7, 2019 at 4:07 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 3 Apr 2019 00:03:22 -0700 > Andrey Smirnov <andrew.smirnov@gmail.com> wrote: > > > Use devm_iio_device_register() and drop explicit call to > > iio_device_unregister(). > > > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> > > Cc: Jonathan Cameron <jic23@kernel.org> > > Cc: Hartmut Knaack <knaack.h@gmx.de> > > Cc: Lars-Peter Clausen <lars@metafoo.de> > > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net> > > Cc: Chris Healy <cphealy@gmail.com> > > Cc: linux-iio@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > No to this one. The thing to think about is the resulting order > of the unwinding that happens in remove. > > Do take a look at the code flow, but in short what happens is: > > 1. driver.remove() > 2. Devm release functions run in the opposite order to they were > called during setup. > > The upshot of the change you just made here is that we turn the power > off to the device before we remove the userspace interfaces, giving > potentially interesting failures.. > > There are two options to avoid this: > > 1. Make everything use devm_ calls (often using devm_add_action_or_reset > for the ones that don't have their own versions). > > 2. Stop using devm managed functions at the first non devm_ call that needs > unwinding. From that point onwards in probe / remove you have to do everything > manually. > Yeah, missed the ordering proble, sorry about that. I'll re-order changes such that this conversion happens last to avoid said problem in v2. Thanks, Andrey Smirnov
diff --git a/drivers/iio/adc/imx7d_adc.c b/drivers/iio/adc/imx7d_adc.c index 72cfe9834bcb..9a46838ec7cf 100644 --- a/drivers/iio/adc/imx7d_adc.c +++ b/drivers/iio/adc/imx7d_adc.c @@ -517,7 +517,7 @@ static int imx7d_adc_probe(struct platform_device *pdev) imx7d_adc_feature_config(info); imx7d_adc_hw_init(info); - ret = iio_device_register(indio_dev); + ret = devm_iio_device_register(dev, indio_dev); if (ret) { imx7d_adc_power_down(info); dev_err(&pdev->dev, "Couldn't register the device.\n"); @@ -539,8 +539,6 @@ static int imx7d_adc_remove(struct platform_device *pdev) struct iio_dev *indio_dev = platform_get_drvdata(pdev); struct imx7d_adc *info = iio_priv(indio_dev); - iio_device_unregister(indio_dev); - imx7d_adc_power_down(info); clk_disable_unprepare(info->clk);
Use devm_iio_device_register() and drop explicit call to iio_device_unregister(). Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com> Cc: Jonathan Cameron <jic23@kernel.org> Cc: Hartmut Knaack <knaack.h@gmx.de> Cc: Lars-Peter Clausen <lars@metafoo.de> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net> Cc: Chris Healy <cphealy@gmail.com> Cc: linux-iio@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- drivers/iio/adc/imx7d_adc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)