Re: [PATCH v3] iio: add adjd_s311 I2C digital color sensor driver

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

On 07/02/2012 11:28 AM, Lars-Peter Clausen wrote:
> On 07/01/2012 12:20 PM, Peter Meerwald wrote:
>> sensor has 4 channels (10-bit each, R/G/B and clear), sensitivity
>> and gain is controlled in the driver by ext_info integration_time
>> and CHAN_INFO_HARDWAREGAIN
>>
>> driver supports triggered buffer and IIO_CHAN_INFO_RAW to get the
>> sensor data
>>
>> patch depends on IIO_MOD_LIGHT_RED etc, the patch providing that
>> (http://permalink.gmane.org/gmane.linux.kernel.iio/4354) has not been
>> merged yet, a new proposal is following
>>
>> v3:
>> * fix warnings
>>
>> v2: address comments by Lars-Peter Clausen
>> * buffer allocation now in update_scan_mode instead of in trigger
>>   handler
>> * simplify trigger code (assume active_scan_mask is not empty, use
>>   for_each_set_bit, use iio_push_to_buffer)
>> * reorder entry in Makefile and Kconfig
>> * fix remove
>>
>> Signed-off-by: Peter Meerwald <pmeerw@pmeerw.net>
> 
> 
> Looks pretty good to me. One issue in the IRQ handler and two minor
> suggestions inline.
> 
>> ---
>>  drivers/iio/light/Kconfig     |   12 ++
>>  drivers/iio/light/Makefile    |    1 +
>>  drivers/iio/light/adjd_s311.c |  395 +++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 408 insertions(+)
>>  create mode 100644 drivers/iio/light/adjd_s311.c
>> [...]
>> +
>> +static ssize_t adjd_s311_write_int_time(struct iio_dev *indio_dev,
>> +	 uintptr_t private, const struct iio_chan_spec *chan, const char *buf,
>> +	 size_t len)
>> +{
>> +	struct adjd_s311_data *data = iio_priv(indio_dev);
>> +	unsigned long int_time;
>> +	int ret;
>> +
>> +	ret = kstrtoul(buf, 10, &int_time);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (int_time > ADJD_S311_INT_MASK)
>> +		return -EINVAL;
>> +
>> +	ret = i2c_smbus_write_word_data(data->client,
>> +		ADJD_S311_INT_REG(chan->address), int_time);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return ret ? ret : len;
> 
> ret will always be zero here.
> 
>> +}
>> +
>> +static irqreturn_t adjd_s311_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct adjd_s311_data *data = iio_priv(indio_dev);
>> +	struct iio_buffer *buffer = indio_dev->buffer;
>> +	int len = 0;
>> +	int i, j = 0;
>> +
>> +	int ret = adjd_s311_req_data(indio_dev);
>> +	if (ret < 0)
>> +		return ret;
> 
> I wouldn't return ret here since this is the interrupt handler and I'm not
> quite sure how the core reacts if it gets a value which is not a
> irqreturn_t, especially considering that irqreturn_t is basically unsigned.
> You also have to call iio_trigger_notify_done before leaving the function
> 

Looks like you are not alone with this,  I've just wrote a cocci patch which
finds paths which return form the trigger handler without calling
iio_trigger_notify_done and it triggers on quite a few other drivers as well.

@r1@
identifier fn;
identifier indio_dev;
expression bh;
@@
(
iio_alloc_pollfunc(bh, &fn, ...)
|
iio_alloc_pollfunc(bh, fn, ...)
|
iio_triggered_buffer_setup(indio_dev, bh, fn, ...)
)
@@
typedef irqreturn_t;
identifier r1.fn;
@@
irqreturn_t fn(...)
{
... when != iio_trigger_notify_done(...)
    when any
* return ...;
 ... when any
}