by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v3 2/5] iio: sensorhub: Add sensorhub bindings

On 16/12/14 07:30, Karol Wrona wrote:
> On 12/06/2014 03:29 PM, Jonathan Cameron wrote:
>> On 05/12/14 19:54, Karol Wrona wrote:
>>> Add sensorhub bindings for sensorhub on Galaxy Gear 2.
>>>
>>> Change-Id: I4ee25aef33c21a4662de230841de9a8684f2c26b
>>> Signed-off-by: Karol Wrona <[email protected]>
>>> Acked-by: Kyungmin Park <[email protected]>
>> Looks good to me. Comments inline. Note I either need a device tree
>> ack or a long delay before I can take this though. Unlikely to get the
>> device tree ack as you haven't cc'd the maintainers or list ;)
> I am curious about DT community opinion and I prefer to know it before
> v4 sending.
They have a lot of patches to review, so might take a while ;)
>
>>
>> I've added the cc's. Please make sure future versions have them or you'll
>> just be slowing the process down (particularly if no one notices they
>> are missing!)
> Thanks, I will remember that.
>>
>>> ---
>>> .../devicetree/bindings/iio/sensorhub.txt | 46 ++++++++++++++++++++
>>> 1 file changed, 46 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/iio/sensorhub.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/sensorhub.txt b/Documentation/devicetree/bindings/iio/sensorhub.txt
>>> new file mode 100644
>>> index 0000000..2aca0c3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/sensorhub.txt
>>> @@ -0,0 +1,46 @@
>>> +Samsung Sensorhub driver
>>> +
>>> +Sensorhub is a MCU which manages several sensors and also plays the role
>>> +of a virtual sensor device.
>>> +
>>> +Required properties:
>>> +- compatible: "samsung,sensorhub-rinato" or "samsung,sensorhub-thermostat"
>>> +- spi-max-frequency: max SPI clock frequency
>>> +- interrupt-parent: interrupt parent
>>> +- interrupts: communication interrupt
>>> +- ap-mcu-gpio: [out] ap to sensorhub line - used during communication
>>> +- mcu-ap-gpio: [in] sensorhub to ap - used during communication
>>> +- mcu-reset: [out] sensorhub reset
>> as commented in previous patch, why do the first two have -gpio and the
>> third not?
> Ok, all 3 will have "gpios" ending
>>> +
>>> +Optional properties:
>>> +- sensor's nodes:
>>> + compatible = "samsung,mpu6500-accel"
>>> + compatible = "samsung,mpu6500-gyro"
>>> + compatible = "samsung,adpd142"
>>> +
>>> +Sensor compatibles are used to match proper sensor driver to real sensor on
>>> +the board. The firmware does not give such information, so it helps to specify
>>> +some sensors properties. Sensors have "samsung" prefixes because frequently
>>> +they will not have much in common with sensors used without sensorhub because
>>> +it can do some data processing.
>> We'll keep that under review. Might make sense, sometimes, to unify the
>> drivers. The different compatible will probably still be needed, but if
>> these devices proliferate I don't want two drivers for everything that
>> gets stuck behind them.
> I think that sensorhub is very similar hid-sensor (without hotpluging) where
> every sensor type has its own representation. The real sensors are not driven
> directly but by external MCU so very often these sensors will not have much
> common with real ones. So I wonder if it could be better to resign from these
> sensor compatibles and represent each sensor as mfd cell so optional properties
> probably will dropped (?)
Might do.
> Generally I intended to have i.e. one ssp-accel driver if they use another
> accelerometer then mpu6500 the buffering manner and sysfs will be handled in the
> same way. I think I was mistaken with these compatibles. There is also some
> probability that the firmware will be fixed someday and it will be able to tell
> more about sensor then the type.
Would be good - particularly if we start to get lots of variants out in the wild.
>
>>> +
>>> +Example:
>>> +
>>> + shub_spi: shub {
>>> + compatible = "samsung,sensorhub-rinato";
>>> + spi-max-frequency = <5000000>;
>>> + interrupt-parent = <&gpx0>;
>>> + interrupts = <2 0>;
>>> + ap-mcu-gpio = <&gpx0 0 0>;
>>> + mcu-ap-gpio = <&gpx0 4 0>;
>>> + mcu-reset = <&gpx0 5 0>;
>>> + sensor@0 {
>>> + compatible = "samsung,mpu6500-accel";
>>> + };
>>> + sensor@1 {
>>> + compatible = "samsung,mpu6500-gyro";
>>> + };
>>> + sensor@2 {
>>> + compatible = "samsung,adpd142";
>>> + };
>>> + };
>>>
>>
>>
>

2014-12-26 12:34:47

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v3 4/5] iio: common: ssp_sensors: Add sensorhub accelerometer sensor

On 08/12/14 10:41, Karol Wrona wrote:
> 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 <[email protected]>
>>> Acked-by: Kyungmin Park <[email protected]>
>>> ---
>>> 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?
Yup. Just suggested the same in a review of an ACPI ALS driver.
We'd currently handle it exactly the same as a hardware buffer
but without the current naming (and with the potential to do
it differently if needed in future).

>>
>>> + 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.
I'd just expand on the comment a little.
something like - /* As this registers for data flow, we need to be sure it is after all other setup */
>>> + 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 <[email protected]>");
>>> +MODULE_DESCRIPTION("Samsung sensorhub accelerometers driver");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html