From: "Ardelean, Alexandru" <alexandru.Ardelean@analog.com>
To: "jic23@kernel.org" <jic23@kernel.org>
Cc: "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Subject: Re: [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants
Date: Mon, 4 May 2020 05:52:35 +0000 [thread overview]
Message-ID: <8695e337ca71351cac2f262b0104a1411c173417.camel@analog.com> (raw)
In-Reply-To: <20200502192542.63cc25a2@archlinux>
On Sat, 2020-05-02 at 19:25 +0100, Jonathan Cameron wrote:
> On Tue, 28 Apr 2020 12:31:28 +0300
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
>
> > This change cleans up the driver's probe function to use only devm_
> > function variants. This also gets rid of the remove function and moves the
> > clock & regulator de-initializations to the 'ad5933_cleanup()' callback.
> >
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
>
> Basic rule of thumb. Whatever you register with devm_add_action_or_reset
> should only cleanup one one thing done in the probe path.
> There is almost always a race if you do more than one bit of cleanup
> per such callback + it's harder to review as it fails the 'obviously correct
> test'.
I did not know that.
That idea missed me up until now.
Will re-spin.
Thanks
Alex
>
> Jonathan
>
> > ---
> > .../staging/iio/impedance-analyzer/ad5933.c | 59 ++++++++-----------
> > 1 file changed, 23 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > index af0bcf95ee8a..06a6dcd7883b 100644
> > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> > @@ -602,11 +602,12 @@ static const struct iio_buffer_setup_ops
> > ad5933_ring_setup_ops = {
> > .postdisable = ad5933_ring_postdisable,
> > };
> >
> > -static int ad5933_register_ring_funcs_and_init(struct iio_dev *indio_dev)
> > +static int ad5933_register_ring_funcs_and_init(struct device *dev,
> > + struct iio_dev *indio_dev)
> > {
> > struct iio_buffer *buffer;
> >
> > - buffer = iio_kfifo_allocate();
> > + buffer = devm_iio_kfifo_allocate(dev);
> > if (!buffer)
> > return -ENOMEM;
> >
> > @@ -676,6 +677,14 @@ static void ad5933_work(struct work_struct *work)
> > }
> > }
> >
> > +static void ad5933_cleanup(void *data)
> > +{
> > + struct ad5933_state *st = data;
> > +
> > + clk_disable_unprepare(st->mclk);
> > + regulator_disable(st->reg);
>
> Please do two separate callbacks so that these can be handled
> in the correct places. I.e. you do something then immediately
> register the handler to undo it.
>
> Currently you can end up disabling a clock you haven't enabled
> (which I am fairly sure will give you an error message).
>
> > +}
> > +
> > static int ad5933_probe(struct i2c_client *client,
> > const struct i2c_device_id *id)
> > {
> > @@ -703,23 +712,28 @@ static int ad5933_probe(struct i2c_client *client,
> > dev_err(&client->dev, "Failed to enable specified VDD
> > supply\n");
> > return ret;
> > }
> > +
> > + ret = devm_add_action_or_reset(&client->dev, ad5933_cleanup, st);
> > + if (ret)
> > + return ret;
> > +
> > ret = regulator_get_voltage(st->reg);
> >
> > if (ret < 0)
> > - goto error_disable_reg;
> > + return ret;
> >
> > st->vref_mv = ret / 1000;
> >
> > st->mclk = devm_clk_get(&client->dev, "mclk");
> > if (IS_ERR(st->mclk) && PTR_ERR(st->mclk) != -ENOENT) {
> > ret = PTR_ERR(st->mclk);
> > - goto error_disable_reg;
> > + return ret;
> > }
> >
> > if (!IS_ERR(st->mclk)) {
> > ret = clk_prepare_enable(st->mclk);
> > if (ret < 0)
> > - goto error_disable_reg;
> > + return ret;
> > ext_clk_hz = clk_get_rate(st->mclk);
> > }
> >
> > @@ -742,41 +756,15 @@ static int ad5933_probe(struct i2c_client *client,
> > indio_dev->channels = ad5933_channels;
> > indio_dev->num_channels = ARRAY_SIZE(ad5933_channels);
> >
> > - ret = ad5933_register_ring_funcs_and_init(indio_dev);
> > + ret = ad5933_register_ring_funcs_and_init(&client->dev, indio_dev);
> > if (ret)
> > - goto error_disable_mclk;
> > + return ret;
> >
> > ret = ad5933_setup(st);
> > if (ret)
> > - goto error_unreg_ring;
> > -
> > - ret = iio_device_register(indio_dev);
> > - if (ret)
> > - goto error_unreg_ring;
> > -
> > - return 0;
> > -
> > -error_unreg_ring:
> > - iio_kfifo_free(indio_dev->buffer);
> > -error_disable_mclk:
> > - clk_disable_unprepare(st->mclk);
> > -error_disable_reg:
> > - regulator_disable(st->reg);
> > -
> > - return ret;
> > -}
> > -
> > -static int ad5933_remove(struct i2c_client *client)
> > -{
> > - struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > - struct ad5933_state *st = iio_priv(indio_dev);
> > -
> > - iio_device_unregister(indio_dev);
> > - iio_kfifo_free(indio_dev->buffer);
> > - regulator_disable(st->reg);
> > - clk_disable_unprepare(st->mclk);
> > + return ret;
> >
> > - return 0;
> > + return devm_iio_device_register(&client->dev, indio_dev);
> > }
> >
> > static const struct i2c_device_id ad5933_id[] = {
> > @@ -801,7 +789,6 @@ static struct i2c_driver ad5933_driver = {
> > .of_match_table = ad5933_of_match,
> > },
> > .probe = ad5933_probe,
> > - .remove = ad5933_remove,
> > .id_table = ad5933_id,
> > };
> > module_i2c_driver(ad5933_driver);
next prev parent reply other threads:[~2020-05-04 5:52 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-28 9:31 [PATCH] staging: iio: ad5933: rework probe to use devm_ function variants Alexandru Ardelean
2020-05-02 18:25 ` Jonathan Cameron
2020-05-04 5:52 ` Ardelean, Alexandru [this message]
2020-05-07 9:50 ` Dan Carpenter
2020-05-08 12:43 ` Jonathan Cameron
2020-05-08 12:57 ` Mark Brown
2020-05-08 15:30 ` Jonathan Cameron
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8695e337ca71351cac2f262b0104a1411c173417.camel@analog.com \
--to=alexandru.ardelean@analog.com \
--cc=devel@driverdev.osuosl.org \
--cc=jic23@kernel.org \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).