On 07/04/2012 08:52 PM, 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 fine, but in need of some cleanup. Various suggestions inline.
> Signed-off-by: srinivas pandruvada <srinivas.pandruvada@xxxxxxxxx>
> ---
> drivers/staging/iio/accel/Kconfig | 10 +
> drivers/staging/iio/accel/Makefile | 3 +
> drivers/staging/iio/accel/hid-sensor-accel-3d.c | 451 +++++++++++++++++++++++
> 3 files changed, 464 insertions(+), 0 deletions(-)
> create mode 100644 drivers/staging/iio/accel/hid-sensor-accel-3d.c
>
> diff --git a/drivers/staging/iio/accel/Kconfig b/drivers/staging/iio/accel/Kconfig
> index 5ab7167..6ab1efa 100644
> --- a/drivers/staging/iio/accel/Kconfig
> +++ b/drivers/staging/iio/accel/Kconfig
> @@ -102,4 +102,14 @@ config SCA3000
> Say yes here to build support for the VTI SCA3000 series of SPI
> accelerometers. These devices use a hardware ring buffer.
>
> +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 3G.
> +
> endmenu
> diff --git a/drivers/staging/iio/accel/Makefile b/drivers/staging/iio/accel/Makefile
> index 95c6666..2ae9865 100644
> --- a/drivers/staging/iio/accel/Makefile
> +++ b/drivers/staging/iio/accel/Makefile
> @@ -33,3 +33,6 @@ obj-$(CONFIG_LIS3L02DQ) += lis3l02dq.o
>
> sca3000-y := sca3000_core.o sca3000_ring.o
> obj-$(CONFIG_SCA3000) += sca3000.o
> +
> +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/staging/iio/accel/hid-sensor-accel-3d.c b/drivers/staging/iio/accel/hid-sensor-accel-3d.c
> new file mode 100644
> index 0000000..1ee65ce
> --- /dev/null
> +++ b/drivers/staging/iio/accel/hid-sensor-accel-3d.c
> @@ -0,0 +1,451 @@
> +/*
> + * 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/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/sysfs.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"
> +
Make this an enum, then....
> +#define CHANNEL_SCAN_INDEX_X 0
> +#define CHANNEL_SCAN_INDEX_Y 1
> +#define CHANNEL_SCAN_INDEX_Z 2
> +
> +struct accel_3d_sample {
> + u32 accel_x;
> + u32 accel_y;
> + u32 accel_z;
> +} __packed;
> +
> +struct accel_3d_state {
struct hid_sensor_hub_attribute_info accel[3]; (using above enum)
> + struct hid_sensor_hub_attribute_info accel_x;
> + struct hid_sensor_hub_attribute_info accel_y;
> + struct hid_sensor_hub_attribute_info accel_z;
> + struct accel_3d_sample accel_sample_data;
u32 accel_val[3] (also using enum).
> +};
> +
> +/* Channel definitions */
> +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,
> + .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,
> + .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,
> + .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';
> + accel_3d_channels[channel].scan_type.realbits = size * 8;
> + accel_3d_channels[channel].scan_type.storagebits = sizeof(u32) * 8;
Always true? Can't do 16 bit channels in 16 bits?
> +}
> +
> +/* 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 hid_sensor_attributes *st = iio_priv(indio_dev);
> + struct accel_3d_state *accel_state =
> + (struct accel_3d_state *)st->private;
> + int report_id = -1;
> + u32 address;
> +
> + *val = 0;
> + *val2 = 0;
> + switch (mask) {
> + case 0:
With the alternative layout above this becomes
const u32 addresses[3] {
HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_X_AXIS,
HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_Y_AXIS,
HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_Z_AXIS
}; //move this outside as useful elsewhere...
report_id = accel_state->accel[chan->scan_index].report_id;
address = addresses[chan->scan_index];
> + switch (chan->scan_index) {
> + case CHANNEL_SCAN_INDEX_X:
> + report_id = accel_state->accel_x.report_id;
> + address =
> + HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_X_AXIS;
> + break;
> + case CHANNEL_SCAN_INDEX_Y:
> + report_id = accel_state->accel_y.report_id;
> + address =
> + HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_Y_AXIS;
> + break;
> + case CHANNEL_SCAN_INDEX_Z:
> + report_id = accel_state->accel_z.report_id;
> + address =
> + HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_Z_AXIS;
> + break;
> + default:
> + report_id = -1;
> + break;
> + }
> + if (report_id >= 0)
> + *val = sensor_hub_input_attr_get_raw_value(
> + st->hsdev,
> + HID_USAGE_SENSOR_ACCEL_3D, address,
> + report_id);
> + else
> + *val = 0;
> + break;
> + case IIO_CHAN_INFO_SCALE:
This works as they are all the same, but more obviously true
(with the above defines as)
*val = accel_state->accel[chan->scan_index].units;
> + *val = accel_state->accel_x.units;
> + break;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = accel_state->accel_x.unit_expo;
> + break;
> + default:
> + break;
> + }
> + return IIO_VAL_INT;
> +}
> +
> +
> +
> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
> + hid_sensor_read_samp_freq,
> + hid_sensor_write_samp_freq);
> +
> +static IIO_DEV_ATTR_HYSTERESIS(S_IWUSR | S_IRUSR,
> + hid_sensor_read_hyst_raw,
> + hid_sensor_write_hyst_raw);
rant about formatting of hystersis in previous email.
Anyhow, I'd like to have hystersis defined as an
element of the chan_spec info_mask as it keeps coming
up. That cuts us down to just sampling frequency and
we could do that as
accel_sampling_frequency which already exists.
The key thing is that if it isn't in info_mask, then
it pretty much isn't available to any in kernel
users of the device...
> +
> +static struct attribute *accel_3d_attributes[] = {
> + /* common attributes */
> + &iio_dev_attr_sampling_frequency.dev_attr.attr,
> + &iio_dev_attr_hyst_raw.dev_attr.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group accel_3d_attribute_group = {
> + .attrs = accel_3d_attributes,
> +};
> +
> +static const struct iio_info accel_3d_info = {
> + .attrs = &accel_3d_attribute_group,
> + .driver_module = THIS_MODULE,
> + .read_raw = &accel_3d_read_raw,
> +};
> +
> +/* 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 =
> + (struct iio_dev *)platform_get_drvdata(priv);
Please clear out all these casts from void *'s just makes for messy code.
> + struct hid_sensor_attributes *st = iio_priv(indio_dev);
> + struct accel_3d_state *accel_state =
> + (struct accel_3d_state *)st->private;
> +
> + dev_dbg(&indio_dev->dev, "accel_3d_proc_event [%d]\n", st->data_ready);
> + if (st->data_ready)
> + hid_sensor_push_data(indio_dev,
> + (u8 *)&accel_state->accel_sample_data,
> + sizeof(struct accel_3d_sample));
> + 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 =
> + (struct iio_dev *)platform_get_drvdata(priv);
> + struct hid_sensor_attributes *st = iio_priv(indio_dev);
> + struct accel_3d_state *accel_state =
> + (struct accel_3d_state *)st->private;
> +
Feels like one ought to be able to decode an axis from that usage_id
and make this much shorter. Would have to look back at the earlier
patch to find out what would work and I'm lazy ;)
> + switch (usage_id) {
> + case HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_X_AXIS:
> + accel_state->accel_sample_data.accel_x =
> + *(u32 *)raw_data;
> + break;
> + case HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_Y_AXIS:
> + accel_state->accel_sample_data.accel_y =
> + *(u32 *)raw_data;
> + break;
> + case HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_Z_AXIS:
> + accel_state->accel_sample_data.accel_z =
> + *(u32 *)raw_data;
> + break;
> + default:
> + break;
> + }
> + return 0;
> +}
> +
> +/* 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;
> +
Lots of missing error handling. Fix this.
Structures should also allow this to be a simple loop over the channels.
> + ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT,
> + usage_id,
> + HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_X_AXIS,
> + &st->accel_x);
> + accel_3d_adjust_channel_bit_mask(CHANNEL_SCAN_INDEX_X,
> + st->accel_x.size);
> +
> + ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT,
> + usage_id,
> + HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_Y_AXIS,
> + &st->accel_y);
> + accel_3d_adjust_channel_bit_mask(CHANNEL_SCAN_INDEX_Y,
> + st->accel_y.size);
> +
> + ret = sensor_hub_input_get_attribute_info(hsdev, HID_INPUT_REPORT,
> + usage_id,
> + HID_USAGE_SENSOR_DATA_MOTION_ACCELERATION_Z_AXIS,
> + &st->accel_z);
> + accel_3d_adjust_channel_bit_mask(CHANNEL_SCAN_INDEX_Z,
> + st->accel_z.size);
> +
> + dev_dbg(&pdev->dev, "accel_3d %x:%x, %x:%x, %x:%x\n",
> + st->accel_x.index,
> + st->accel_x.report_id,
> + st->accel_y.index, st->accel_y.report_id,
> + st->accel_z.index, st->accel_z.report_id);
> +
> + return 0;
> +}
> +
> +/* Function to initialize the processing for usage id */
> +static int accel_3d_init(struct platform_device *pdev,
> + struct hid_sensor_hub_device *hsdev,
> + unsigned usage_id)
> +{
> + int ret = 0;
> + static char *name = "accel_3d";
> + struct iio_dev *indio_dev;
> + struct accel_3d_state *accel_state;
> + struct hid_sensor_attributes *st;
> +
> + accel_state = kzalloc(sizeof(struct accel_3d_state), GFP_KERNEL);
> + if (accel_state == NULL) {
> + ret = -ENOMEM;
> + goto error_ret;
> + }
> +
> + indio_dev = iio_device_alloc(sizeof(struct hid_sensor_attributes));
I'd normally expect accel_state to be in iio_priv(indio_dev)... That
might then embed st.
> + if (indio_dev == NULL) {
> + ret = -ENOMEM;
> + goto error_free_state;
> + }
> + platform_set_drvdata(pdev, indio_dev);
> + st = iio_priv(indio_dev);
> + st->hsdev = hsdev;
> + st->pdev = pdev;
> + st->private = (void *)accel_state;
> +
> + ret = hid_sensor_parse_common_attributes(hsdev, usage_id, st);
> + 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;
> + }
> +
> + 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);
> + 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;
> + }
Convention is to nor enable anything initially. Leaves everything
up to userspace (and avoids some annoying race conditions that
have a habit of showing up!)
> + iio_scan_mask_set(indio_dev, indio_dev->buffer,
> + CHANNEL_SCAN_INDEX_X);
> + iio_scan_mask_set(indio_dev, indio_dev->buffer,
> + CHANNEL_SCAN_INDEX_Y);
> + iio_scan_mask_set(indio_dev, indio_dev->buffer,
> + CHANNEL_SCAN_INDEX_Z);
> + st->data_ready = false;
> + ret = hid_sensor_setup_trigger(indio_dev, name);
> + 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_free_state:
> + kfree(accel_state);
> +error_ret:
> + return ret;
> +}
> +
> +/* Function to deinitialize the processing for usage id */
> +static int accel_3d_exit(struct platform_device *pdev)
> +{
> + int ret = 0;
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct hid_sensor_attributes *st = iio_priv(indio_dev);
> + struct accel_3d_state *accel_state =
> + (struct accel_3d_state *)st->private;
st->private is void * so shouldn't need type cast to assign it
to anything.
> +
> + 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);
> + kfree(accel_state);
> + return ret;
> +}
> +
> +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;
> + u32 *pdata = (u32 *)pdev->dev.platform_data;
> + struct hid_sensor_hub_device *hsdev;
> +
> + hsdev = (struct hid_sensor_hub_device *)*pdata;
> + 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;
> + ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_ACCEL_3D,
> + &accel_3d_callbacks);
> + return 0;
> +}
> +
> +static int __devinit hid_accel_3d_remove(struct platform_device *pdev)
> +{
> + u32 *pdata = (u32 *)pdev->dev.platform_data;
> + struct hid_sensor_hub_device *hsdev;
> +
> + hsdev = (struct hid_sensor_hub_device *)*pdata;
why all the type casting mess here? If pdev->dev.platform_data is not
an hid_sensor_hub_device * then we need a really really clear
explation of why not.
> + 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,
> +};
> +
> +/*--------------------------------------------------------------------------*/
> +
> +static int __init hid_accel_3d_init(void)
> +{
> + return platform_driver_register(&hid_accel_3d_platform_driver);
> +}
> +
> +static void __exit hid_accel_3d_exit(void)
> +{
> + platform_driver_unregister(&hid_accel_3d_platform_driver);
> +}
> +
> +module_init(hid_accel_3d_init);
> +module_exit(hid_accel_3d_exit);
module_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-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
