Re: [PATCH 1/2] iio: adc: Cosmic Circuits 10001 ADC driver

[Jonathan Cameron <jic23@kernel.org>]

On 04/11/14 23:29, Ezequiel Garcia wrote:
> Hi everyone,
> 
> Thanks for the review. I've fixed most of the comments except for the
> invalid sample representation issue.
> 
> On 10/31/2014 02:19 PM, Lars-Peter Clausen wrote:
>> On 10/29/2014 09:45 PM, Ezequiel Garcia wrote:
>>> From: Phani Movva <Phani.Movva@imgtec.com>
>>>
>>> This commit adds support for Cosmic Circuits 10001 10-bit ADC device.
>>>
>>> Signed-off-by: Phani Movva <Phani.Movva@imgtec.com>
>>> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
>>> [Ezequiel: code style cleaning]
>>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
>>
>> Looks very good. Just a few very minor issues.
>>
>> [...]
>>> +static int cc_adc_poll_done(struct iio_dev *dev, int channel,
>>> +                unsigned int delay)
>>> +{
>>> +    struct cc_10001_adc_device *adc_dev = iio_priv(dev);
>>> +    int val = INVALID_SAMPLED_OUTPUT;
>>
>> I'm not so sure that returning a fake sample is such a good idea. When
>> reading from sysfs we should definitely return an error if there is one.
> 
> Right.
> 
>> For buffer reading dropping the sample is probably not such a good idea,
>> but we should agree on and document a standard representation of invalid
>> samples.
>>
> 
> Hm, sure. What do you suggest? I can't see other drivers doing anything
> like this.
> 
It's ancient, but I vaguely recall one driver (can't find it right now
- may no longer exist) in which the try to reenable on the trigger
could detect that it had missed a sample (as the interrupt had
not fallen) and so would fire off the trigger again...
In that particular case we set the timestamp to 0 to indicate that we
new there was a sample but didn't know when it was captured.

Not really the same, but there are nasty to handle corner cases we ought
to include in any discussion of invalid data...

Is this a real issue - or are we talking, in event of hardware failure or
bug?  If so I'd spit a warning out to the logs and probably push nothing
at all into the buffer.

A magic flag on the buffer would be nice, but in general there is nowhere
to put it without in some cases greatly increasing the buffer element size.
I suppose we could have an error flag for the buffer to say, 'something
missed' though...

Alternatively we 'could' have drivers expose their own 'INVALID' flag via
a sysfs attribute...