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-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html