On 06/28/10 08:59, Barry Song wrote:
> On Sat, Jun 26, 2010 at 1:18 AM, Jonathan Cameron <jic23@xxxxxxxxx> wrote:
>> Hi Barry,
>>
>> I've dropped back to the original patch for a close analysis of what is
>> going on. Sorry for the delay in getting to this!
>>
>>> 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.
>> This time I'm going to concentrate of specific issues.
>>
>> Note I'm reading the code as being directed at the vast majority
>> of cases. What you do here fairly radically changes the job of
>> the device driver. What I don't want to do is end up with
>> cases where a tiny difference in hardware results in two completely
>> different approaches being needed to write the driver.
>>
>> In a fairly illogical process I wrote this from the bottom :)
>>
>>>
>>> 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
>>> + * @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;
>>> + s64 last_timestamp;
>>> + struct iio_dev *indio_dev;
>>> + struct iio_trigger *trig;
>>
>> Not all drivers will want shared centralized buffers. Simple
>> devices might use the various spi and i2c functions that allocate
>> data storage as needed within the functions.
>>> + u8 *tx;
>>> + u8 *rx;
>>> + struct mutex buf_lock;
>>
>> Assumes single irq (lot of adis devices can have several) and makes
>> a copy of it to get round deliberate hiding of bus info.
>>> + int irq;
>>> + int (*set_irq)(struct iio_state *st, bool enable);
>>
>> My issues with this are given where it is used.
>>> + int (*hw_read_ring)(struct iio_state *st, u8 *rx);
>>
>> (as uncommented above) This is used to hide the bus specific info.
>>> + 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);
>>> +}
>>> +
>> We discussed various options for this, but I'm personally yet to be
>> convinced that we aren't making the driver code much harder to follow
>> for the sake of saving lines of code.
>>
>> I'm also not sure that the approach currently used here is the right
>> way to do it in any driver which doesn't help. One thing I would
>> like to do is to loose the additional data copy required by the current
>> approach of building a local record by copying form the raw data, then
>> copying this record into the buffer.
>>
>> Preallocating space is easy enough with kfifo so I will probably do
>> a rework of those patches shortly. For my sw_ring buffer I'm not sure
>> if it can be easily done, but I'll look into it.
> That main problem here is that the data layout is not coincident for
> kinds of chips. But the SW ring procedure is needed by all drivers. My
> idea is letting all the following done in bottom level driver:
>
> 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],
> ...
> *((s64 *)(data + ((i + 3)/4)*4)) = st->last_timestamp;
>
> Then the flow will be:
> 1. allocate buffer to hold data
Yes. Ideally that will change to be a call to a routine supplied by the
buffer implementation as part of a move towards multiple buffer implementations.
Note this one would need a hook to have the driver tell it how much space the
next step will be needing. This hook would also clean up the buffer allocation
code in preenable.
> 2. fill the buffer according to special data layout
> 3. store_to ring
Yup, this will become a paired operation with step 1. (more of a 'done
with buffer so it can no be read from the ring, or pushed onto the ring
depending on implementation'.)
> 4. notify the event
>
> The procedure is common for all chips using SW ring, only step 2 need
> be instantized. Then adding hook named get_ring_elemants(), which read
> HW and organize data layout, is ok in fact.
Agreed. This way the driver still maintains control of what is going on
and is still easy to read, but some of the complexity (error checking etc)
is moved to a standard function. My next question is where to put this function.
Perhaps within the sw ring implementation?
>
>>
>>> +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;
>>> +}
>>> +
>>
>> This one is pretty specific, though only in the central
>> section working out the size. Can we come up with an easy way to
>> share the rest of the code?
>>
>> The previous discussion including some possible solutions.
>> I'm yet to be convinced that there is a clean way to do the equivalent
>> of this given the large amount of knowledge your shared code would have
>> to have about the underlying data.
>
> Here the main problem is sizeof(16) is thinked suitable for all chips.
True for a lot of chips, but not all. I'd rather not have that restriction
in there.
> Could we add a size_per_element field, then we calucate the total bpd
> according to the size_per_element and scan_count?
That would work, but is going to be non trivial as we would also want to
generate the userspace interface (attributes currently handled by drivers)
from these values.
As I said above, I'd be more inclined (for now anyway) to let the driver provide
a 'get_bpd' call back. It would be handy both here, and in the buffer
fill code.
With either approach, we then have a nice general form of this function that
can be used with all sw buffers, so perhaps this belongs in the ring_sw
module as well?
>
>>> +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;
>>> +}
>>> +
>>
>> Agree entirely with the next two. Have moved to industrialio-trigger
>> in todays first patch set. They apply in all current cases and
>> for that matter in any I have yet thought up.
>
> Acked.
>
>>> +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);
>>> +
>> One reason I'm not keen on this as is, is that it means that all
>> drivers using this code have to use the same buffer implementation.
>> I'm very keen to make this selectable on a per driver basis.
>
> Why can't all devices by SW rings use the same buffer implementation?
> Why do they want a different SW ring?
Although we aren't using it much at the moment, the ability to do this looks
to be crucial for the future of IIO. A non exhaustive list of why includes:
1) ring_sw is terrible. I wrote it late one night. No one has ever taken
on reviewing it and I for one am far from convinced it is correct. It was only
meant to be a proof of concept (though we have been using it for a while in
'production' data capture systems).
2) ring_sw is unsuprisingly a ring buffer. In many applications fifo behaviour
is preferable.
3) ring_sw has maximum size of one page. Not good for devices at high rates.
4) lots of interesting work is going on in the area of lockless ring buffers in
the kernel at the moment. Whilst then tend to be complex, they have much better
scalablity than mine.
5) the buffer hooks are probably the best way to interface to input which people
keep requesting.
6) in slow applications a simple locked buffer would be more appropriate unless we
are sure there are no downsides to the lockless case.
So in the medium term (or short if someone else does it ;) we should have
* Something equivalent to ring_sw with all of the hooks for events to userspace etc.
* kfifo based fifo. It's simple, but may require excessive locking. (also with this
I'm interested in whether I can mess around within spi / i2c subsystems to get
some controllers to do scatter gather dma directly into the buffer)
* input hooked buffer.
* high performance large ring buffer. (I'm waiting to see what comes out of the tracing
discussions to see if it is relevant to us).
>
>> Obviously we could add hooks to make that work, but then we begin
>> to destroy the point of your patch.
>>
>>> + ring = iio_sw_rb_allocate(indio_dev);
>>> + if (!ring) {
>>> + ret = -ENOMEM;
>>> + return ret;
>>> + }
>>> + indio_dev->ring = ring;
>>> +
>>> + iio_ring_sw_register_funcs(&ring->access);
>>
>> As discussed above. This one is actually a bit of a pain.
>>> + ring->preenable = &iio_common_ring_preenable;
>> These last two are the same for all triggered software ring buffers,
>> so I've moved the code into industrialio-trigger.c for now.
>>> + ring->postenable = &iio_common_ring_postenable;
>>> + ring->predisable = &iio_common_ring_predisable;
>>> + ring->owner = THIS_MODULE;
>>> +
>>
>> This makes it clear that it is sensible to have an alloc_pollfunc function
>> as the next 8 or so lines occur every single time. That was in my first
>> patch set today.
>
> Acked.
>
>>> + 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);
>>> +
>> Small save, but worth having if we can sort the configure function so
>> everyone is happy.
>>> +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);
>>> +
>>
>> Only purpose of these wrapping functions is to allow for clean
>> Kconfig selection of whether ring is enable or not. I have just
>> sent out a patch that adds relevant stubs to the core allowing these
>> to go from all drivers anyway. Glad to get those out of the way!
>> Thanks for making the waste apparent.
>
> Acked.
>
>>> +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);
>> You already know from previous emails that there is an issue with
>> this one given in creates a couple of structures. Would have
>> to be a separate instance per driver.
>
> Yes. Here i will give a seperate instance per driver.
>
>>> +
>>> +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,
>>> +};
>>> +
>>
>> Hiding away the bus type requires a copy of the irq already available.
>>> +static int iio_common_trigger_try_reen(struct iio_trigger *trig)
>>> +{
>>> + struct iio_state *st = trig->private_data;
>>> + enable_irq(st->irq);
>>> + return 0;
>>> +}
>>> +
>>
>> This is far too specific for my liking. It only even applies to
>> the majority of your drivers due to their current simple nature.
>> If you are going to do this unification I'd like to see it post
>> addition of full hardware event handling (I'm intending to do
>> this at somepoint but am low on time currently and only have an
>> adis16350 to play with)
>>
>> Assumptions:
>> 1) Interrupt line is used for dataready and other events.
>> If this isn't true then the driver should be using the event
>> list system at all. Adds overhead to this path for no gain.
>> With some of the adis sensors you have 2 interrupt lines.
>> A sensible option would be to setup one line for dataready
>> and the other for all other events.
>> 2) Assumes that we have data ready on the first interrupt line.
>> Perhaps this makes sense but it isn't a current requirement
>> or even a suggested option.
>
> Here we can build the matched pair for interrupt index and trigger
> name, then we can check the interrupt index by trigger name with the
> pair.
The complexity is not that we might have multiple triggers, its that
the physical lines may be doing other things as well such as threshold
interrupts etc. Can we hold this chunk of the patch until we have a few
more devices implementing events and see where we are at that stage?
It certainly looks like there are some savings in code to be made
here, but I'm not yet of the best way to do it!
> If one device has more than one trigger and one interrupt, we transfer
> the param of interrupt index to set_irq(), then the process flow can
> be shared too.
True.
> Anyway, bottom drivers don't want to implement the trigger_set_state
> per trigger if the flow is same.
I don't follow what you mean here.
>
>>> +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);
>>> + 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;
>>> +}
>>> +
>>
>> The rest of your factoring out obviously requires the shared driver
>> state structure. This all makes sense if we think that is a desirable
>> thing to do.
>>> +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);
>>
>> snipped rest as it is obvious boiler pate.
>>
>> As an aside. There is an awful lot of repeated code relating to low level
>> device access in the adis drivers. Perhaps that is a good opportunity for
>> unifying code? Maybe we want and adis_library module? Or perhaps a number
>> of the devices can be combined into a single driver? Just a thought
>> that came up whilst looking at that code again.
>
> Good idea in fact. Many codes are same, for example spi read. But it
> is really difficult to find the orderliness to build a library for the
> moment.
One for a later round of clean ups! Perhaps I'll take a look at how
easy this would be to do.
Jonathan
--
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
