The driver doesn't aim to replace what has existed in iio core. Just
improvement and a module which can be shared by most devices. Devices
which have special path can still self-define like before.
On Fri, Jun 11, 2010 at 6:22 PM, Jonathan Cameron <jic23@xxxxxxxxx> wrote:
> On 06/11/10 04:19, Barry Song wrote:
>> On Thu, Jun 10, 2010 at 9:48 PM, Jonathan Cameron <jic23@xxxxxxxxx> wrote:
>>> On 06/10/10 05:48, Barry Song wrote:
>>>> Just RFC for discussion. It is not the last patch To keep the change
>>>> lest, i extract the common factor for most iio devices to a common
>>>> ring trigger module. Most devices can use the interfaces in this
>>>> module directly. So copy-paste for ring and trigger is not necessary
>>>> now.
>>> Hi Barry,
>>>
>>> I'm afraid I'm not in a position to take a particularly
>>> close look at this right now (on a rather slow net connection)
>>> Thanks for taking a look at what we can unify.
>>>
>>> My immediate thought is that this patch may go a little too far,
>>> certainly at this stage. It produces generic support for a
>>> particular type of trigger (fine and useful) but I'm not
>>> sure this should be grouped with the ring buffer setup elements.
>>> There are plenty of devices that would benefit from parts of this
>>> code, but currently it is all of the take it or leave it variety.
>>>
>>> In particular I'm not sure the ring buffer filling code should
>>> be in here. It really isn't as consistent as the code implies
>>> and by the presense of that element you have reduces the general
>>> usefulness of rest. I'm very doubtful that we want to move things
>>> like the rx and tx buffers out of the device drivers themselves.
>>> The device drivers should control all access to the device,
>>> not a core driver like this.
>> Basically, rx and tx can be moved into the priv of iio_state. Or
>> iio_state can be moved into iio_dev? Then this structure is not
>> needed?
> They could but I still think these should be part of the locale
> device driver state. I agree they don't need to be, but conceptualy
> to my mind everything to do with actual communications with the chip
> is not something the core (or helper modules like this one) should
> touch.
The bottom level hardware read/write can be part of the device driver.
But the flow can be in the core.
The iio_state is derived from objects like nand_chip, nand_base can
handle data flow, the bottom_level nand driver just fill the last
hardware access entries.
>>>
>>> Currently we don't have internal hooks to the contents
>>> of the 'scans '. These could be added, but I think that would
>>> add more complexity than simply allowing the drivers to handle
>>> this aspect, not to mention adding some unecessary time penalty
>>> to this path.
>>>
>>>>
>>>> For iio core changes:
>>>> 1. extract xxx_state to iio_state and add a private data to contain
>>>> chip-specific data
>>>> 2. extract all xxx_ring/xxx_trigger api to common api
>>>>
>>>> Index: drivers/staging/iio/iio.h
>>>> ===================================================================
>>>> --- drivers/staging/iio/iio.h (revision 8893)
>>>> +++ drivers/staging/iio/iio.h (working copy)
>>>> @@ -447,4 +447,46 @@
>>>>
>>>> int iio_get_new_idr_val(struct idr *this_idr);
>>>> void iio_free_idr_val(struct idr *this_idr, int id);
>>>> +
>>>> +/**
>>>> + * struct iio_state - hardware level hooks for iio device
>>>> + * @work_trigger_to_ring: bh for triggered event handling
>>>> + * @work_cont_thresh: CLEAN
>>>> + * @inter: used to check if new interrupt has been triggered
>>>> + * @last_timestamp: passing timestamp from th to bh of interrupt handler
>>>> + * @indio_dev: industrial I/O device structure
>>>> + * @trig: data ready trigger registered with iio
>>>> + * @tx: transmit buffer
>>>> + * @rx: recieve buffer
>>>> + * @buf_lock: mutex to protect tx and rx
>>>> + * @irq: interrupt number
>>>> + * @set_irq: control the disable/enable of external interrupt
>>>> + * @hw_read_ring: read ring data from hardware by spi/i2c.
>>>> + **/
>>>> +struct iio_state {
>>>> + struct device *parent_dev;
>>>> + struct work_struct work_trigger_to_ring;
>>>
>>>> + struct iio_work_cont work_cont_thresh;
>>>> + s64 last_timestamp;
>>>> + struct iio_dev *indio_dev;
>>>> + struct iio_trigger *trig;
>>>> + u8 *tx;
>>>> + u8 *rx;
>>>> + struct mutex buf_lock;
>>>> + int irq;
>>>> + int (*set_irq)(struct iio_state *st, bool enable);
>>>> + int (*hw_read_ring)(struct iio_state *st, u8 *rx);
>>>> + void *priv;
>>>> +};
>>>> +
>>>> +static inline void iio_state_set_privdata(struct iio_state *st, void *data)
>>>> +{
>>>> + st->priv = data;
>>>> +}
>>>> +
>>>> +static inline void * iio_state_get_privdata(struct iio_state *st)
>>>> +{
>>>> + return st->priv;
>>>> +}
>>>> +
>>>> #endif /* _INDUSTRIAL_IO_H_ */
>>>> Index: drivers/staging/iio/common-ring-trigger.c
>>>> ===================================================================
>>>> --- drivers/staging/iio/common-ring-trigger.c (revision 0)
>>>> +++ drivers/staging/iio/common-ring-trigger.c (revision 0)
>>>> @@ -0,0 +1,277 @@
>>>> +/* The software ring with trigger which can be used by most drivers
>>>> + *
>>>> + * Copyright (c) 2010 Barry Song
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms of the GNU General Public License version 2 as published by
>>>> + * the Free Software Foundation.
>>>> + *
>>>> + */
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/fs.h>
>>>> +#include <linux/poll.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/cdev.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include "iio.h"
>>>> +#include "ring_generic.h"
>>>> +#include "trigger.h"
>>>> +#include "ring_sw.h"
>>>> +
>>>> +/*
>>>> + * combine_8_to_16(): utility function to munge to u8s into u16
>>>> + */
>>>> +static inline u16 combine_8_to_16(u8 lower, u8 upper)
>>>> +{
>>>> + u16 _lower = lower;
>>>> + u16 _upper = upper;
>>>> + return _lower | (_upper << 8);
>>>> +}
>>>> +
>>>> +static void iio_common_ring_poll_func_th(struct iio_dev *indio_dev)
>>>> +{
>>>> + struct iio_state *st = iio_dev_get_devdata(indio_dev);
>>>> + st->last_timestamp = indio_dev->trig->timestamp;
>>>> + schedule_work(&st->work_trigger_to_ring);
>>>> +}
>>>> +/
>>>
>>> This function is the one that mainly bothers me. We might
>>> have a number of devices that do fit in here but there are
>>> others that would use pretty much everything else but
>>> this. I'd possibly be more inclined to leave this
>>> to the drivers. That is where the knowledge about the
>>> data layout in 'rx' comes from.
>>
>> Here we can let the hw_read_ring() do more, not only read hw, but also
>> fill the last data buffer, like combine_8_to_16().
> but once it is doing that much, what is te point of the following?
> You will save a few repeated lines, but at the cost of driver
> readability. I'm yet to be convinced that this function should exist.
> Why not pass the appropriate work struct in having initialized it
> in the chip driver? There are certainly elements of this code that
> will not be ideal for every device anyway (such as the kmalloc
> being in here!)
not just several saved lines. But mean the bottom level driver doesn't
need to care the flow of trigger_bh_to_ring().
Bottom level driver doesn't need to self-define its
trigger_bh_to_ring(), it only read hardware data and fill the data
buffer. That's what the bottom level driver wants to do.
>>
>>>> +static void iio_common_trigger_bh_to_ring(struct work_struct *work_s)
>>>> +{
>>>> + struct iio_state *st
>>>> + = container_of(work_s, struct iio_state,
>>>> + work_trigger_to_ring);
>>>> +
>>>> + int i = 0;
>>>> + s16 *data;
>>>> + size_t datasize = st->indio_dev
>>>> + ->ring->access.get_bpd(st->indio_dev->ring);
>>>> +
>>>> + data = kmalloc(datasize , GFP_KERNEL);
>>>> + if (data == NULL) {
>>>> + dev_err(st->parent_dev, "memory alloc failed in ring bh");
>>>> + return;
>>>> + }
>>>> +
>>>> + if (st->indio_dev->scan_count)
>>>> + if (st->hw_read_ring(st, st->rx) >= 0)
>>>> + for (; i < st->indio_dev->scan_count; i++) {
>>>> + data[i] = combine_8_to_16(st->rx[i*2+1],
>>>> + st->rx[i*2]);
>>>> + }
>>>> +
>>>> + /* Guaranteed to be aligned with 8 byte boundary */
>>>> + if (st->indio_dev->scan_timestamp)
>>>> + *((s64 *)(data + ((i + 3)/4)*4)) = st->last_timestamp;
>>>> +
>>>> + st->indio_dev->ring->access.store_to(st->indio_dev->ring,
>>>> + (u8 *)data,
>>>> + st->last_timestamp);
>>>> +
>>>> + iio_trigger_notify_done(st->indio_dev->trig);
>>>> + kfree(data);
>>>> +
>>>> + return;
>>>> +}
>>>> +
>>>> +static int iio_common_ring_preenable(struct iio_dev *indio_dev)
>>>> +{
>>>> + size_t size;
>>>> + dev_dbg(&indio_dev->dev, "%s\n", __func__);
>>>> + /* Check if there are any scan elements enabled, if not fail*/
>>>> + if (!(indio_dev->scan_count || indio_dev->scan_timestamp))
>>>> + return -EINVAL;
>>>> +
>>>> + if (indio_dev->ring->access.set_bpd) {
>>>> + if (indio_dev->scan_timestamp)
>>>> + if (indio_dev->scan_count)
>>>> + /* Timestamp (aligned to s64) and data */
>>>> + size = (((indio_dev->scan_count * sizeof(s16))
>>>> + + sizeof(s64) - 1)
>>>> + & ~(sizeof(s64) - 1))
>>>> + + sizeof(s64);
>>>> + else /* Timestamp only */
>>>> + size = sizeof(s64);
>>>> + else /* Data only */
>>>> + size = indio_dev->scan_count*sizeof(s16);
>>>> + indio_dev->ring->access.set_bpd(indio_dev->ring, size);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int iio_common_ring_postenable(struct iio_dev *indio_dev)
>>>> +{
>>>> + return indio_dev->trig
>>>> + ? iio_trigger_attach_poll_func(indio_dev->trig,
>>>> + indio_dev->pollfunc)
>>>> + : 0;
>>>> +}
>>>> +
>>>> +static int iio_common_ring_predisable(struct iio_dev *indio_dev)
>>>> +{
>>>> + return indio_dev->trig
>>>> + ? iio_trigger_dettach_poll_func(indio_dev->trig,
>>>> + indio_dev->pollfunc)
>>>> + : 0;
>>>> +}
>>>> +
>>>> +int iio_configure_common_ring(struct iio_dev *indio_dev)
>>>> +{
>>>> + int ret = 0;
>>>> + struct iio_state *st = indio_dev->dev_data;
>>>> + struct iio_ring_buffer *ring;
>>>> + INIT_WORK(&st->work_trigger_to_ring, iio_common_trigger_bh_to_ring);
>>>> +
>>>> + ring = iio_sw_rb_allocate(indio_dev);
>>>> + if (!ring) {
>>>> + ret = -ENOMEM;
>>>> + return ret;
>>>> + }
>>>> + indio_dev->ring = ring;
>>>> +
>>>> + iio_ring_sw_register_funcs(&ring->access);
>>>> + ring->preenable = &iio_common_ring_preenable;
>>>> + ring->postenable = &iio_common_ring_postenable;
>>>> + ring->predisable = &iio_common_ring_predisable;
>>>> + ring->owner = THIS_MODULE;
>>>> +
>>>> + indio_dev->pollfunc = kzalloc(sizeof(*indio_dev->pollfunc), GFP_KERNEL);
>>>> + if (indio_dev->pollfunc == NULL) {
>>>> + ret = -ENOMEM;
>>>> + goto error_iio_sw_rb_free;;
>>>> + }
>>>> + indio_dev->pollfunc->poll_func_main = &iio_common_ring_poll_func_th;
>>>> + indio_dev->pollfunc->private_data = indio_dev;
>>>> + indio_dev->modes |= INDIO_RING_TRIGGERED;
>>>> + return 0;
>>>> +
>>>> +error_iio_sw_rb_free:
>>>> + iio_sw_rb_free(indio_dev->ring);
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(iio_configure_common_ring);
>>>> +
>>>> +void iio_unconfigure_common_ring(struct iio_dev *indio_dev)
>>>> +{
>>>> + kfree(indio_dev->pollfunc);
>>>> + iio_sw_rb_free(indio_dev->ring);
>>>> +}
>>>> +EXPORT_SYMBOL(iio_unconfigure_common_ring);
>>>> +
>>> This next one is pointless except possibly to unify
>>> some naming so why do it?
>> Just a copy-paste and rename for existing code. Here we can let driver
>> call iio_ring_buffer_register/iio_ring_buffer_unregister directly?
> exactly.
>>>> +int iio_initialize_common_ring(struct iio_ring_buffer *ring)
>>>> +{
>>>> + return iio_ring_buffer_register(ring, 0);
>>>> +}
>>>> +EXPORT_SYMBOL(iio_initialize_common_ring);
>>>> +
>>>> +void iio_uninitialize_common_ring(struct iio_ring_buffer *ring)
>>>> +{
>>>> + iio_ring_buffer_unregister(ring);
>>>> +}
>>>> +EXPORT_SYMBOL(iio_uninitialize_common_ring);
>>>> +
>>>> +static int iio_common_trigger_poll(struct iio_dev *dev_info,
>>>> + int index,
>>>> + s64 timestamp,
>>>> + int no_test)
>>>> +{
>>>> + struct iio_state *st = iio_dev_get_devdata(dev_info);
>>>> + struct iio_trigger *trig = st->trig;
>>>> +
>>>> + trig->timestamp = timestamp;
>>>> + iio_trigger_poll(trig);
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +IIO_EVENT_SH(common_trig, &iio_common_trigger_poll);
>>>> +
>>>> +static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
>>>> +
>>>> +static struct attribute *iio_trigger_attrs[] = {
>>>> + &dev_attr_name.attr,
>>>> + NULL,
>>>> +};
>>>> +
>>>> +static const struct attribute_group iio_trigger_attr_group = {
>>>> + .attrs = iio_trigger_attrs,
>>>> +};
>>>> +
>>>> +static int iio_common_trigger_try_reen(struct iio_trigger *trig)
>>>> +{
>>>> + struct iio_state *st = trig->private_data;
>>>> + enable_irq(st->irq);
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int iio_common_trigger_set_state(struct iio_trigger *trig,
>>>> + bool state)
>>>> +{
>>>> + struct iio_state *st = trig->private_data;
>>>> + struct iio_dev *indio_dev = st->indio_dev;
>>>> + int ret = 0;
>>>> +
>>>> + dev_dbg(&indio_dev->dev, "%s (%d)\n", __func__, state);
>>>> + ret = st->set_irq(st, state);
>>> This again is making some nasty assumptions. Yes some drivers
>>> use a shared event to supply dat ready events, others do not
>>> and if the line isn't shared they shouldn't as it adds a fair
>>> bit of complexity to this fast path.
>> you mean we should know the right interrupt line for data ready event
>> if there are more than one indio_dev->interrupts elemant?
>
> no. I meant that indio_dev->interrupts is meant to be NULL in those
> cases where the only interrupt (on a given line) is used for triggering.
> The situation
> used here is only meant to occur if the same line may indicate data
> ready or say a threshold interrupt and has to check which it is.
> If we only have a dataready signal this path adds considerably
> complexity for no reason. The reason I didn't comment on this in
> your drivers is that I assumed that event functionality was in the
> works. As it currently stands none of them should be using this
> method, they should all be using and handling the interrupt directly.
> The reason this complexity exists in the lis3l02dq is that it only
> has a single physical interrupt line an this is shared between data
> ready and theshold events. In my original adis16350 I dedicated
> one line to the alarms and one to the data ready. Obviously this
> is a bit restrictive on possible wiring so it may make sense to go
> the more complex route in software.
>>
>>>> + if (state == false) {
>>>> + iio_remove_event_from_list(&iio_event_common_trig,
>>>> + &indio_dev->interrupts[0]
>>>> + ->ev_list);
>>>> + flush_scheduled_work();
>>>> + } else {
>>>> + iio_add_event_to_list(&iio_event_common_trig,
>>>> + &indio_dev->interrupts[0]->ev_list);
>>>> + }
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int iio_probe_common_trigger(struct iio_dev *indio_dev)
>>>> +{
>>>> + int ret;
>>>> + struct iio_state *st = indio_dev->dev_data;
>>>> +
>>>> + st->trig = iio_allocate_trigger();
>>>> + st->trig->name = kmalloc(IIO_TRIGGER_NAME_LENGTH, GFP_KERNEL);
>>>> + if (!st->trig->name) {
>>>> + ret = -ENOMEM;
>>>> + goto error_free_trig;
>>>> + }
>>>> + snprintf((char *)st->trig->name,
>>>> + IIO_TRIGGER_NAME_LENGTH,
>>>> + "iio-dev%d", indio_dev->id);
>>>> + st->trig->dev.parent = st->parent_dev;
>>>> + st->trig->owner = THIS_MODULE;
>>>> + st->trig->private_data = st;
>>>> + st->trig->set_trigger_state = &iio_common_trigger_set_state;
>>>> + st->trig->try_reenable = &iio_common_trigger_try_reen;
>>>> + st->trig->control_attrs = &iio_trigger_attr_group;
>>>> + ret = iio_trigger_register(st->trig);
>>>> +
>>>> + /* select default trigger */
>>>> + indio_dev->trig = st->trig;
>>>> + if (ret)
>>>> + goto error_free_trig_name;
>>>> +
>>>> + return 0;
>>>> +
>>>> +error_free_trig_name:
>>>> + kfree(st->trig->name);
>>>> +error_free_trig:
>>>> + iio_free_trigger(st->trig);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +EXPORT_SYMBOL(iio_probe_common_trigger);
>>>> +
>>>> +void iio_remove_common_trigger(struct iio_dev *indio_dev)
>>>> +{
>>>> + struct iio_state *state = indio_dev->dev_data;
>>>> +
>>>> + iio_trigger_unregister(state->trig);
>>>> + kfree(state->trig->name);
>>>> + iio_free_trigger(state->trig);
>>>> +}
>>>> +EXPORT_SYMBOL(iio_remove_common_trigger);
>>>> Index: drivers/staging/iio/common-ring-trigger.h
>>>> ===================================================================
>>>> --- drivers/staging/iio/common-ring-trigger.h (revision 0)
>>>> +++ drivers/staging/iio/common-ring-trigger.h (revision 0)
>>>> @@ -0,0 +1,24 @@
>>>> +/* The industrial I/O frequently used ring with trigger
>>>> + *
>>>> + * Copyright (c) 2010 Barry Song
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify it
>>>> + * under the terms of the GNU General Public License version 2 as published by
>>>> + * the Free Software Foundation.
>>>> + *
>>>> + */
>>>> +
>>>> +#ifndef _IIO_COMMON_RING_TRIGGER_H_
>>>> +#define _IIO_COMMON_RING_TRIGGER_H_
>>>> +
>>>> +#include "iio.h"
>>>> +#include "ring_generic.h"
>>>> +
>>>> +int iio_configure_common_ring(struct iio_dev *indio_dev);
>>>> +void iio_unconfigure_common_ring(struct iio_dev *indio_dev);
>>>> +int iio_initialize_common_ring(struct iio_ring_buffer *ring);
>>>> +void iio_uninitialize_common_ring(struct iio_ring_buffer *ring);
>>>> +
>>>> +int iio_probe_common_trigger(struct iio_dev *indio_dev);
>>>> +void iio_remove_common_trigger(struct iio_dev *indio_dev);
>>>> +#endif /* _IIO_COMMON_RING_TRIGGER_H_ */
>>>> Index: drivers/staging/iio/Kconfig
>>>> ===================================================================
>>>> --- drivers/staging/iio/Kconfig (revision 8893)
>>>> +++ drivers/staging/iio/Kconfig (working copy)
>>>> @@ -38,6 +38,12 @@
>>>> ring buffers. The triggers are effectively a 'capture
>>>> data now' interrupt.
>>>>
>>>> +config IIO_COMMON_RING_TRIGGER
>>>> + boolean "Enable frequently used ring with trigger"
>>>> + depends on IIO_SW_RING && IIO_TRIGGER
>>>> + help
>>>> + Provides a generic ring with trigger. I can be used by most
>>>> + IIO devices.
>>>>
>>>> source "drivers/staging/iio/accel/Kconfig"
>>>> source "drivers/staging/iio/adc/Kconfig"
>>>> Index: drivers/staging/iio/Makefile
>>>> ===================================================================
>>>> --- drivers/staging/iio/Makefile (revision 8893)
>>>> +++ drivers/staging/iio/Makefile (working copy)
>>>> @@ -9,6 +9,8 @@
>>>>
>>>> obj-$(CONFIG_IIO_SW_RING) += ring_sw.o
>>>>
>>>> +obj-$(CONFIG_IIO_COMMON_RING_TRIGGER) += common-ring-trigger.o
>>>> +
>>>> obj-y += accel/
>>>> obj-y += adc/
>>>> obj-y += addac/
>>>>
>>>>
>>>>
>>>
>>>
>>>
>>
>
>
--
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
