On 07/04/2012 08:52 PM, srinivas pandruvada wrote:
> This patch contains the common code, which is used by all HID sensors.
> There are some common set of attributes, which every hid sensor
> needs it. This patch contains all such attributes processing.
> Also the trigger interface is common among all HID sensors. This
> patch contains common trigger functions utilized by all HID sensors.
>
This patch is rather less well cleaned up that the previous.
Main issue is with the data format for hysterisis. Not a chance
what you have here will go in. Please format it to a simple
xxxx.yyyy form.
> Signed-off-by: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxx>
> ---
> drivers/staging/iio/Kconfig | 1 +
> drivers/staging/iio/Makefile | 1 +
> drivers/staging/iio/common/Kconfig | 6 +
> drivers/staging/iio/common/Makefile | 5 +
> drivers/staging/iio/common/hid-sensors/Kconfig | 21 +++
> drivers/staging/iio/common/hid-sensors/Makefile | 6 +
> .../iio/common/hid-sensors/hid-sensor-attributes.c | 182 ++++++++++++++++++++
> .../iio/common/hid-sensors/hid-sensor-attributes.h | 60 +++++++
> .../iio/common/hid-sensors/hid-sensor-trigger.c | 104 +++++++++++
> .../iio/common/hid-sensors/hid-sensor-trigger.h | 27 +++
> 10 files changed, 413 insertions(+), 0 deletions(-)
> create mode 100644 drivers/staging/iio/common/Kconfig
> create mode 100644 drivers/staging/iio/common/Makefile
> create mode 100644 drivers/staging/iio/common/hid-sensors/Kconfig
> create mode 100644 drivers/staging/iio/common/hid-sensors/Makefile
> create mode 100644 drivers/staging/iio/common/hid-sensors/hid-sensor-attributes.c
> create mode 100644 drivers/staging/iio/common/hid-sensors/hid-sensor-attributes.h
> create mode 100644 drivers/staging/iio/common/hid-sensors/hid-sensor-trigger.c
> create mode 100644 drivers/staging/iio/common/hid-sensors/hid-sensor-trigger.h
>
> diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig
> index 04cd6ec..7b058f4 100644
> --- a/drivers/staging/iio/Kconfig
> +++ b/drivers/staging/iio/Kconfig
> @@ -29,6 +29,7 @@ source "drivers/staging/iio/accel/Kconfig"
> source "drivers/staging/iio/adc/Kconfig"
> source "drivers/staging/iio/addac/Kconfig"
> source "drivers/staging/iio/cdc/Kconfig"
> +source "drivers/staging/iio/common/Kconfig"
> source "drivers/staging/iio/frequency/Kconfig"
> source "drivers/staging/iio/gyro/Kconfig"
> source "drivers/staging/iio/impedance-analyzer/Kconfig"
> diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
> index fa6937d..c7ca152 100644
> --- a/drivers/staging/iio/Makefile
> +++ b/drivers/staging/iio/Makefile
> @@ -17,6 +17,7 @@ obj-y += accel/
> obj-y += adc/
> obj-y += addac/
> obj-y += cdc/
> +obj-y += common/
> obj-y += frequency/
> obj-y += gyro/
> obj-y += impedance-analyzer/
> diff --git a/drivers/staging/iio/common/Kconfig b/drivers/staging/iio/common/Kconfig
> new file mode 100644
> index 0000000..bed569a
> --- /dev/null
> +++ b/drivers/staging/iio/common/Kconfig
> @@ -0,0 +1,6 @@
> +#
> +# IIO common modules
> +#
> +
> +source "drivers/staging/iio/common/hid-sensors/Kconfig"
> +
> diff --git a/drivers/staging/iio/common/Makefile b/drivers/staging/iio/common/Makefile
> new file mode 100644
> index 0000000..23852b6
> --- /dev/null
> +++ b/drivers/staging/iio/common/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the IIO common modules.
I'd be a bit more descriptive bit.
support modules required by various different device types? Something
like that.
> +#
> +
> +obj-y += hid-sensors/
> diff --git a/drivers/staging/iio/common/hid-sensors/Kconfig b/drivers/staging/iio/common/hid-sensors/Kconfig
> new file mode 100644
> index 0000000..da64fef
> --- /dev/null
> +++ b/drivers/staging/iio/common/hid-sensors/Kconfig
> @@ -0,0 +1,21 @@
> +#
> +# Hid Sensor common modules
> +#
> +menu "Hid Sensor IIO Common"
> +
> +config HID_SENSOR_IIO_COMMON
> + tristate "Common modules for all HID Sensor IIO drivers"
> + depends on HID_SENSOR_HUB
> + select IIO_TRIGGER if IIO_BUFFER
> + help
> + Say yes here to build support for HID sensor to use
> + HID sensor common processing for attributes and IIO triggers.
> +
> +config HID_SENSOR_ENUM_BASE_QUIRKS
> + tristate "ENUM base quirks for HID Sensor IIO drivers"
> + depends on HID_SENSOR_IIO_COMMON
> + help
> + Say yes here to build support for sensor hub FW using
> + enumeration, which is using 1 as base instead of 0.
No way of detecting that and loosing the config option?
> +
> +endmenu
> diff --git a/drivers/staging/iio/common/hid-sensors/Makefile b/drivers/staging/iio/common/hid-sensors/Makefile
> new file mode 100644
> index 0000000..1f463e0
> --- /dev/null
> +++ b/drivers/staging/iio/common/hid-sensors/Makefile
> @@ -0,0 +1,6 @@
> +#
> +# Makefile for the Hid sensor common modules.
> +#
> +
> +obj-$(CONFIG_HID_SENSOR_IIO_COMMON) += hid-sensor-iio-common.o
> +hid-sensor-iio-common-y := hid-sensor-attributes.o hid-sensor-trigger.o
> diff --git a/drivers/staging/iio/common/hid-sensors/hid-sensor-attributes.c b/drivers/staging/iio/common/hid-sensors/hid-sensor-attributes.c
> new file mode 100644
> index 0000000..d39c93e
> --- /dev/null
> +++ b/drivers/staging/iio/common/hid-sensors/hid-sensor-attributes.c
> @@ -0,0 +1,182 @@
> +/*
> + * 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 "hid-sensor-attributes.h"
> +
why? Part of spec?
> +#define MAXIMUM_SAMP_FREQUENCY 1000
> +
> +ssize_t hid_sensor_read_samp_freq(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct hid_sensor_attributes *st = iio_priv(indio_dev);
> + s32 value;
> + int len;
> + int ret;
> + int conv_value;
> +
> + ret = sensor_hub_get_feature(st->hsdev,
> + st->poll.report_id,
> + st->poll.index, &value);
> + if (ret < 0 || value <= 0)
> + len = sprintf(buf, "0\n");
> + else {
> + if (st->poll.units == HID_USAGE_SENSOR_UNITS_MILLISECOND)
> + conv_value = 1000/value;
Not going to be terribly accurate and we do allow decimal places
in the abi.
> + else if (st->poll.units == HID_USAGE_SENSOR_UNITS_SECOND)
> + conv_value = 1/value;
Given we are integer, what do you think that is likely to give?
> + else
> + conv_value = value; /*Assume HZ*/
> + len = sprintf(buf, "%d\n", conv_value);
> + }
> + return len;
> +}
> +EXPORT_SYMBOL(hid_sensor_read_samp_freq);
> +
> +ssize_t hid_sensor_write_samp_freq(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct hid_sensor_attributes *st = iio_priv(indio_dev);
> + int value;
> + int conv_value;
> + int ret;
> +
> + if (kstrtoint(buf, 10, &value) < 0)
> + return -EINVAL;
> +
> + if (value > MAXIMUM_SAMP_FREQUENCY)
> + value = MAXIMUM_SAMP_FREQUENCY;
> +
> + if (value && st->poll.units == HID_USAGE_SENSOR_UNITS_MILLISECOND)
> + conv_value = 1000/value;
> + else if (value && st->poll.units == HID_USAGE_SENSOR_UNITS_SECOND)
> + conv_value = 1/value;
> + else
> + conv_value = value; /*Assume HZ*/
> +
> + ret = sensor_hub_set_feature(st->hsdev,
> + st->poll.report_id,
> + st->poll.index,
> + conv_value);
> +
> + if (ret < 0)
> + return ret;
> + return strlen(buf);
> +}
> +EXPORT_SYMBOL(hid_sensor_write_samp_freq);
> +
Hysterisis is common enough we should just add it to
the iio_chan_spec->info_mask and do it through the read_raw
callbacks. Would you mind doing a patch to do that?
> +ssize_t hid_sensor_read_hyst_raw(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> +
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct hid_sensor_attributes *st = iio_priv(indio_dev);
> + s32 value;
> + int len;
> + int ret;
> +
> + ret = sensor_hub_get_feature(st->hsdev,
> + st->sensitivity.report_id,
> + st->sensitivity.index, &value);
> + if (ret < 0 || value < 0)
> + len = sprintf(buf, "0\n");
> + else
Not happy with this interface at all.
Scale out the units please to get to the relevant base unit.
e.g. accel is m/s^2 The exponents here are simple powers
of 10 I think? Hence trivial to apply. This should
be a simple decimal number. There may be some arguement for
extending the buffered interface to handle the exponents, but
here we have a massive text buffer and it's a slow path
so lets just do it the simple way.
> + len = sprintf(buf, "units:%d,exp:%d,value:%d\n",
> + st->sensitivity.units,
> + st->sensitivity.unit_expo, value);
> + return len;
> +}
> +EXPORT_SYMBOL(hid_sensor_read_hyst_raw);
> +
> +ssize_t hid_sensor_write_hyst_raw(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct hid_sensor_attributes *st = iio_priv(indio_dev);
> + int value;
> + int ret;
> +
> + if (kstrtoint(buf, 10, &value) < 0)
> + return -EINVAL;
> +
> + if (value <= 0)
> + value = 0;
> + ret = sensor_hub_set_feature(st->hsdev,
> + st->sensitivity.report_id,
> + st->sensitivity.index,
> + value);
This looks suspiciously like a magic number is needed.
Please do the relevant conversions to go from a floating point
string input to the right representation.
> +
> + if (ret < 0)
> + return ret;
> +
> + return strlen(buf);
not return len;?
> +}
> +EXPORT_SYMBOL(hid_sensor_write_hyst_raw);
> +
> +int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
> + u32 usage_id,
> + struct hid_sensor_attributes *st)
> +{
> + int ret;
> +
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> + HID_FEATURE_REPORT, usage_id,
> + HID_SENSOR_POLLING, &st->poll);
> +
Do something with ret? Otherwise why have it?
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> + HID_FEATURE_REPORT, usage_id,
> + HID_SENSOR_REPORT_STATE,
> + &st->report_state);
> +
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> + HID_FEATURE_REPORT, usage_id,
> + HID_SENSOR_POWER_STATE,
> + &st->power_state);
> +
> + ret = sensor_hub_input_get_attribute_info(hsdev,
> + HID_FEATURE_REPORT, usage_id,
> + HID_USAGE_SENSOR_PROPERTY_CHANGE_SENSITIVITY_ABS,
> + &st->sensitivity);
> +
> + hid_dbg(hsdev->hdev, "common attributes: %x:%x, %x:%x, %x:%x %x:%x\n",
> + st->poll.index, st->poll.report_id,
> + st->report_state.index, st->report_state.report_id,
> + st->power_state.index, st->power_state.report_id,
> + st->sensitivity.index, st->sensitivity.report_id);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(hid_sensor_parse_common_attributes);
> +
> +MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxx>");
> +MODULE_DESCRIPTION("HID Sensor common attribute processing");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/staging/iio/common/hid-sensors/hid-sensor-attributes.h b/drivers/staging/iio/common/hid-sensors/hid-sensor-attributes.h
> new file mode 100644
> index 0000000..c859b7e
> --- /dev/null
> +++ b/drivers/staging/iio/common/hid-sensors/hid-sensor-attributes.h
> @@ -0,0 +1,60 @@
> +/*
> + * 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.
> + *
> + */
> +#ifndef _HID_SENSORS_ATTRIBUTES_H
> +#define _HID_SENSORS_ATTRIBUTES_H
> +
> +/**
> + * IIO_DEV_ATTR_HYSTERESIS - sets hysterisis value
> + * @_mode: sysfs file mode/permissions
> + * @_show: output method for the attribute
> + * @_store: input method for the attribute
> + **/
> +#define IIO_DEV_ATTR_HYSTERESIS(_mode, _show, _store) \
> + IIO_DEVICE_ATTR(hyst_raw, _mode, _show, _store, 0)
> +
> +/* Common sysfs attributes for HID sensors */
> +struct hid_sensor_attributes {
> + struct hid_sensor_hub_device *hsdev;
> + struct platform_device *pdev;
> + unsigned usage_id;
> + bool data_ready;
> + struct hid_sensor_hub_attribute_info poll;
> + struct hid_sensor_hub_attribute_info report_state;
> + struct hid_sensor_hub_attribute_info power_state;
> + struct hid_sensor_hub_attribute_info sensitivity;
> + void *private;
> +};
> +
> +int hid_sensor_parse_common_attributes(struct hid_sensor_hub_device *hsdev,
> + u32 usage_id,
> + struct hid_sensor_attributes *st);
> +ssize_t hid_sensor_read_hyst_raw(struct device *dev,
> + struct device_attribute *attr,
> + char *buf);
> +ssize_t hid_sensor_write_hyst_raw(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len);
> +ssize_t hid_sensor_read_samp_freq(struct device *dev,
> + struct device_attribute *attr,
> + char *buf);
> +ssize_t hid_sensor_write_samp_freq(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len);
> +
> +#endif
> diff --git a/drivers/staging/iio/common/hid-sensors/hid-sensor-trigger.c b/drivers/staging/iio/common/hid-sensors/hid-sensor-trigger.c
> new file mode 100644
> index 0000000..bef5560
> --- /dev/null
> +++ b/drivers/staging/iio/common/hid-sensors/hid-sensor-trigger.c
> @@ -0,0 +1,104 @@
> +/*
> + * 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/trigger_consumer.h>
> +#include <linux/iio/trigger.h>
> +#include "hid-sensor-attributes.h"
> +
> +static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
> + bool state)
> +{
> + struct iio_dev *indio_dev = trig->private_data;
> + struct hid_sensor_attributes *st = iio_priv(indio_dev);
> + int ret;
> + int state_val;
> +
> + dev_dbg(&indio_dev->dev, "hid_sensor_data_rdy_trigger_set_state %d\n",
> + state);
> + state_val = state ? 1 : 0;
bool is defined 0 or 1 with gcc.
> +#if (defined CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS) || \
> + (defined CONFIG_HID_SENSOR_ENUM_BASE_QUIRKS_MODULE)
> + hid_dbg(st->hsdev, " Using ENUM base FW QUIRKS\n");
> + ++state_val;
> +#endif
> + st->data_ready = state;
> + ret = sensor_hub_set_feature(st->hsdev, st->power_state.report_id,
> + st->power_state.index,
> + (s32)state_val);
more unhandled return values. Please clear these up everywhere.
> +
> + ret = sensor_hub_set_feature(st->hsdev, st->report_state.report_id,
> + st->report_state.index,
> + (s32)state_val);
> + return ret;
> +}
> +
> +void hid_sensor_remove_trigger(struct iio_dev *indio_dev)
> +{
> + iio_trigger_unregister(indio_dev->trig);
> + iio_trigger_free(indio_dev->trig);
> +}
> +EXPORT_SYMBOL(hid_sensor_remove_trigger);
> +
> +static const struct iio_trigger_ops hid_sensor_trigger_ops = {
> + .owner = THIS_MODULE,
> + .set_trigger_state = &hid_sensor_data_rdy_trigger_set_state,
> +};
> +
> +int hid_sensor_setup_trigger(struct iio_dev *indio_dev, char *name)
> +{
> + int ret;
> + struct iio_trigger *trig;
> +
> + trig = iio_trigger_alloc("%s-dev%d", name, indio_dev->id);
> + if (trig == NULL) {
> + dev_err(&indio_dev->dev, "Trigger Allocate Failed\n");
> + ret = -ENOMEM;
> + goto error_ret;
> + }
> +
> + trig->dev.parent = indio_dev->dev.parent;
> + trig->private_data = (void *)indio_dev;
> + trig->ops = &hid_sensor_trigger_ops;
> + ret = iio_trigger_register(trig);
> +
> + if (ret) {
> + dev_err(&indio_dev->dev, "Trigger Register Failed\n");
> + goto error_free_trig;
> + }
> + indio_dev->trig = trig;
> +
> + return ret;
> +
> +error_free_trig:
> + iio_trigger_free(trig);
> +error_ret:
> + return ret;
> +}
> +EXPORT_SYMBOL(hid_sensor_setup_trigger);
> +
> +MODULE_AUTHOR("Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxx>");
> +MODULE_DESCRIPTION("HID Sensor trigger processing");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/staging/iio/common/hid-sensors/hid-sensor-trigger.h b/drivers/staging/iio/common/hid-sensors/hid-sensor-trigger.h
> new file mode 100644
> index 0000000..9ff3d02
> --- /dev/null
> +++ b/drivers/staging/iio/common/hid-sensors/hid-sensor-trigger.h
> @@ -0,0 +1,27 @@
> +/*
> + * 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.
> + *
> + */
> +#ifndef _HID_SENSOR_TRIGGER_H
> +#define _HID_SENSOR_TRIGGER_H
> +
one blank line is plenty
> +
> +
> +int hid_sensor_setup_trigger(struct iio_dev *indio_dev, char *name);
> +void hid_sensor_remove_trigger(struct iio_dev *indio_dev);
> +
> +#endif
>
--
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
