Am Mittwoch, 30. Januar 2013, 16:22:32 schrieb Lars-Peter Clausen:
> On 01/29/2013 03:59 PM, Manuel Stahl wrote:
> > Signed-off-by: Manuel Stahl <manuel.stahl@xxxxxxxxxxxxxxxxx>
>
> Hi,
>
> Looks mostly good. One usage of outdated API and otherwise mostly just minor
> style issues.
Ah sorry, I used 3.7 as reference.
[...]
> > +static irqreturn_t itg3200_trigger_handler(int irq, void *p)
> > +{
> > + struct iio_poll_func *pf = p;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct itg3200 *st = iio_priv(indio_dev);
> > + struct iio_buffer *buffer = indio_dev->buffer;
> > + __be16 buf[ITG3200_SCAN_ELEMENTS + sizeof(s64)/sizeof(u16)];
> > +
> > + /* Clear IRQ */
> > + itg3200_read_irq_status(indio_dev);
> > +
> > + if (!bitmap_empty(indio_dev->active_scan_mask, indio_dev->masklength)) {
> > + int ret;
> > + unsigned i, j = 0;
> > +
> > + ret = itg3200_read_all_channels(st->i2c, buf);
> > + if (ret < 0)
> > + goto error_ret;
> > +
> > + /* Select only active scan elements */
> > + for (i = 0; i < ITG3200_SCAN_ELEMENTS; i++)
> > + if (iio_scan_mask_query(indio_dev, buffer, i))
> > + buf[j++] = buf[i];
>
> The IIO core can take care of this kind of demuxing for you. If you set
> available_scan_masks to { 0xffff..., 0x0 } it will know that the device will
> only be able to sample all channels at once. A user will still be able to
> select a subset of channels and the IIO core will take care of picking the
> right samples.
Where do I set the available_scan_masks?
>
> > + }
> > +
> > + if (indio_dev->scan_timestamp)
> > + memcpy(buf + indio_dev->scan_bytes - sizeof(s64),
> > + &pf->timestamp, sizeof(pf->timestamp));
> > +
> > + iio_push_to_buffer(buffer, (u8 *)buf, pf->timestamp);
>
> This won't work in the latest version of IIO, use
>
> iio_push_to_buffers(indio_dev, (u8 *)buf); instead
[...]
> Do we really need this wrapper? I think it might be better to move
> itg3200_set_irq_data_rdy here, especially considering that it is unused in
> case buffer support is not enabled.
>
> Similar I think it makes sense to move itg3200_read_all_channels and
> itg3200_read_irq_status to tg3200_buffer.c
That's OK, I just need to put the register definitions into the header file.
Actually itg3200_read_irq_status is not needed anyway since we use a
non latched IRQ line.
I would also combine itg3200_buffer.c and itg3200_trigger.c into a single file.
Any suggestions for the name?
--
Regards,
Manuel Stahl
--
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
