Re: [PATCH 2/3 V2] IIO: Add basic MXS LRADC driver

[Lars-Peter Clausen <lars@metafoo.de>]

On 08/12/2012 01:11 AM, Marek Vasut wrote:
> Dear Lars-Peter Clausen,
> 
>> On 08/03/2012 05:28 PM, Marek Vasut wrote:
>>> This driver is very basic. It supports userland trigger, buffer and
>>> raw access to channels. The support for delay channels is missing
>>> altogether.
>>
>> Looks mostly good to me. Some comments inline.
>>
>> I think you need to provide a documentation for the devicetree binding,
>> even though it's a rather simple binding.
> 
> Definitelly, will add it.
> 
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> Cc: Jonathan Cameron <jic23@kernel.org>
>>> Cc: Juergen Beisert <jbe@pengutronix.de>
>>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>>> Cc: Shawn Guo <shawn.guo@linaro.org>
>>> Cc: Wolfgang Denk <wd@denx.de>
>>> ---
>>>
>>>  drivers/staging/iio/adc/Kconfig     |   12 +
>>>  drivers/staging/iio/adc/Makefile    |    1 +
>>>  drivers/staging/iio/adc/mxs-lradc.c |  591
>>>  +++++++++++++++++++++++++++++++++++ 3 files changed, 604 insertions(+)
>>>  create mode 100644 drivers/staging/iio/adc/mxs-lradc.c
>>>
>>> V2: Use delay channel 0 in case of buffered sampling so the samples are
>>> deployed
>>>
>>>     continuously.
>>>     Disallow RAW sampling while buffered mode is enabled to simplify
>>>     code.
>>>
>>> diff --git a/drivers/staging/iio/adc/Kconfig
>>> b/drivers/staging/iio/adc/Kconfig index 67711b7..97ca697 100644
>>> --- a/drivers/staging/iio/adc/Kconfig
>>> +++ b/drivers/staging/iio/adc/Kconfig
>>
>> [...]
>>
>>> + */
>>> +static int mxs_lradc_read_raw(struct iio_dev *iio_dev,
>>> +			const struct iio_chan_spec *chan,
>>> +			int *val, int *val2, long m)
>>> +{
>>> [...]
>>> +
>>> +	init_completion(&lradc->completion);
>>
>> This should rather be INIT_COMPLETION. init_completion should only called
>> once in probe, since it will reinitialize the spinlock which opens up race
>> conditions.
>>
>>> [...]
>>> +
>>> +	/* Wait for completion on the channel, 1 second max. */
>>> +	ret = wait_for_completion_killable_timeout(&lradc->completion,
>>> +						msecs_to_jiffies(1000));
>>
>> msecs_to_jiffies(1000) = HZ, but I guess both is OK.
> 
> Even on NOHZ kernel?

Yes HZ is always the number of jiffies per second.

> 
>>> [...]
>>> +}
>>
>> [...]
>>
>>> +
>>> +static int __devinit mxs_lradc_probe(struct platform_device *pdev)
>>> +{
>>> [...]
>>> +
>>> +	/* Grab all IRQ sources */
>>> +	for (i = 0; i < 13; i++) {
>>> +		lradc->irq[i] = platform_get_irq(pdev, i);
>>> +		if (lradc->irq[i] < 0) {
>>> +			ret = -EINVAL;
>>> +			goto err_addr;
>>> +		}
>>> +
>>> +		ret = devm_request_irq(dev, lradc->irq[i],
>>> +					mxs_lradc_handle_irq, 0,
>>> +					mxs_lradc_irq_name[i], iio);
>>
>> devm_request_irq is a bit dangerous as long we do not have a
>> devm_iio_device_alloc. The IRQ will only be freed after the memory for the
>> IIO device has been freed, which means there is a slight window where the
>> IRQ could fire although the memory has already been freed.
> 
> The IRQ is disabled, see mxs_lradc_hw_stop(), so it should be all right.

Ok.