linux-kernel - Re: [PATCH v3 9/9] drivers: iio: imu: Add support for adis1657x family

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Sun, 19 May 2024 19:57:07 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Ramona Gradinariu <ramona.bolboaca13@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, conor+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, robh@...nel.org, nuno.sa@...log.com
Subject: Re: [PATCH v3 9/9] drivers: iio: imu: Add support for adis1657x
 family

On Fri, 17 May 2024 10:47:50 +0300
Ramona Gradinariu <ramona.bolboaca13@...il.com> wrote:

> Add support for ADIS1657X family devices in already exiting ADIS16475
> driver.
> 
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@...il.com>

Whilst it's not necessarily vital to support, it I'm curious about
what happens to the hardware timestamp? I thought we had one driver
still doing hardware timestamps directly to the buffer, but I can't
find it so I guess we now deal with alignment in the few devices with
this support.  The st_lsm6dsx has this sort of combining of local
and fifo timestamps for example.

As it stands I think you push the same timestamp for all scans read
from the fifo on a particular watermark interrupt?  That isn't
ideal given we should definitely be able to do better than that.

> +
> +static const struct iio_dev_attr *adis16475_fifo_attributes[] = {
> +	&iio_dev_attr_hwfifo_watermark_min.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_watermark_max.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_watermark.dev_attr.attr,
> +	&iio_dev_attr_hwfifo_enabled.dev_attr.attr,
The autobuilder caught this one.  Drop the dev_attr.attr.

> +	NULL
> +};
> +

> +
> +static const struct iio_buffer_setup_ops adis16475_buffer_ops = {
> +	.postenable = adis16475_buffer_postenable,
> +	.postdisable = adis16475_buffer_postdisable,
> +};
> +
> +static int adis16475_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> +	struct adis16475 *st  = iio_priv(indio_dev);
> +	int ret;
> +	u16 wm_lvl;
> +
> +	adis_dev_lock(&st->adis);

As a follow up perhaps consider defining magic to use guard() for these as there are
enough users that will be simplified to make it worth the effort.	

> +
> +	val = min_t(unsigned int, val, ADIS16575_MAX_FIFO_WM);
> +
> +	wm_lvl = ADIS16575_WM_LVL(val - 1);
> +	ret = __adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, ADIS16575_WM_LVL_MASK, wm_lvl);
> +	if (ret)
> +		goto unlock;
> +
> +	st->fifo_watermark = val;
> +
> +unlock:
> +	adis_dev_unlock(&st->adis);
> +	return ret;
> +}
> +




> @@ -1514,8 +1978,20 @@ static int adis16475_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
> 
> -	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
> -						 adis16475_trigger_handler);
> +	if (st->info->flags & ADIS16475_HAS_FIFO) {
> +		ret = devm_adis_setup_buffer_and_trigger_with_attrs(&st->adis, indio_dev,
> +								    adis16475_trigger_handler_with_fifo,
> +								    &adis16475_buffer_ops,
> +								    adis16475_fifo_attributes);
> +		if (ret)
> +			return ret;
blank line here.

> +		/* Update overflow behavior to always overwrite the oldest sample. */
> +		ret = adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL,
> +				       ADIS16575_OVERFLOW_MASK, (u16)ADIS16575_OVERWRITE_OLDEST);
		if (ret) in here.
> +	} else {
> +		ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
> +							 adis16475_trigger_handler);
and if (ret) in here and drop the one that follows.

otherwise we end up with odd code pattern of handling one case in this scope and not
the other two.

> +	}