adc: adrv902x: adrv9025.c: Tracking cals status #2705

danmois · 2025-01-27T13:16:27Z

PR Description

Add RX_QEC, ORX_QEC, TX_QEC, TX_LOL status IIO attributes.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly (if there is the case)

danmois · 2025-01-27T14:21:50Z

V2

correct commit title

amiclaus · 2025-01-28T12:34:54Z

drivers/iio/adc/adrv902x/adrv9025.c

 	u64 tmask, mask;
 	int val, ret = 0;
+	u32 txchan = 0;


i think explicit initialization is not needed here.

amiclaus · 2025-01-28T12:35:15Z

drivers/iio/adc/adrv902x/adrv9025.c

+
+		adi_adrv9025_RxQecStatus_t rxQecStatus = { 0 };
+
+		if (chan->channel < 4)


magic number

replaced with macro

amiclaus · 2025-01-28T12:35:37Z

drivers/iio/adc/adrv902x/adrv9025.c

+			ret = adi_adrv9025_TrackingCalRxQecStatusGet(phy->madDevice, rxchan, &rxQecStatus);
+		else
+			ret = adi_adrv9025_TrackingCalOrxQecStatusGet(phy->madDevice, rxchan, (adi_adrv9025_OrxQecStatus_t *)&rxQecStatus);
+		if (ret == 0)


if (!ret)

danmois · 2025-01-28T13:17:32Z

V3

implement review changes

nunojsa · 2025-01-28T15:36:01Z

drivers/iio/adc/adrv902x/adrv9025.c

+		case TX_LOL_STATUS:
+			ret = sprintf(buf, "err %d %% %d perf %d iter cnt %d update cnt %d\n", txLolStatus.errorCode, txLolStatus.percentComplete,
+				      txLolStatus.varianceMetric, txLolStatus.iterCount, txLolStatus.updateCount);
+			break;


This is not really how sysfs attrs should be handled. It's a value per file (yes, we do have driver doing the same kind of abuse but we should avoid it). This looks like debug information so I would move it to debugfs.

nunojsa · 2025-01-28T15:37:40Z

drivers/iio/adc/adrv902x/adrv9025.c

+	case RX_QEC_STATUS:
+		rxchan = ADI_ADRV9025_RX1 << chan->channel;
+
+		adi_adrv9025_RxQecStatus_t rxQecStatus = { 0 };


although c11 already allows for mixed code declarations, it is still not an encouraged style in the kernel...

Thank you for the observation. Replaced in the new version.

danmois · 2025-01-29T21:23:49Z

V4

move status read to debugfs

nunojsa · 2025-01-30T08:53:31Z

drivers/iio/adc/adrv902x/adrv9025.c

@@ -1345,13 +1345,63 @@ static const struct iio_info adrv9025_phy_info = {
 	.attrs = &adrv9025_phy_attribute_group,
 };

+static ssize_t adrv9025_rx_qec_status_read(adi_adrv9025_Device_t       *device,
+					   adi_adrv9025_RxChannels_e   rxChannel,
+					   char *buf)


Do not use tabs in here...

I hope I got this right (spaces for the second line and new line for the last).

nunojsa · 2025-01-30T08:59:56Z

drivers/iio/adc/adrv902x/adrv9025.c

+		case DBGFS_TX3_LOL_STATUS:
+			chan = ADI_ADRV9025_TX1 << (entry->cmd - DBGFS_TX0_LOL_STATUS);
+			len = adrv9025_tx_lol_status_read(phy->madDevice, chan, buf);
+			break;


This also does not look good. What if you fail to get the status? Right now you return 0 and then print val (which is 0) to userspace and that value is meaningless. This needs some refactor (at least for the new attrs). Properly return error codes when they happen...

Return error in case status cannot be read.

nunojsa · 2025-01-30T09:00:15Z

drivers/iio/adc/adrv902x/adrv9025.c

+		len = sprintf(buf, "err %d %% %d perf %d iter cnt %d update cnt %d\n", rxQecStatus.errorCode, rxQecStatus.percentComplete,
+			      rxQecStatus.selfcheckIrrDb, rxQecStatus.iterCount, rxQecStatus.updateCount);
+
+	return len;


Flip the. You can get rid of len and the typical pattern is to first look for errors... You're also ignoring errors...

if (ret) return ret; return sprintf(...);

V4

return error in case status cannot be read

reorganize to better handle error cases

danmois · 2025-02-07T07:25:17Z

V5

replace spaces with tabs in function definitions

Add RX_QEC, ORX_QEC, TX_QEC, TX_LOL status IIO attributes. Signed-off-by: George Mois <george.mois@analog.com>

danmois · 2025-02-07T07:38:35Z

V6

fix whitespace errors

nunojsa

Looks much better to me now

danmois requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, CiprianRegus and machschmitt as code owners January 27, 2025 13:16

danmois changed the title ~~adc: adrv902x: adrv9025.c: Tracking calls status~~ adc: adrv902x: adrv9025.c: Tracking cals status Jan 27, 2025

danmois force-pushed the madura_tr_calls branch from b6584bc to 9b859c5 Compare January 27, 2025 14:21

amiclaus reviewed Jan 28, 2025

View reviewed changes

danmois force-pushed the madura_tr_calls branch 2 times, most recently from dcbdd21 to 9c5111a Compare January 28, 2025 13:15

nunojsa reviewed Jan 28, 2025

View reviewed changes

danmois force-pushed the madura_tr_calls branch from 9c5111a to 9a11f26 Compare January 29, 2025 21:21

nunojsa reviewed Jan 30, 2025

View reviewed changes

danmois force-pushed the madura_tr_calls branch 2 times, most recently from f2e9c6e to f674a0e Compare February 7, 2025 07:24

adc: adrv902x: adrv9025.c: Tracking cals status

a63dc02

Add RX_QEC, ORX_QEC, TX_QEC, TX_LOL status IIO attributes. Signed-off-by: George Mois <george.mois@analog.com>

danmois force-pushed the madura_tr_calls branch from f674a0e to a63dc02 Compare February 7, 2025 07:37

nunojsa approved these changes Feb 7, 2025

View reviewed changes

nunojsa merged commit 1c561ce into main Feb 10, 2025
13 checks passed

nunojsa deleted the madura_tr_calls branch February 10, 2025 08:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adc: adrv902x: adrv9025.c: Tracking cals status #2705

adc: adrv902x: adrv9025.c: Tracking cals status #2705

danmois commented Jan 27, 2025 •

edited

Loading

danmois commented Jan 27, 2025

amiclaus Jan 28, 2025

danmois Jan 28, 2025

amiclaus Jan 28, 2025

danmois Jan 28, 2025

amiclaus Jan 28, 2025

danmois Jan 28, 2025

danmois commented Jan 28, 2025

nunojsa Jan 28, 2025

danmois Jan 29, 2025

nunojsa Jan 28, 2025

danmois Jan 29, 2025

danmois commented Jan 29, 2025

nunojsa Jan 30, 2025

danmois Feb 5, 2025

nunojsa Jan 30, 2025

danmois Feb 5, 2025

nunojsa Jan 30, 2025

danmois Feb 5, 2025

danmois commented Feb 7, 2025

danmois commented Feb 7, 2025

nunojsa left a comment


		adi_adrv9025_RxQecStatus_t rxQecStatus = { 0 };

		if (chan->channel < 4)

adc: adrv902x: adrv9025.c: Tracking cals status #2705

adc: adrv902x: adrv9025.c: Tracking cals status #2705

Conversation

danmois commented Jan 27, 2025 • edited Loading

PR Description

PR Type

PR Checklist

danmois commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmois commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmois commented Jan 29, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmois commented Feb 7, 2025

danmois commented Feb 7, 2025

nunojsa left a comment

Choose a reason for hiding this comment

danmois commented Jan 27, 2025 •

edited

Loading