On 06/26/2012 02:54 AM, srinivas pandruvada wrote:
> Added usage id processing for Accelrometer 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.
Hi,
Some comments inline, similar comments also apply for the other IIO drivers
in this series.
> [...]
> 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..77d4663
> --- /dev/null
> +++ b/drivers/staging/iio/accel/hid-sensor-accel-3d.c
> @@ -0,0 +1,537 @@
> [...]
> +#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 "../iio.h"
> +#include "../sysfs.h"
> +#include "../trigger.h"
> +#include "../kfifo_buf.h"
> +#include "../trigger_consumer.h"
> +#include "../triggered_buffer.h"
IIO has moved out of staging in 3.5, so this wont build with the latest
upstream.
> +
> +/*Format: HID-SENSOR-usage_id_in_hex*/
> +#define DRIVER_NAME "HID-SENSOR-200073"
> +
> +#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_device *hsdev;
> + struct platform_device *pdev;
> + struct hid_sensor_common_attributes *common_attrb;
> + 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;
> + bool data_ready;
> +};
> +
> +static int hid_sensor_data_rdy_trigger_set_state(struct iio_trigger *trig,
> + bool state)
> +{
> + struct iio_dev *indio_dev = trig->private_data;
> + struct accel_3d_state *st = iio_priv(indio_dev);
> + int state_val;
> + char buffer[16];
> +
> + dev_dbg(&indio_dev->dev, "hid_sensor_data_rdy_trigger_set_state %d\n",
> + state);
> + st->data_ready = state;
> + state_val = state ? 1 : 0;
> + sprintf(buffer, "%d", state_val);
> + hid_sensor_write_report_state(st->common_attrb, strlen(buffer), buffer);
> + hid_sensor_write_power_state(st->common_attrb, strlen(buffer), buffer);
This looks kind of odd, passing a integer by using a string.
> +
> + return 0;
> +}
> +
> +void hid_sensor_remove_trigger(struct iio_dev *indio_dev)
> +{
> + iio_trigger_unregister(indio_dev->trig);
> + iio_free_trigger(indio_dev->trig);
> +}
> +
> +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_allocate_trigger("%s-dev%d", name, indio_dev->id);
> + if (trig == NULL) {
> + 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);
> +
> + /* select default trigger */
> + indio_dev->trig = trig;
> + if (ret)
> + goto error_free_trig;
> +
> + return ret;
> +
> +error_free_trig:
> + iio_free_trigger(trig);
> +error_ret:
> + return ret;
> +}
> +
The trigger code looks pretty similar in all your IIO drivers, maybe it
makes sense to put this into a common module.
> +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,
> + }
> +};
> +
> +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;
I would this expect to fail badly if there were two devices with different
specs registered at the same time.
> +}
> +
> +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;
> +
> + *val = 0;
> + *val2 = 0;
> + switch (mask) {
> + case 0:
> + 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(
> + accel_state->hsdev,
> + HID_USAGE_SENSOR_ACCEL_3D, address,
> + report_id);
> + else
> + *val = 0;
> + break;
> + case IIO_CHAN_INFO_SCALE:
> + *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 int accel_3d_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val,
> + int val2,
> + long mask)
> +{
> + dev_err(&indio_dev->dev, "%s\n", __func__);
> + return 0;
> +}
If you don't want to do anything here just don't set the callback in the
iio_info struct.
> +
> +static ssize_t hid_sensor_accel_3d_read_hyst_raw(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int len;
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct accel_3d_state *accel_state = iio_priv(indio_dev);
> + len = hid_sensor_read_hyst_raw(accel_state->common_attrb, strlen(buf),
> + buf);
> + return len;
> +}
> +
> +static ssize_t hid_sensor_accel_3d_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 accel_3d_state *accel_state = iio_priv(indio_dev);
> + return hid_sensor_write_hyst_raw(accel_state->common_attrb,
> + strlen(buf), buf);
> +}
> +
> +static ssize_t hid_sensor_read_accel_3d_samp_freq(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct accel_3d_state *accel_state = iio_priv(indio_dev);
> +
> + return hid_sensor_read_samp_freq(accel_state->common_attrb,
> + strlen(buf), buf);
This doesn't make sense, buf is empty at this point, so a strlen will return 0.
> +}
> +
> +static ssize_t hid_sensor_write_accel_3d_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 accel_3d_state *accel_state = iio_priv(indio_dev);
> +
> + return hid_sensor_write_samp_freq(accel_state->common_attrb,
> + strlen(buf), buf);
> +}
> +
> +static IIO_DEV_ATTR_SAMP_FREQ(S_IWUSR | S_IRUGO,
> + hid_sensor_read_accel_3d_samp_freq,
> + hid_sensor_write_accel_3d_samp_freq);
> +
> +#define IIO_DEV_ATTR_HYSTERESIS(_mode, _show, _store, _addr) \
> + IIO_DEVICE_ATTR(hyst_raw, _mode, _show, _store, _addr)
> +
> +static IIO_DEV_ATTR_HYSTERESIS(S_IWUSR | S_IRUSR,
> + hid_sensor_accel_3d_read_hyst_raw,
> + hid_sensor_accel_3d_write_hyst_raw,
> + HID_USAGE_SENSOR_PROPERTY_CHANGE_SENSITIVITY_ABS);
> +
> +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,
> + .write_raw = &accel_3d_write_raw,
> +};
> +
> +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;
> +
> + if (!buffer)
> + 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);
> +}
> +
> +static irqreturn_t hid_sensor_trigger_handler(int irq, void *p)
> +{
> + return IRQ_HANDLED;
> +}
If you don't want to do anything here just pass NULL to
iio_triggered_buffer_setup.
> [...]
> +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(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-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
