On 14/01/17 17:48, Andreas Klinger wrote:
> Hi Jonathan,
>
> see comments below.
>
> Andreas
>
>
> Jonathan Cameron <ji...@kernel.org> schrieb am Sat, 14. Jan 12:17:
>> On 10/01/17 18:48, Andreas Klinger wrote:
>>> This is the IIO driver for devantech srf08 ultrasonic ranger which can be
>>> used to measure the distances to an object.
>>>
>>> The sensor supports I2C with some registers.
>>>
>>> Supported Features include:
>>> - read the distance in ranging mode in centimeters
>>> - output of the driver is directly the read value
>>> - together with the scale the driver delivers the distance in meters
>>> - only the first echo of the nearest object is delivered
>>> - set max gain register; userspace enters analogue value
>>> - set range registers; userspace enters range in millimeters in 43 mm steps
>>>
>>> Features not supported by this driver:
>>> - ranging mode in inches or in microseconds
>>> - ANN mode
>>> - change I2C address through this driver
>>> - light sensor
>>>
>>> The driver was added in the directory "proximity" of the iio subsystem
>>> in absence of another directory named "distance".
>>> There is also a new submenu "distance"
>> Hi Andreas,
>>
>> Sorry it took me a while to get to this!
>>
>> I'd not bother with the new submenu. Perhaps we should rename the
>> proximity menu to proximity/distance.
>>
>> We already the lightening detector in there which is definitely not
>> measuring proximity in the convetional sense!
>>
>> Anyhow, the actual code is fine, but we need to think about how the
>> userspace ABI fits within the wider IIO ABI. Naming and approaches
>> that make sense in a single class of drivers can end up meaining
>> very different things for other drivers. Various suggestions inline.
>>
>> Jonathan
>>>
>>> Signed-off-by: Andreas Klinger <a...@it-klinger.de>
>>> ---
>>> drivers/iio/proximity/Kconfig | 15 ++
>>> drivers/iio/proximity/Makefile | 1 +
>>> drivers/iio/proximity/srf08.c | 362
>>> +++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 378 insertions(+)
>>> create mode 100644 drivers/iio/proximity/srf08.c
>>>
>>> diff --git a/drivers/iio/proximity/Kconfig b/drivers/iio/proximity/Kconfig
>>> index ef4c73db5b53..7b10a137702b 100644
>>> --- a/drivers/iio/proximity/Kconfig
>>> +++ b/drivers/iio/proximity/Kconfig
>>> @@ -46,3 +46,18 @@ config SX9500
>>> module will be called sx9500.
>>>
>>> endmenu
>>> +
>>> +menu "Distance sensors"
>>> +
>>> +config SRF08
>>> + tristate "Devantech SRF08 ultrasonic ranger sensor"
>>> + depends on I2C
>>> + help
>>> + Say Y here to build a driver for Devantech SRF08 ultrasonic
>>> + ranger sensor. This driver can be used to measure the distance
>>> + of objects.
>>> +
>>> + To compile this driver as a module, choose M here: the
>>> + module will be called srf08.
>>> +
>>> +endmenu
>>> diff --git a/drivers/iio/proximity/Makefile b/drivers/iio/proximity/Makefile
>>> index 9aadd9a8ee99..e914c2a5dd49 100644
>>> --- a/drivers/iio/proximity/Makefile
>>> +++ b/drivers/iio/proximity/Makefile
>>> @@ -5,4 +5,5 @@
>>> # When adding new entries keep the list in alphabetical order
>>> obj-$(CONFIG_AS3935) += as3935.o
>>> obj-$(CONFIG_LIDAR_LITE_V2) += pulsedlight-lidar-lite-v2.o
>>> +obj-$(CONFIG_SRF08) += srf08.o
>>> obj-$(CONFIG_SX9500) += sx9500.o
>>> diff --git a/drivers/iio/proximity/srf08.c b/drivers/iio/proximity/srf08.c
>>> new file mode 100644
>>> index 000000000000..f38c74ed0933
>>> --- /dev/null
>>> +++ b/drivers/iio/proximity/srf08.c
>>> @@ -0,0 +1,362 @@
>>> +/*
>>> + * srf08.c - Support for Devantech SRF08 ultrasonic ranger
>>> + *
>>> + * Copyright (c) 2016 Andreas Klinger <a...@it-klinger.de>
>>> + *
>>> + * This file is subject to the terms and conditions of version 2 of
>>> + * the GNU General Public License. See the file COPYING in the main
>>> + * directory of this archive for more details.
>>> + *
>>> + * For details about the device see:
>>> + * http://www.robot-electronics.co.uk/htm/srf08tech.html
>>> + */
>>> +
>>> +#include <linux/err.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/delay.h>
>>> +#include <linux/module.h>
>>> +#include <linux/bitops.h>
>>> +#include <linux/iio/iio.h>
>>> +#include <linux/iio/sysfs.h>
>>> +
>>> +/* registers of SRF08 device */
>>> +#define SRF08_WRITE_COMMAND 0x00 /* Command Register */
>>> +#define SRF08_WRITE_MAX_GAIN 0x01 /* Max Gain Register: 0 .. 31 */
>>> +#define SRF08_WRITE_RANGE 0x02 /* Range Register: 0 .. 255 */
>>> +#define SRF08_READ_SW_REVISION 0x00 /* Software Revision */
>>> +#define SRF08_READ_LIGHT 0x01 /* Light Sensor during last echo */
>>> +#define SRF08_READ_ECHO_1_HIGH 0x02 /* Range of first echo received
>>> */
>>> +#define SRF08_READ_ECHO_1_LOW 0x03 /* Range of first echo received
>>> */
>>> +
>>> +#define SRF08_CMD_RANGING_CM 0x51 /* Ranging Mode - Result in cm
>>> */
>>> +
>>> +#define SRF08_DEFAULT_GAIN 1025 /* max. analogue value of Gain */
>>> +#define SRF08_DEFAULT_RANGE 11008 /* max. value of Range in mm */
>>> +
>>> +struct srf08_data {
>>> + struct i2c_client *client;
>>> + int gain; /* Max Gain */
>>> + int range_mm; /* Range in mm */
>>> + struct mutex lock;
>>> +};
>>> +
>>> +static const int srf08_gain[] = {
>>> + 94, 97, 100, 103, 107, 110, 114, 118,
>>> + 123, 128, 133, 139, 145, 152, 159, 168,
>>> + 177, 187, 199, 212, 227, 245, 265, 288,
>>> + 317, 352, 395, 450, 524, 626, 777, 1025 };
>>> +
>>> +static int srf08_read_ranging(struct srf08_data *data)
>>> +{
>>> + struct i2c_client *client = data->client;
>>> + int ret, i;
>>> +
>>> + mutex_lock(&data->lock);
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + SRF08_WRITE_COMMAND, SRF08_CMD_RANGING_CM);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "write command - err: %d\n", ret);
>>> + mutex_unlock(&data->lock);
>>> + return ret;
>>> + }
>>> +
>>> + /*
>>> + * normally after 65 ms the device should have the read value
>>> + * we round it up to 100 ms
>> I'd suggest this should be adapted so that it takes advantage of knowing
>> roughly how long it is going to take as the 'range' maximum is changed.
>> So perhaps in the basic case, sleep for 65 msecs, then poll at 5msec
>> intervals. If we know it's going to be a lot faster, then poll it from
>> an earlier time.
>>> + *
>>> + * we read here until a correct version number shows up as
>>> + * suggested by the documentation
>>> + */
>>> + for (i = 0; i < 5; i++) {
>>> + ret = i2c_smbus_read_byte_data(data->client,
>>> + SRF08_READ_SW_REVISION);
>>> +
>>> + /* check if a valid version number is read */
>>> + if (ret < 255 && ret > 0)
>>> + break;
>>> + msleep(20);
>>> + }
>>> +
>>> + if (ret >= 255 || ret <= 0) {
>>> + dev_err(&client->dev, "device not ready\n");
>>> + mutex_unlock(&data->lock);
>>> + return -EIO;
>>> + }
>>> +
>>> + ret = i2c_smbus_read_word_swapped(data->client,
>>> + SRF08_READ_ECHO_1_HIGH);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "cannot read distance: ret=%d\n", ret);
>>> + mutex_unlock(&data->lock);
>>> + return ret;
>>> + }
>>> +
>>> + mutex_unlock(&data->lock);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int srf08_read_raw(struct iio_dev *indio_dev,
>>> + struct iio_chan_spec const *channel, int *val,
>>> + int *val2, long mask)
>>> +{
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> + int ret;
>>> +
>>> + if (channel->type != IIO_DISTANCE)
>>> + return -EINVAL;
>>> +
>>> + switch (mask) {
>>> + case IIO_CHAN_INFO_RAW:
>>> + ret = srf08_read_ranging(data);
>>> + if (ret < 0)
>>> + return ret;
>>> + *val = ret;
>>> + return IIO_VAL_INT;
>>> + case IIO_CHAN_INFO_SCALE:
>>> + /* 1 LSB is 1 cm */
>>> + *val = 0;
>>> + *val2 = 10000;
>>> + return IIO_VAL_INT_PLUS_MICRO;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +}
>>> +
>>> +static ssize_t srf08_show_range_mm_available(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + int i, len = 0;
>>> +
>>> + for (i = 0; i < 256; i++)
>>> + len += scnprintf(buf + len, PAGE_SIZE - len,
>>> + "%d ", (i + 1) * 43);
>>> +
>>> + buf[len - 1] = '\n';
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(range_mm_available, S_IRUGO,
>>> + srf08_show_range_mm_available, NULL, 0);
>>> +
>>> +static ssize_t srf08_show_range_mm(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> +
>>> + return sprintf(buf, "%d\n", data->range_mm);
>>> +}
>>> +
>>> +/*
>>> + * set the range of the sensor to an even multiple of 43 mm
>>> + * which corresponds to 1 LSB in the register
>>> + *
>>> + * register value corresponding range
>>> + * 0x00 43 mm
>>> + * 0x01 86 mm
>>> + * 0x02 129 mm
>>> + * ...
>>> + * 0xFF 11008 mm
>>> + */
>>> +static ssize_t srf08_write_range_mm(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> + struct i2c_client *client = data->client;
>>> + int ret;
>>> + unsigned int val, mod;
>>> + u8 regval;
>>> +
>>> + ret = kstrtouint(buf, 10, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + ret = val / 43 - 1;
>>> + mod = val % 43;
>>> +
>>> + if (mod || (ret < 0) || (ret > 255))
>>> + return -EINVAL;
>>> +
>>> + regval = ret;
>>> +
>>> + mutex_lock(&data->lock);
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + SRF08_WRITE_RANGE, regval);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "write_range - err: %d\n", ret);
>>> + mutex_unlock(&data->lock);
>>> + return ret;
>>> + }
>>> +
>>> + data->range_mm = val;
>>> +
>>> + mutex_unlock(&data->lock);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(range_mm, S_IRUGO | S_IWUSR,
>>> + srf08_show_range_mm, srf08_write_range_mm, 0);
>>> +
>>> +static ssize_t srf08_show_gain_available(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + int i, len = 0;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
>>> + len += sprintf(buf + len, "%d ", srf08_gain[i]);
>>> +
>>> + len += sprintf(buf + len, "\n");
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(gain_available, S_IRUGO,
>>> + srf08_show_gain_available, NULL, 0);
>>> +
>>> +static ssize_t srf08_show_gain(struct device *dev,
>>> + struct device_attribute *attr, char *buf)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> + int len;
>>> +
>>> + len = sprintf(buf, "%d\n", data->gain);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static ssize_t srf08_write_gain(struct device *dev,
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t len)
>>> +{
>>> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>>> + struct srf08_data *data = iio_priv(indio_dev);
>>> + struct i2c_client *client = data->client;
>>> + int ret, i;
>>> + unsigned int val;
>>> + u8 regval;
>>> +
>>> + ret = kstrtouint(buf, 10, &val);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(srf08_gain); i++)
>>> + if (val == srf08_gain[i]) {
>>> + regval = i;
>>> + break;
>>> + }
>>> +
>>> + if (i >= ARRAY_SIZE(srf08_gain))
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&data->lock);
>>> +
>>> + ret = i2c_smbus_write_byte_data(data->client,
>>> + SRF08_WRITE_MAX_GAIN, regval);
>>> + if (ret < 0) {
>>> + dev_err(&client->dev, "write_gain - err: %d\n", ret);
>>> + mutex_unlock(&data->lock);
>>> + return ret;
>>> + }
>>> +
>>> + data->gain = val;
>>> +
>>> + mutex_unlock(&data->lock);
>>> +
>>> + return len;
>>> +}
>>> +
>>> +static IIO_DEVICE_ATTR(gain, S_IRUGO | S_IWUSR,
>>> + srf08_show_gain, srf08_write_gain, 0);
>>> +
>>> +static struct attribute *srf08_attributes[] = {
>>> + &iio_dev_attr_range_mm.dev_attr.attr,
>>> + &iio_dev_attr_range_mm_available.dev_attr.attr,
>>> + &iio_dev_attr_gain.dev_attr.attr,
>>> + &iio_dev_attr_gain_available.dev_attr.attr,
>> Hmm. Custom attributes always give us issues. The primary point of IIO
>> is to enforce (more or less) standard interfaces.
>>
>> If you do need to add something new then that is fine (and I do think
>> you need to here!).
>>
>> They need to be formally proposed as an addition to the ABI with
>> docs in /Documentation/ABI/testing/sysfs-bus-iio*
>>
>> Once we take one driver using it it becomes part of our ABI that
>> userspace will need to handle, hence we consider these very
>> carefully.
>>
>> My gut feeling would be that gain needs to be more specific as it's
>> a term that can mean very different things.. Here we are talking
>> about an amplifier on a signal that we are then looking at the timing
>> of. It might otherwise be interpretted as another term for what
>> we term 'scale' in IIO.
>>
>> So what to call it... Perhaps afegain for Analog front end gain?
>> We might want to add this to the core supported attrs, but lets
>> not do so until we see if we have this on a number of devices.
>>
>
> In /Documentation/ABI/testing/sysfs-bus-iio-proximity-as3935 there is also a
> gain used in a similar situation and it's called there "sensor_sensitivity"
>
> What it we also use this name here?
It's not ideal as it's not linked to a particular channel or anything,
but lets go with that as at least we will be consistent between drivers.
>
>> The description would need to make it explicit that this gain is
>> for cases where we aren't measuring the magnitude of what is
>> being amplified.
>>
>> For the range, it's an interesting one. Again the term range could
>> mean too many things within the wider ABI. We need to make it more
>> specific.
>>
>> Actually reading the datasheet, I think this is fundamentally about the
>> maximum sampling frequency rather than directly about the range.
>> The only reason you'd reduce the range is to speed that up. It doesn't
>> improve the resolution, the device simply answers quicker.
>>
>> So I'd support this as sampling_frequency. You could then use
>> the the iio_info_mask_*_available and relevant callback to provide
>> info on what it then restricts the possible output values to
>> (rather than controlling it directly).
>>
>
> By changing the range one cannot influence the sampling frequency directly. I
> have seen on the oszilloscope that the telegrams arrive almost at the same
> time
> with different settings of range and the same gain.
>
> Only if the gain is also adjusted the sensor works faster and a higher
> frequency
> can be used. But the gain is also used to adjust the sensitivity of the
> sensor.
That's rather weird and not what the datasheet suggests. Ah well.
>
> What about calling it "sensor_domain" or "sensor_max_range"?
hmm. Not sure - propose that with appropriate Docs and we can think more on it.
>
>
>>> + NULL,
>>> +};
>>> +
>>> +static const struct attribute_group srf08_attribute_group = {
>>> + .attrs = srf08_attributes,
>>> +};
>>> +
>>> +static const struct iio_chan_spec srf08_channels[] = {
>>> + {
>>> + .type = IIO_DISTANCE,
>>> + .info_mask_separate =
>>> + BIT(IIO_CHAN_INFO_RAW) |
>>> + BIT(IIO_CHAN_INFO_SCALE),
>>> + },
>>> +};
>>> +
>>> +static const struct iio_info srf08_info = {
>>> + .read_raw = srf08_read_raw,
>>> + .attrs = &srf08_attribute_group,
>>> + .driver_module = THIS_MODULE,
>>> +};
>>> +
>>> +static int srf08_probe(struct i2c_client *client,
>>> + const struct i2c_device_id *id)
>>> +{
>>> + struct iio_dev *indio_dev;
>>> + struct srf08_data *data;
>>> +
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_READ_BYTE_DATA |
>>> + I2C_FUNC_SMBUS_WRITE_BYTE_DATA |
>>> + I2C_FUNC_SMBUS_READ_WORD_DATA))
>>> + return -ENODEV;
>>> +
>>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>>> + if (!indio_dev)
>>> + return -ENOMEM;
>>> +
>>> + data = iio_priv(indio_dev);
>>> + i2c_set_clientdata(client, indio_dev);
>>> + data->client = client;
>>> +
>>> + /*
>>> + * set default values of device here
>>> + * these values are already set on the hardware after power on
>>> + */
>>> + data->gain = SRF08_DEFAULT_GAIN;
>>> + data->range_mm = SRF08_DEFAULT_RANGE;
>> We should be a little careful with assumptions about the device having
>> just been powered on. The driver might simply have been removed and
>> reprobed. So I'd sugest rewriting them whatever to be sure we have
>> what we expect. Either that or if they can be read back, then just
>> always retrieve them from the device.
>
> You are right.
> Then i'll set the default value at the sensor, because it cannot be read.
>
>>> +
>>> + indio_dev->name = dev_name(&client->dev);
>>> + indio_dev->dev.parent = &client->dev;
>>> + indio_dev->modes = INDIO_DIRECT_MODE;
>>> + indio_dev->info = &srf08_info;
>>> + indio_dev->channels = srf08_channels;
>>> + indio_dev->num_channels = ARRAY_SIZE(srf08_channels);
>>> +
>>> + mutex_init(&data->lock);
>>> +
>>> + return devm_iio_device_register(&client->dev, indio_dev);
>>> +}
>>> +
>>> +static const struct i2c_device_id srf08_id[] = {
>>> + { "srf08", 0 },
>>> + { }
>>> +};
>>> +MODULE_DEVICE_TABLE(i2c, srf08_id);
>>> +
>>> +static struct i2c_driver srf08_driver = {
>>> + .driver = {
>>> + .name = "srf08",
>>> + },
>>> + .probe = srf08_probe,
>>> + .id_table = srf08_id,
>>> +};
>>> +module_i2c_driver(srf08_driver);
>>> +
>>> +MODULE_AUTHOR("Andreas Klinger <a...@it-klinger.de>");
>>> +MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
>>> +MODULE_LICENSE("GPL");
>>>
>>
>
