Message-ID: <20240428162555.3ddf31ea@jic23-huawei>
Date: Sun, 28 Apr 2024 16:25:55 +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,
 linux-doc@...r.kernel.org, devicetree@...r.kernel.org, corbet@....net,
 conor+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org, robh@...nel.org,
 Ramona Gradinariu <ramona.gradinariu@...log.com>, Nuno Sá
 <nuno.sa@...log.com>
Subject: Re: [PATCH 4/5] iio: adis16480: add support for adis16545/7
 families

On Tue, 23 Apr 2024 11:42:09 +0300
Ramona Gradinariu <ramona.bolboaca13@...il.com> wrote:

> The ADIS16545 and ADIS16547 are a complete inertial system that
> includes a triaxis gyroscope and a triaxis accelerometer.
> The serial peripheral interface (SPI) and register structure provide a
> simple interface for data collection and configuration control.
> 
> These devices are similar to the ones already supported in the driver,
> with changes in the scales, timings and the max spi speed in burst
> mode.
> Also, they support delta angle and delta velocity readings in burst
> mode, for which support was added in the trigger handler.
> 
> Signed-off-by: Nuno Sá <nuno.sa@...log.com>

What is Nuno's relationship to this patch?  You are author and the sender
which is fine, but in that case you need to make Nuno's involvement explicit.
Perhaps a Co-developed-by or similar is appropriate?

> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@...log.com>
A few comments inline.  Biggest one is I'd like a clear statement of why you
can't do a burst of one type, then a burst of other.  My guess is that the
transition is very time consuming?  If so, that is fine, but you should be able
to let available_scan_masks handle the disjoint channel sets.

From what I recall a similar case was why that got added in the first place.
The uses in optimizing for devices that 'want' to grab lots of channels was
a later use case.

Jonathan



>  
>  static bool adis16480_validate_crc(const u16 *buf, const u8 n_elem, const u32 crc)
> @@ -1200,7 +1355,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  	struct adis16480 *st = iio_priv(indio_dev);
>  	struct adis *adis = &st->adis;
>  	struct device *dev = &adis->spi->dev;
> -	int ret, bit, offset, i = 0;
> +	int ret, bit, offset, i = 0, buff_offset = 0;
>  	__be16 *buffer;
>  	u32 crc;
>  	bool valid;
> @@ -1233,8 +1388,8 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  	 * 16-bit responses containing the BURST_ID depending on the sclk. If
>  	 * clk > 3.6MHz, then we will have two BURST_ID in a row. If clk < 3MHZ,
>  	 * we have only one. To manage that variation, we use the transition from the
> -	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5. If
> -	 * we not find this variation in the first 4 segments, then the data should
> +	 * BURST_ID to the SYS_E_FLAG register, which will not be equal to 0xA5A5/0xC3C3.
> +	 * If we not find this variation in the first 4 segments, then the data should
>  	 * not be valid.
>  	 */
>  	buffer = adis->buffer;
> @@ -1242,7 +1397,7 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  		u16 curr = be16_to_cpu(buffer[offset]);
>  		u16 next = be16_to_cpu(buffer[offset + 1]);
>  
> -		if (curr == ADIS16495_BURST_ID && next != ADIS16495_BURST_ID) {
> +		if (curr == st->burst_id && next != st->burst_id) {
>  			offset++;
>  			break;
>  		}
> @@ -1269,11 +1424,22 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  		switch (bit) {
>  		case ADIS16480_SCAN_TEMP:
>  			st->data[i++] = buffer[offset + 1];
> +			/*
> +			 * The temperature channel has 16-bit storage size.
> +			 * We need to perform the padding to have the buffer
> +			 * elements naturally aligned in case there are any
> +			 * 32-bit storage size channels enabled which have a
> +			 * scan index higher than the temperature channel scan
> +			 * index.
> +			 */
> +			if (*indio_dev->active_scan_mask &
> +			    GENMASK(ADIS16480_SCAN_DELTVEL_Z, ADIS16480_SCAN_DELTANG_X))
> +				st->data[i++] = 0;

I think it is harmless to always do this. If there is no data after this channel
then we are writing data that won't be copied anywhere. If there is data after
it is needed.

So I think you can drop the condition but do add a comment on it being necessary
if there is data afterwards and harmless if there isn't.
	

>  			break;
>  		case ADIS16480_SCAN_GYRO_X ... ADIS16480_SCAN_ACCEL_Z:
>  			/* The lower register data is sequenced first */
> -			st->data[i++] = buffer[2 * bit + offset + 3];
> -			st->data[i++] = buffer[2 * bit + offset + 2];
> +			st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 3];
> +			st->data[i++] = buffer[2 * (bit - buff_offset) + offset + 2];
>  			break;
>  		}
>  	}
> @@ -1285,10 +1451,38 @@ static irqreturn_t adis16480_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static int adis16480_update_scan_mode(struct iio_dev *indio_dev,
> +				      const unsigned long *scan_mask)
> +{
> +	u16 en;
> +	int ret;
> +	struct adis16480 *st = iio_priv(indio_dev);
> +
> +	if (st->chip_info->has_burst_delta_data) {
> +		if ((*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) &&
> +		    (*scan_mask & ADIS16545_BURST_DATA_SEL_1_CHN_MASK))
> +			return -EINVAL;

Use available scan_masks to enforce this.  That can have mutually exclusive
sets like you have here.

However, what stops 2 burst reads - so do them one after the other if needed.
You'd still want available_scan_masks to have the subsets though but added
to that the combination of the two.

> +		if (*scan_mask & ADIS16545_BURST_DATA_SEL_0_CHN_MASK) {
> +			en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 0);
> +			st->burst_id = 0xA5A5;

Give the magic value a name via a define.

> +		} else {
> +			en = FIELD_PREP(ADIS16545_BURST_DATA_SEL_MASK, 1);
> +			st->burst_id = 0xC3C3;
> +		}
> +
> +		ret = __adis_update_bits(&st->adis, ADIS16480_REG_CONFIG,
> +					 ADIS16545_BURST_DATA_SEL_MASK, en);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return adis_update_scan_mode(indio_dev, scan_mask);
> +}

>  
> @@ -1498,6 +1692,8 @@ static int adis16480_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> +	st->burst_id = ADIS16495_BURST_ID;

Why this default? Probably wants an explanatory comment.

> +

Powered by blists - more mailing lists