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@xxxxxxxxxx> > > > 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 } -- 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
