On 01/31/2013 12:54 PM, Manuel Stahl wrote:
> 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?
Assign it to indio_dev->available_scan_masks in your probe function.
>
>>
>>> + }
>>> +
>>> + 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?
Usually it's just called ..._buffer even though it has the code for both the
trigger and the buffer.
--
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
