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@xxxxxxx>
>>> Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
>>> Cc: Juergen Beisert <jbe@xxxxxxxxxxxxxx>
>>> Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
>>> Cc: Shawn Guo <shawn.guo@xxxxxxxxxx>
>>> Cc: Wolfgang Denk <wd@xxxxxxx>
>>> ---
>>>
>>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
