Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support

On Wed, Aug 28, 2013 at 04:18:35PM +0200, Sebastian Andrzej Siewior wrote:

* Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:

quoted

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index a952538..ae2202b 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c

…

quoted

+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)

...

quoted

+	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
+	if (trig == NULL)
+		return NULL;
+

...

quoted

+	ret = iio_trigger_register(trig);
+	if (ret)

shouldn't you free the trigger / undo iio_trigger_alloc() here?

quoted

+		return NULL;
+

...
Possible. It's done in the probe section just 
to keep error handling bunched there.

quoted

static const char * const chan_name_ain[] = {

@@ -220,7 +394,8 @@ static int tiadc_probe(struct platform_device *pdev)

					  sizeof(struct tiadc_device));
	if (indio_dev == NULL) {
		dev_err(&pdev->dev, "failed to allocate iio device\n");
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto err_ret;

Why the jump instead of leave right away?

quoted

	}

Again. Error handling all in one place.

quoted

@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)

	struct tiadc_device *adc_dev = iio_priv(indio_dev);
	u32 step_en;

+	free_irq(adc_dev->irq, indio_dev);
	iio_device_unregister(indio_dev);
+	iio_buffer_unregister(indio_dev);
	tiadc_channels_remove(indio_dev);

I think you need also to

	iio_trigger_unregister(adc_dev->trig);
	iio_trigger_free(adc_dev->trig);

here.

Indeed.
Thanks for pointing it out.

Zubair