Re: [PATCH v2 2/2] iio: distance: srf08: add IIO driver for us ranger

[Andreas Klinger]

Hi Jonathan,

see comments below.

Andreas


Jonathan Cameron <jic23@xxxxxxxxxx> 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 <ak@xxxxxxxxxxxxx>
> > ---
> >  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 <ak@xxxxxxxxxxxxx>
> > + *
> > + * 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?

> 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. 

What about calling it "sensor_domain" or "sensor_max_range"?


> > +	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 <ak@xxxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("Devantech SRF08 ultrasonic ranger driver");
> > +MODULE_LICENSE("GPL");
> > 
> 

--