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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 08/28/2013 08:22 PM, Zubair Lutfullah : wrote:
> On Wed, Aug 28, 2013 at 04:18:35PM +0200, Sebastian Andrzej Siewior wrote:
>> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>>
>>> 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
>> …
>>
>>> +static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
> ...
>>> +	trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
>>> +	if (trig == NULL)
>>> +		return NULL;
>>> +
> ...
>>> +	ret = iio_trigger_register(trig);
>>> +	if (ret)
>>
>> shouldn't you free the trigger / undo iio_trigger_alloc() here?
>>
>>> +		return NULL;
>>> +
> ...
> Possible. It's done in the probe section just 
> to keep error handling bunched there.

The caller of iio_trigger *tiadc_iio_allocate_trigger() should not
cleanup after it in case of a half failure. The function itself should
be in charge of it. However you never assign the pointer so you leak
the memory here.

>>
>>>
>>> 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?
>>
>>> 	}
> Again. Error handling all in one place.

But you simply leave at the end of the function, no error handling is
done / required if you leave here. So this a zero change and has
nothing to do with the "continuous sampling support" support.

Sebastian