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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200806190208.7d4899e2@archlinux>
Date:   Thu, 6 Aug 2020 19:02:08 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Alexandru Ardelean <alexandru.ardelean@...log.com>
Cc:     <linux-iio@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        Stefan Popa <stefan.popa@...log.com>
Subject: Re: [PATCH 2/2] iio: imu: adis16480: Add the option to
 enable/disable burst mode

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

> From: Stefan Popa <stefan.popa@...log.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@...log.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@...log.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,