lkml.org 
[lkml]   [2014]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v3 4/5] iio: common: ssp_sensors: Add sensorhub accelerometer sensor
On 12/06/2014 03:52 PM, Jonathan Cameron wrote:
> On 05/12/14 19:54, Karol Wrona wrote:
>> This patch adds accelerometer iio driver which uses sensorhub as data
>> provider.
>>
>> Change-Id: I4686741b7401ec5cbf4b5d0f2a5cc146fbe24d53
>> Signed-off-by: Karol Wrona <k.wrona@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>> drivers/iio/accel/Makefile | 1 +
>> drivers/iio/accel/ssp_accel_sensor.c | 203 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 204 insertions(+)
>> create mode 100644 drivers/iio/accel/ssp_accel_sensor.c
>>
>> diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
>> index a593996..df6a0b2 100644
>> --- a/drivers/iio/accel/Makefile
>> +++ b/drivers/iio/accel/Makefile
>> @@ -9,6 +9,7 @@ obj-$(CONFIG_HID_SENSOR_ACCEL_3D) += hid-sensor-accel-3d.o
>> obj-$(CONFIG_KXCJK1013) += kxcjk-1013.o
>> obj-$(CONFIG_KXSD9) += kxsd9.o
>> obj-$(CONFIG_MMA8452) += mma8452.o
>> +obj-$(CONFIG_IIO_SSP_SENSOR) += ssp_accel_sensor.o
>>
>> obj-$(CONFIG_IIO_ST_ACCEL_3AXIS) += st_accel.o
>> st_accel-y := st_accel_core.o
>> diff --git a/drivers/iio/accel/ssp_accel_sensor.c b/drivers/iio/accel/ssp_accel_sensor.c
>> new file mode 100644
>> index 0000000..0a47c29
>> --- /dev/null
>> +++ b/drivers/iio/accel/ssp_accel_sensor.c
>> @@ -0,0 +1,203 @@
>> +/*
>> + * Copyright (C) 2014, Samsung Electronics Co. Ltd. All Rights Reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that 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.
>> + *
>> + */
>> +
>> +#include <linux/iio/common/ssp_sensors.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/kfifo_buf.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include "../common/ssp_sensors/ssp_iio_sensor.h"
>> +
>> +#define SSP_CHANNEL_COUNT 3
>> +
>> +#define SSP_ACCEL_NAME "ssp-accelerometer"
>> +static const char ssp_accel_device_name[] = SSP_ACCEL_NAME;
>> +
>> +enum ssp_accel_3d_channel {
>> + SSP_CHANNEL_SCAN_INDEX_X,
>> + SSP_CHANNEL_SCAN_INDEX_Y,
>> + SSP_CHANNEL_SCAN_INDEX_Z,
>> + SSP_CHANNEL_SCAN_INDEX_TIME,
>> + SSP_ACCEL_3D_CHANNEL_MAX,
> you don't actually use this max element so drop it.
>> +};
>> +
>> +static int ssp_accel_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int *val,
>> + int *val2, long mask)
>> +{
>> + u32 t;
>> + struct ssp_data *data = dev_get_drvdata(indio_dev->dev.parent->parent);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + t = ssp_get_sensor_delay(data, SSP_ACCELEROMETER_SENSOR);
>> + ssp_convert_to_freq(t, val, val2);
>> + return IIO_VAL_INT_PLUS_MICRO;
>> + default:
>> + break;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int ssp_accel_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int val,
>> + int val2, long mask)
>> +{
>> + int ret;
>> + struct ssp_data *data = dev_get_drvdata(indio_dev->dev.parent->parent);
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_SAMP_FREQ:
>> + ret = ssp_convert_to_time(val, val2);
>> + ret = ssp_change_delay(data, SSP_ACCELEROMETER_SENSOR, ret);
>> + if (ret < 0)
>> + dev_err(&indio_dev->dev, "accel sensor enable fail\n");
>> +
>> + return ret;
>> + default:
>> + break;
>> + }
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static struct iio_info ssp_accel_iio_info = {
>> + .read_raw = &ssp_accel_read_raw,
>> + .write_raw = &ssp_accel_write_raw,
>> +};
>> +
>> +static const struct iio_chan_spec ssp_acc_channels[] = {
>> + SSP_CHANNEL_AG(IIO_ACCEL, IIO_MOD_X, SSP_CHANNEL_SCAN_INDEX_X),
>> + SSP_CHANNEL_AG(IIO_ACCEL, IIO_MOD_Y, SSP_CHANNEL_SCAN_INDEX_Y),
>> + SSP_CHANNEL_AG(IIO_ACCEL, IIO_MOD_Z, SSP_CHANNEL_SCAN_INDEX_Z),
>> + IIO_CHAN_SOFT_TIMESTAMP(SSP_CHANNEL_SCAN_INDEX_TIME),
> hmm. Not actually a soft timestamp so I guess we should really rename that
> macro. Actually is your resulting timestamp 32 bit? If so you could
> specify the channel as such and include a scale to allow userspace to
> convert it as it wishes.
Ok, I will rethink that. This timestamp is 64-bit and it is system time
plus diff taken from sample but I would have relative time at all on 32-bit.
>> +};
>> +
>> +static int ssp_process_accel_data(struct iio_dev *indio_dev, void *buf,
>> + int64_t timestamp)
> We could hang a set of triggers of the parent device and use the in
> conjunction with some local buffering to fire off the children.
> There would be the advantage that we could trigger other sensors off them
> (to get roughly synchronized additional signals). Probably not worth it
> though but I thought I'd mention the option.
>> +{
>> + __le32 time;
>> + const int len = SSP_ACCELEROMETER_SIZE;
>> + int64_t calculated_time;
>> + char *data;
>> + int ret;
>> +
>> + if (indio_dev->scan_bytes == 0)
>> + return 0;
>> +
>> + data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
> I'd allocate this in your iio_priv structure and setup the length in preenable
> etc. Last thing we want in here is to waste time with a memory allocation and
> free.
You are right. Generally for commands in ssp_spi.c these allocation won't hurt
but here I created a pure waste..
>> +
>> + memcpy(data, buf, len);
> It's a little uggly but I think you can avoid the copy if the timestamp
> isn't in use (as then we don't need the additional space).
>> +
>> + if (indio_dev->scan_timestamp) {
>> + memcpy(&time, &((char *)buf)[len], SSP_TIME_SIZE);
>> + calculated_time =
>> + timestamp + (int64_t)le32_to_cpu(time) * 1000000;
> Cool - our first hardware supplied timestamp ;)
>> + }
>> +
>> + ret = iio_push_to_buffers_with_timestamp(indio_dev, data,
>> + calculated_time);
>> +
>> + kfree(data);
>> +
>> + return ret;
>> +}
>> +
>> +static const struct iio_buffer_setup_ops ssp_accel_buffer_ops = {
>> + .postenable = &ssp_common_buffer_postenable,
>> + .predisable = &ssp_common_buffer_predisable,
>> +};
>> +
>> +static int ssp_accel_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> + struct iio_dev *indio_dev;
>> + struct ssp_sensor_data *spd;
>> +
>> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*spd));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + spd = iio_priv(indio_dev);
>> +
>> + spd->process_data = ssp_process_accel_data;
>> + spd->type = SSP_ACCELEROMETER_SENSOR;
>> +
>> + indio_dev->name = ssp_accel_device_name;
>> + indio_dev->dev.parent = &pdev->dev;
>> + indio_dev->dev.of_node = pdev->dev.of_node;
>> + indio_dev->info = &ssp_accel_iio_info;
>> + indio_dev->modes = INDIO_BUFFER_HARDWARE;
> hmm. A rare beast with no polled access. But is it actually a hardware
> buffer? Doesn't look that much like one. I think we may need a more
> refined set of options for the buffer type.
Here I'm confused. Do you mean a new buffer type in IIO?
>
>> + indio_dev->channels = ssp_acc_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(ssp_acc_channels);
>> +
>> + ret = ssp_common_setup_buffer(indio_dev, &ssp_accel_buffer_ops);
>> + if (ret < 0) {
>> + dev_err(&pdev->dev, "Buffer setup fail\n");
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, indio_dev);
>> +
>> + ret = iio_device_register(indio_dev);
>> + if (ret < 0) {
>> + iio_buffer_unregister(indio_dev);
>> + iio_kfifo_free(indio_dev->buffer);
>> + return ret;
>> + }
>> +
>> + /* need to be really sure about success for this one */
> err, what does the comment mean?
this only adds ptr to the table so I wanted to assure that it should be done
after ii_device_register, maybe this is too obvious to comment.
>> + ssp_register_consumer(indio_dev, SSP_ACCELEROMETER_SENSOR);
>> +
>> + return 0;
>> +}
>> +
>> +static int ssp_accel_remove(struct platform_device *pdev)
>> +{
>> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
>> +
>> + iio_device_unregister(indio_dev);
>> + iio_buffer_unregister(indio_dev);
>> + iio_kfifo_free(indio_dev->buffer);
> Feels rather like we need a devm_iio_kfifo_alloc
> Actually, as you are wrapping the allocation, I'd prefer you have
> an obviously matched cleanup function to ssp_common_setup_buffer
>> +
>> + return 0;
>> +}
>> +
>> +static const struct of_device_id ssp_accel_id_table[] = {
>> + {
>> + .compatible = "samsung,mpu6500-accel",
>> + },
>> + {},
>> +};
>> +
>> +static struct platform_driver ssp_accel_driver = {
>> + .driver = {
>> + .owner = THIS_MODULE,
>> + .name = SSP_ACCEL_NAME,
>> + .of_match_table = ssp_accel_id_table,
>> + },
>> + .probe = ssp_accel_probe,
>> + .remove = ssp_accel_remove,
>> +};
>> +
>> +module_platform_driver(ssp_accel_driver);
>> +
>> +MODULE_AUTHOR("Karol Wrona <k.wrona@samsung.com>");
>> +MODULE_DESCRIPTION("Samsung sensorhub accelerometers driver");
>> +MODULE_LICENSE("GPL");
>>
>
>



\
 
 \ /
  Last update: 2014-12-08 12:21    [W:0.081 / U:0.524 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site