Re: [PATCH 2/2] iio: imu: adis16480: Add the option to enable/disable burst mode

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 4 Aug 2020 13:04:14 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Stefan Popa <stefan.popa@analog.com>
> 
> Although the burst read function does not require a stall time between
> each 16-bit segment, it however requires more processing since the
> software needs to look for the BURST_ID and take into account the offset
> to the first data channel. Some users might find it useful to be able to
> switch between the two modes.

Ah, when you say future patch, you meant extremely near future. :)

> 
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

First rule of new ABI, document it.

As mentioned in previous patch, this sort of user interface is very hard to
use. You are much better off estimating whether it is a good idea or
not for a given set of channels.   If it isn't don't use it, if
it is turn it on.
There is compelling reason to let users choose that I can think of...


Jonathan

> ---
>  drivers/iio/imu/adis16480.c | 48 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
> 
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index 9b100c8fb744..7d7712c33435 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -330,6 +330,45 @@ static int adis16480_debugfs_init(struct iio_dev *indio_dev)
>  
>  #endif
>  
> +static ssize_t adis16495_burst_mode_enable_get(struct device *dev,
> +					       struct device_attribute *attr,
> +					       char *buf)
> +{
> +	struct adis16480 *st = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%d\n", st->adis.burst->en);
> +}
> +
> +static ssize_t adis16495_burst_mode_enable_set(struct device *dev,
> +					       struct device_attribute *attr,
> +					       const char *buf, size_t len)
> +{
> +	struct adis16480 *st = iio_priv(dev_to_iio_dev(dev));
> +	bool val;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &val);
> +	if (ret)
> +		return ret;
> +
> +	st->adis.burst->en = val;
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(burst_mode_enable, 0644,
> +		       adis16495_burst_mode_enable_get,
> +		       adis16495_burst_mode_enable_set, 0);
> +
> +static struct attribute *adis16495_attributes[] = {
> +	&iio_dev_attr_burst_mode_enable.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group adis16495_attribute_group = {
> +	.attrs = adis16495_attributes,
> +};
> +
>  static int adis16480_set_freq(struct iio_dev *indio_dev, int val, int val2)
>  {
>  	struct adis16480 *st = iio_priv(indio_dev);
> @@ -1131,6 +1170,14 @@ static const struct iio_info adis16480_info = {
>  	.debugfs_reg_access = adis_debugfs_reg_access,
>  };
>  
> +static const struct iio_info adis16495_info = {
> +	.attrs = &adis16495_attribute_group,
> +	.read_raw = &adis16480_read_raw,
> +	.write_raw = &adis16480_write_raw,
> +	.update_scan_mode = adis_update_scan_mode,
> +	.debugfs_reg_access = adis_debugfs_reg_access,
> +};
> +
>  static int adis16480_stop_device(struct iio_dev *indio_dev)
>  {
>  	struct adis16480 *st = iio_priv(indio_dev);
> @@ -1365,6 +1412,7 @@ static int adis16480_probe(struct spi_device *spi)
>  	if (st->chip_info->burst) {
>  		st->adis.burst = st->chip_info->burst;
>  		st->adis.burst_extra_len = st->chip_info->burst->extra_len;
> +		indio_dev->info = &adis16495_info;
>  	}
>  
>  	ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev,