On 02/08/2012 05:28, srinivas pandruvada wrote:
Added usage id processing for Accelerometer 3D.This uses IIO
interfaces for triggerred buffer to present data to user
mode.This uses HID sensor framework for registering callback
events from the sensor hub.
Mostly good. Couple of bits and bobs inline. The only bigish
one is to take the chan_spec array constant and do your changes
on the copy rather than changing the array then making a copy...
Also I think there is one place that prevents multiple instances
of the driver.
Sorry it took me so long to get to this!
Signed-off-by: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxx>
---
drivers/iio/Kconfig | 1 +
drivers/iio/Makefile | 1 +
drivers/iio/accel/Kconfig | 16 ++
drivers/iio/accel/Makefile | 6 +
drivers/iio/accel/hid-sensor-accel-3d.c | 420 +++++++++++++++++++++++++++++++
5 files changed, 444 insertions(+), 0 deletions(-)
create mode 100644 drivers/iio/accel/Kconfig
create mode 100644 drivers/iio/accel/Makefile
create mode 100644 drivers/iio/accel/hid-sensor-accel-3d.c
diff --git a/drivers/iio/Kconfig b/drivers/iio/Kconfig
index 61646c7..eb78b0f 100644
--- a/drivers/iio/Kconfig
+++ b/drivers/iio/Kconfig
@@ -54,6 +54,7 @@ config IIO_CONSUMERS_PER_TRIGGER
This value controls the maximum number of consumers that a
given trigger may handle. Default is 2.
+source "drivers/iio/accel/Kconfig"
source "drivers/iio/adc/Kconfig"
source "drivers/iio/amplifiers/Kconfig"
source "drivers/iio/common/Kconfig"
diff --git a/drivers/iio/Makefile b/drivers/iio/Makefile
index db0a043..aed602a 100644
--- a/drivers/iio/Makefile
+++ b/drivers/iio/Makefile
@@ -10,6 +10,7 @@ industrialio-$(CONFIG_IIO_TRIGGER) += industrialio-trigger.o
obj-$(CONFIG_IIO_TRIGGERED_BUFFER) += industrialio-triggered-buffer.o
obj-$(CONFIG_IIO_KFIFO_BUF) += kfifo_buf.o
+obj-y += accel/
obj-y += adc/
obj-y += amplifiers/
obj-y += common/
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
new file mode 100644
index 0000000..b2510c4
--- /dev/null
+++ b/drivers/iio/accel/Kconfig
@@ -0,0 +1,16 @@
+#
+# Accelerometer drivers
+#
+menu "Accelerometers"
+
+config HID_SENSOR_ACCEL_3D
+ depends on HID_SENSOR_HUB
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ select HID_SENSOR_IIO_COMMON
+ tristate "HID Acelerometers 3D"
+ help
+ Say yes here to build support for the HID SENSOR
+ accelerometers 3D.
+
+endmenu
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
new file mode 100644
index 0000000..657d671
--- /dev/null
+++ b/drivers/iio/accel/Makefile
@@ -0,0 +1,6 @@
+#
+# Makefile for industrial I/O accelerometer drivers
+#
+
+hid-sensor-accel-3d-drv-y := hid-sensor-accel-3d.o
+obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-accel-3d-drv.o
diff --git a/drivers/iio/accel/hid-sensor-accel-3d.c b/drivers/iio/accel/hid-sensor-accel-3d.c
new file mode 100644
index 0000000..8f7bf0d
--- /dev/null
+++ b/drivers/iio/accel/hid-sensor-accel-3d.c
@@ -0,0 +1,420 @@
+/*
+ * HID Sensors Driver
+ * Copyright (c) 2012, Intel Corporation.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/slab.h>
+#include <linux/hid-sensor-hub.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+#include "../common/hid-sensors/hid-sensor-attributes.h"
+#include "../common/hid-sensors/hid-sensor-trigger.h"
+
+/*Format: HID-SENSOR-usage_id_in_hex*/
+#define DRIVER_NAME "HID-SENSOR-200073"
I can see why you do this, but it is rather opaque to anyone without
the spec.. Maybe HID-SENSOR-200073-accel or similar?
+
+enum accel_3d_channel{
+ CHANNEL_SCAN_INDEX_X,
+ CHANNEL_SCAN_INDEX_Y,
+ CHANNEL_SCAN_INDEX_Z,
+ ACCEL_3D_CHANNEL_MAX,
+};
+
+struct accel_3d_state {
+ struct hid_sensor_iio_common common_attributes;
+ struct hid_sensor_hub_attribute_info accel[ACCEL_3D_CHANNEL_MAX];
+ u32 accel_val[ACCEL_3D_CHANNEL_MAX];
+};
+
+const u32 accel_3d_addresses[ACCEL_3D_CHANNEL_MAX] = {
+ HID_USAGE_SENSOR_ACCEL_X_AXIS,
+ HID_USAGE_SENSOR_ACCEL_Y_AXIS,
+ HID_USAGE_SENSOR_ACCEL_Z_AXIS
+};
+
+/* Channel definitions */
make this const and adjust the copy not this.
+static struct iio_chan_spec accel_3d_channels[] = {
+ {
+ .type = IIO_ACCEL,
+ .modified = 1,
+ .channel2 = IIO_MOD_X,
+ .info_mask = IIO_CHAN_INFO_OFFSET_SHARED_BIT |
+ IIO_CHAN_INFO_SCALE_SHARED_BIT |
+ IIO_CHAN_INFO_SAMP_FREQ_SHARED_BIT |
+ IIO_CHAN_INFO_HYSTERESIS_SHARED_BIT,
+ .scan_index = CHANNEL_SCAN_INDEX_X,
+ }, {
+ .type = IIO_ACCEL,
+ .modified = 1,
+ .channel2 = IIO_MOD_Y,
+ .info_mask = IIO_CHAN_INFO_OFFSET_SHARED_BIT |
+ IIO_CHAN_INFO_SCALE_SHARED_BIT |
+ IIO_CHAN_INFO_SAMP_FREQ_SHARED_BIT |
+ IIO_CHAN_INFO_HYSTERESIS_SHARED_BIT,
+ .scan_index = CHANNEL_SCAN_INDEX_Y,
+ }, {
+ .type = IIO_ACCEL,
+ .modified = 1,
+ .channel2 = IIO_MOD_Z,
+ .info_mask = IIO_CHAN_INFO_OFFSET_SHARED_BIT |
+ IIO_CHAN_INFO_SCALE_SHARED_BIT |
+ IIO_CHAN_INFO_SAMP_FREQ_SHARED_BIT |
+ IIO_CHAN_INFO_HYSTERESIS_SHARED_BIT,
+ .scan_index = CHANNEL_SCAN_INDEX_Z,
+ }
+};
+
+/* Adjust channel real bits based on report descriptor */
+static void accel_3d_adjust_channel_bit_mask(int channel, int size)
+{
+ accel_3d_channels[channel].scan_type.sign = 's';
+ /* Real storage bits will change based on the report desc. */
+ accel_3d_channels[channel].scan_type.realbits = size * 8;
+ /* Maximum size of a sample to capture is u32 */
+ accel_3d_channels[channel].scan_type.storagebits = sizeof(u32) * 8;
As stated below (note I review in reverse as drivers make more sense
that way around typically ;) this really wants to operate on a local
copy rather than the one above (which can then be made static const).
+}
+
+/* Channel read_raw handler */
+static int accel_3d_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2,
+ long mask)
+{
+ struct accel_3d_state *accel_state = iio_priv(indio_dev);
+ int report_id = -1;
+ u32 address;
+ int ret;
+
+ *val = 0;
+ *val2 = 0;
+ switch (mask) {
+ case 0:
+ report_id =
+ accel_state->accel[chan->scan_index].report_id;
+ address = accel_3d_addresses[chan->scan_index];
+ if (report_id >= 0)
+ *val = sensor_hub_input_attr_get_raw_value(
+ accel_state->common_attributes.hsdev,
+ HID_USAGE_SENSOR_ACCEL_3D, address,
+ report_id);
+ else
+ *val = 0;
Return an error perhaps? -EBUSY maybe though I'm not sure I understand
when this
might happen.
+ break;
+ case IIO_CHAN_INFO_SCALE:
+ *val = accel_state->accel[CHANNEL_SCAN_INDEX_X].units;
+ break;
+ case IIO_CHAN_INFO_OFFSET:
+ *val = hid_sensor_convert_exponent(
+ accel_state->accel[CHANNEL_SCAN_INDEX_X].unit_expo);
+ break;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = hid_sensor_read_samp_freq_value(
+ &accel_state->common_attributes, val, val2);
+ break;
+ case IIO_CHAN_INFO_HYSTERESIS:
+ ret = hid_sensor_read_raw_hyst_value(
+ &accel_state->common_attributes, val, val2);
+ break;
+ default:
+ break;
+ }
+
+ if (*val2)
+ return IIO_VAL_INT_PLUS_MICRO;
+ else
+ return IIO_VAL_INT;
Hmm. This is a bit clunky and personally I'd expect the units to remain
consistent
whereas if *val2 happens to be zero in one of the ones where it need not
be it will
flip to only reporting IIO_VAL_INT and not showing the .000000 as one
might expect.
Hence I'd set this explicitily in each case.
+}
+
+/* Channel write_raw handler */
+static int accel_3d_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val,
+ int val2,
+ long mask)
+{
+ struct accel_3d_state *accel_state = iio_priv(indio_dev);
+ int ret = 0;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = hid_sensor_write_samp_freq_value(
+ &accel_state->common_attributes, val, val2);
+ break;
+ case IIO_CHAN_INFO_HYSTERESIS:
+ ret = hid_sensor_write_raw_hyst_value(
+ &accel_state->common_attributes, val, val2);
+ break;
+ default:
+ ret = -EINVAL;
+ }
+
+ return ret;
+}
+
+static int accel_3d_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ long mask)
+{
+ return IIO_VAL_INT_PLUS_MICRO;
+}
This is the default, so not necessary unless it is for consistency against
other drivers...
+
+static const struct iio_info accel_3d_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = &accel_3d_read_raw,
+ .write_raw = &accel_3d_write_raw,
+ .write_raw_get_fmt = &accel_3d_write_raw_get_fmt,
+};
+
+/* Function to push data to buffer */
+void hid_sensor_push_data(struct iio_dev *indio_dev, u8 *data, int len)
+{
+ struct iio_buffer *buffer = indio_dev->buffer;
+ s64 timestamp = iio_get_time_ns();
+ int datum_sz;
+
+ dev_dbg(&indio_dev->dev, "hid_sensor_push_data\n");
+ if (!buffer) {
+ dev_err(&indio_dev->dev, "Buffer == NULL\n");
+ return;
+ }
+ datum_sz = buffer->access->get_bytes_per_datum(buffer);
+ if (len > datum_sz) {
+ dev_err(&indio_dev->dev, "Datum size mismatch %d:%d\n", len,
+ datum_sz);
+ return;
+ }
+ buffer->access->store_to(buffer, (u8 *)data, timestamp);
+}
+
+/* Callback handler to send event after all samples are received and captured */
+int accel_3d_proc_event(struct hid_sensor_hub_device *hsdev, unsigned usage_id,
+ void *priv)
+{
+ struct iio_dev *indio_dev = platform_get_drvdata(priv);
+ struct accel_3d_state *accel_state = iio_priv(indio_dev);
+
+ dev_dbg(&indio_dev->dev, "accel_3d_proc_event [%d]\n",
+ accel_state->common_attributes.data_ready);
+ if (accel_state->common_attributes.data_ready)
+ hid_sensor_push_data(indio_dev,
+ (u8 *)accel_state->accel_val,
+ sizeof(accel_state->accel_val));
+
+ return 0;
+}
+
+/* Capture samples in local storage */
+int accel_3d_capture_sample(struct hid_sensor_hub_device *hsdev,
+ unsigned usage_id,
+ size_t raw_len, char *raw_data,
+ void *priv)
+{
+ struct iio_dev *indio_dev = platform_get_drvdata(priv);
+ struct accel_3d_state *accel_state = iio_priv(indio_dev);
+ int offset;
+ int ret = -EINVAL;
+
+ switch (usage_id) {
+ case HID_USAGE_SENSOR_ACCEL_X_AXIS:
+ case HID_USAGE_SENSOR_ACCEL_Y_AXIS:
+ case HID_USAGE_SENSOR_ACCEL_Z_AXIS:
+ offset = usage_id - HID_USAGE_SENSOR_ACCEL_X_AXIS;
+ accel_state->accel_val[CHANNEL_SCAN_INDEX_X + offset] =
+ *(u32 *)raw_data;
+ ret = 0;
+ break;
+ default:
+ break;
+ }
+
+ return ret;
+}
+
+/* Parse report which is specific to an usage id*/
+static int accel_3d_parse_report(struct platform_device *pdev,
+ struct hid_sensor_hub_device *hsdev,
+ unsigned usage_id,
+ struct accel_3d_state *st)
+{
+ int ret;
+ int i;
+
Easier to follow if you start with i = CHANNEL_SCAN_INDEX_X and then
just use
i where you currently have CHANNEL_SCAN_INDEX_X + i (or 0 + i!)
+ for (i = 0; i <= CHANNEL_SCAN_INDEX_Z; ++i) {
+ ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT,
+ usage_id,
+ HID_USAGE_SENSOR_ACCEL_X_AXIS + i,
+ &st->accel[CHANNEL_SCAN_INDEX_X + i]);
+ accel_3d_adjust_channel_bit_mask(CHANNEL_SCAN_INDEX_X + i,
+ st->accel[CHANNEL_SCAN_INDEX_X + i].size);
+ if (ret < 0)
+ break;
+ }
+ dev_dbg(&pdev->dev, "accel_3d %x:%x, %x:%x, %x:%x\n",
+ st->accel[0].index,
+ st->accel[0].report_id,
+ st->accel[1].index, st->accel[1].report_id,
+ st->accel[2].index, st->accel[2].report_id);
+
+ return ret;
+}
+
+/* Function to initialize the processing for usage id */
+static inline int accel_3d_init(struct platform_device *pdev,
+ struct hid_sensor_hub_device *hsdev,
+ unsigned usage_id)
+{
+ int ret = 0;
should this not be static const?
+ static char *name = "accel_3d";
+ struct iio_dev *indio_dev;
+ struct accel_3d_state *accel_state;
+
+ indio_dev = iio_device_alloc(sizeof(struct accel_3d_state));
+ if (indio_dev == NULL) {
+ ret = -ENOMEM;
+ goto error_ret;
+ }
+ platform_set_drvdata(pdev, indio_dev);
+
+ accel_state = iio_priv(indio_dev);
+ accel_state->common_attributes.hsdev = hsdev;
+ accel_state->common_attributes.pdev = pdev;
+
+ ret = hid_sensor_parse_common_attributes(hsdev, usage_id, &accel_state->common_attributes);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to setup common attributes\n");
+ goto error_free_dev;
+ }
+
+ ret = accel_3d_parse_report(pdev, hsdev, usage_id, accel_state);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to setup attributes\n");
+ goto error_free_dev;
+ }
+
This copying strategy would be more 'logical' if you copied the channels
first,
then ammended them appropriately rather than messing with the original
copy then copying it. Also avoids any future issues with parallel
loading of
multiple instances of this driver.
+ indio_dev->channels = kmemdup(accel_3d_channels,
+ sizeof(accel_3d_channels),
+ GFP_KERNEL);
+ if (!indio_dev->channels) {
+ dev_err(&pdev->dev, "failed to duplicate channels\n");
+ goto error_free_dev;
+ }
+
+ indio_dev->num_channels =
+ ARRAY_SIZE(accel_3d_channels);
This might just be my email client (which is notorious for breaking
patch formatting ;) but the above should fit on one line.
+ indio_dev->dev.parent = &pdev->dev;
+ indio_dev->info = &accel_3d_info;
+ indio_dev->name = name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = iio_triggered_buffer_setup(indio_dev, &iio_pollfunc_store_time,
+ NULL, NULL);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to initialize trigger buffer\n");
+ goto error_free_dev_mem;
+ }
+ accel_state->common_attributes.data_ready = false;
+ ret = hid_sensor_setup_trigger(indio_dev, name,
+ &accel_state->common_attributes);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "trigger setup failed\n");
+ goto error_unreg_buffer_funcs;
+ }
+
+ ret = iio_device_register(indio_dev);
+ if (ret) {
+ dev_err(&pdev->dev, "device register failed\n");
+ goto error_remove_trigger;
+ }
+
+ return ret;
+
+error_remove_trigger:
+ hid_sensor_remove_trigger(indio_dev);
+error_unreg_buffer_funcs:
+ iio_triggered_buffer_cleanup(indio_dev);
+error_free_dev_mem:
+ kfree(indio_dev->channels);
+error_free_dev:
+ iio_device_free(indio_dev);
+error_ret:
+ return ret;
+}
+
+/* Function to deinitialize the processing for usage id */
+static inline void accel_3d_exit(struct platform_device *pdev)
+{
+ struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+
+ iio_device_unregister(indio_dev);
+ hid_sensor_remove_trigger(indio_dev);
+ iio_triggered_buffer_cleanup(indio_dev);
+ kfree(indio_dev->channels);
+ iio_device_free(indio_dev);
no need for a blank line here.
+
+}
+
+static struct hid_sensor_hub_callbacks accel_3d_callbacks = {
+ .send_event = accel_3d_proc_event,
+ .capture_sample = accel_3d_capture_sample,
+};
+
+static int __devinit hid_accel_3d_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
+
+ ret = accel_3d_init(pdev, hsdev, HID_USAGE_SENSOR_ACCEL_3D);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "accel_3d_init failed\n");
+ return ret;
+ }
+ accel_3d_callbacks.pdev = pdev;
Does this little bit here not prevent us having multiple sensor hubs?
Just allocate that callback structure locally then assign the two constant
elements.
+ ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_ACCEL_3D,
+ &accel_3d_callbacks);
+
+ return ret;
+}
+
+static int __devinit hid_accel_3d_remove(struct platform_device *pdev)
+{
+ struct hid_sensor_hub_device *hsdev = pdev->dev.platform_data;
+
+ accel_3d_exit(pdev);
+
+ return sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_ACCEL_3D);
+}
+
+static struct platform_driver hid_accel_3d_platform_driver = {
+ .driver = {
+ .name = DRIVER_NAME,
+ .owner = THIS_MODULE,
+ },
+ .probe = hid_accel_3d_probe,
+ .remove = hid_accel_3d_remove,
+};
+module_platform_driver(hid_accel_3d_platform_driver);
+
+MODULE_DESCRIPTION("HID Sensor Accel 3D");
+MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxx>");
+MODULE_LICENSE("GPL");
--
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
