Received-SPF: pass (google.com: domain of linux-kernel+bounces-183214-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 19 May 2024 19:57:07 +0100
From: Jonathan Cameron <jic23@kernel.org>
To: Ramona Gradinariu <ramona.bolboaca13@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
 devicetree@vger.kernel.org, conor+dt@kernel.org,
 krzysztof.kozlowski+dt@linaro.org, robh@kernel.org, nuno.sa@analog.com
Subject: Re: [PATCH v3 9/9] drivers: iio: imu: Add support for adis1657x
 family
Message-ID: <20240519195707.71163f84@jic23-huawei>
In-Reply-To: <20240517074750.87376-10-ramona.bolboaca13@gmail.com>
References: <20240517074750.87376-1-ramona.bolboaca13@gmail.com>
	<20240517074750.87376-10-ramona.bolboaca13@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

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

> Add support for ADIS1657X family devices in already exiting ADIS16475
> driver.
> 
> Signed-off-by: Ramona Gradinariu <ramona.bolboaca13@gmail.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.

> +	}