| Message ID | 20200401125936.6398-3-alexandru.ardelean@analog.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] iio: kfifo: add iio_device_attach_kfifo_buffer() helper | expand |
On Wed, 1 Apr 2020 15:59:36 +0300 Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > This driver calls iio_kfifo_allocate() vs devm_iio_kfifo_allocate(). But > the conversion is still simpler here, and cleans-up/reduces some error > paths. > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> This mixes devm managed stuff an unmanaged. Hence it fails the 'obviously correct' test. If you wanted to do this you'd first need to sort out the unmanaged bits to be automatically unwound (regulators and clocks). Or potentially reorder the driver so those happen after this allocation is done. Thanks, Jonathan > --- > .../staging/iio/impedance-analyzer/ad5933.c | 28 ++++--------------- > 1 file changed, 5 insertions(+), 23 deletions(-) > > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c > index af0bcf95ee8a..7bde93c6dd74 100644 > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c > @@ -602,22 +602,6 @@ 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) > -{ > - struct iio_buffer *buffer; > - > - buffer = iio_kfifo_allocate(); > - if (!buffer) > - return -ENOMEM; > - > - iio_device_attach_buffer(indio_dev, buffer); > - > - /* Ring buffer functions - here trigger setup related */ > - indio_dev->setup_ops = &ad5933_ring_setup_ops; > - > - return 0; > -} > - > static void ad5933_work(struct work_struct *work) > { > struct ad5933_state *st = container_of(work, > @@ -738,26 +722,25 @@ static int ad5933_probe(struct i2c_client *client, > indio_dev->dev.parent = &client->dev; > indio_dev->info = &ad5933_info; > indio_dev->name = id->name; > - indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE); > + indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->channels = ad5933_channels; > indio_dev->num_channels = ARRAY_SIZE(ad5933_channels); > > - ret = ad5933_register_ring_funcs_and_init(indio_dev); > + ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE, > + &ad5933_ring_setup_ops); > if (ret) > goto error_disable_mclk; > > ret = ad5933_setup(st); > if (ret) > - goto error_unreg_ring; > + goto error_disable_mclk; > > ret = iio_device_register(indio_dev); > if (ret) > - goto error_unreg_ring; > + goto error_disable_mclk; > > return 0; > > -error_unreg_ring: > - iio_kfifo_free(indio_dev->buffer); > error_disable_mclk: > clk_disable_unprepare(st->mclk); > error_disable_reg: > @@ -772,7 +755,6 @@ static int ad5933_remove(struct i2c_client *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); >
On Sun, 2020-04-05 at 11:49 +0100, Jonathan Cameron wrote: > [External] > > On Wed, 1 Apr 2020 15:59:36 +0300 > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote: > > > This driver calls iio_kfifo_allocate() vs devm_iio_kfifo_allocate(). But > > the conversion is still simpler here, and cleans-up/reduces some error > > paths. > > > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> > > This mixes devm managed stuff an unmanaged. Hence it fails the 'obviously > correct' > test. If you wanted to do this you'd first need to sort out the unmanaged > bits to be automatically unwound (regulators and clocks). Or potentially > reorder > the driver so those happen after this allocation is done. > Yeah. I was a bit sloppy here. I think tried a broader cleanup/rework would be a better idea here. > Thanks, > > Jonathan > > > --- > > .../staging/iio/impedance-analyzer/ad5933.c | 28 ++++--------------- > > 1 file changed, 5 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c > > b/drivers/staging/iio/impedance-analyzer/ad5933.c > > index af0bcf95ee8a..7bde93c6dd74 100644 > > --- a/drivers/staging/iio/impedance-analyzer/ad5933.c > > +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c > > @@ -602,22 +602,6 @@ 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) > > -{ > > - struct iio_buffer *buffer; > > - > > - buffer = iio_kfifo_allocate(); > > - if (!buffer) > > - return -ENOMEM; > > - > > - iio_device_attach_buffer(indio_dev, buffer); > > - > > - /* Ring buffer functions - here trigger setup related */ > > - indio_dev->setup_ops = &ad5933_ring_setup_ops; > > - > > - return 0; > > -} > > - > > static void ad5933_work(struct work_struct *work) > > { > > struct ad5933_state *st = container_of(work, > > @@ -738,26 +722,25 @@ static int ad5933_probe(struct i2c_client *client, > > indio_dev->dev.parent = &client->dev; > > indio_dev->info = &ad5933_info; > > indio_dev->name = id->name; > > - indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE); > > + indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->channels = ad5933_channels; > > indio_dev->num_channels = ARRAY_SIZE(ad5933_channels); > > > > - ret = ad5933_register_ring_funcs_and_init(indio_dev); > > + ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE, > > + &ad5933_ring_setup_ops); > > if (ret) > > goto error_disable_mclk; > > > > ret = ad5933_setup(st); > > if (ret) > > - goto error_unreg_ring; > > + goto error_disable_mclk; > > > > ret = iio_device_register(indio_dev); > > if (ret) > > - goto error_unreg_ring; > > + goto error_disable_mclk; > > > > return 0; > > > > -error_unreg_ring: > > - iio_kfifo_free(indio_dev->buffer); > > error_disable_mclk: > > clk_disable_unprepare(st->mclk); > > error_disable_reg: > > @@ -772,7 +755,6 @@ static int ad5933_remove(struct i2c_client *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); > >
diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c index af0bcf95ee8a..7bde93c6dd74 100644 --- a/drivers/staging/iio/impedance-analyzer/ad5933.c +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c @@ -602,22 +602,6 @@ 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) -{ - struct iio_buffer *buffer; - - buffer = iio_kfifo_allocate(); - if (!buffer) - return -ENOMEM; - - iio_device_attach_buffer(indio_dev, buffer); - - /* Ring buffer functions - here trigger setup related */ - indio_dev->setup_ops = &ad5933_ring_setup_ops; - - return 0; -} - static void ad5933_work(struct work_struct *work) { struct ad5933_state *st = container_of(work, @@ -738,26 +722,25 @@ static int ad5933_probe(struct i2c_client *client, indio_dev->dev.parent = &client->dev; indio_dev->info = &ad5933_info; indio_dev->name = id->name; - indio_dev->modes = (INDIO_BUFFER_SOFTWARE | INDIO_DIRECT_MODE); + indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->channels = ad5933_channels; indio_dev->num_channels = ARRAY_SIZE(ad5933_channels); - ret = ad5933_register_ring_funcs_and_init(indio_dev); + ret = iio_device_attach_kfifo_buffer(indio_dev, INDIO_BUFFER_SOFTWARE, + &ad5933_ring_setup_ops); if (ret) goto error_disable_mclk; ret = ad5933_setup(st); if (ret) - goto error_unreg_ring; + goto error_disable_mclk; ret = iio_device_register(indio_dev); if (ret) - goto error_unreg_ring; + goto error_disable_mclk; return 0; -error_unreg_ring: - iio_kfifo_free(indio_dev->buffer); error_disable_mclk: clk_disable_unprepare(st->mclk); error_disable_reg: @@ -772,7 +755,6 @@ static int ad5933_remove(struct i2c_client *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);
This driver calls iio_kfifo_allocate() vs devm_iio_kfifo_allocate(). But the conversion is still simpler here, and cleans-up/reduces some error paths. Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com> --- .../staging/iio/impedance-analyzer/ad5933.c | 28 ++++--------------- 1 file changed, 5 insertions(+), 23 deletions(-)