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?
>
> 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().
>> +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?
>> +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?
>> + 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
