Madura DPD #3016

danmois · 2025-11-13T08:47:31Z

PR Description

Add DPD functionality to the Madura driver.

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 compiled my changes, including the documentation
I have tested the changes on the relevant hardware
[] I have updated the documentation outside this repo accordingly
[] I have provided links for the relevant upstream lore

gastmaier

looked into the ci output to check sanity. the two new warnings seems to be valid, I highlighted with comments.
There are extra warnings outside touched lines, that would be nice to progressively resolve.

In the files changed tab, action summary page, and log file, you can see the ci warnings

gastmaier · 2025-11-13T09:37:13Z

drivers/iio/adc/adrv902x/adrv9025.c


+	ret = sysfs_create_bin_file(&indio_dev->dev.kobj, &phy->bin);
+	if (ret < 0)
+		return ret;


From ci: shouldn't this go to the error de-initialization path?

Yes it should, thank you! I will update the function.

Also do not use the above function. Directly accessing a fundamental thing as kobjects from a driver is a very strong sign of something very odd. Use the device_create_bin_file() wrapper.

gastmaier · 2025-11-13T09:37:31Z

drivers/iio/adc/adrv902x/adrv9025.c

+
+		ret = sscanf(line, "%hhu %hhu %hhu %hhu %s", &i, &j, &k, &lut, coef);
+		if (ret != 5)
+			return ret;


From ci: this is never a error code.

True. I wonder if an error message should be displayed before returning -EINVAL?

Likely yes 😉

danmois · 2025-11-13T13:25:20Z

V2

fix error de-initialization path
display message and return correct value in case of error in adrv9025_parse_dpd_coef
use device_create_bin_file() instead of sysfs_create_bin_file()

gastmaier · 2025-11-14T13:30:36Z

drivers/iio/adc/adrv902x/adrv9025.c

 	return 0;

+out_remove_device_bin:
+	device_remove_bin_file(&phy->indio_dev->dev, &phy->bin);


Shouldn't you remove the bin file at the remove method as well?

Done. Thanks!

vasbimpikasadi · 2025-11-14T14:14:38Z

drivers/iio/adc/adrv902x/adrv9025.c

 				      !!(phy->cal_mask.calMask & val));
 		break;
+	case ADRV9025_CAL_MASK:
+		ret = sprintf(buf, "%d\n",


This may be outside the scope of this PR as it looks like a minor addition, but sysfs_emit() should be used instead of sprintf() for sysfs attributes (proper bounds checking, preferred kernel way. Just a thought, might be a chance to modernise the sysfs methods of the driver.

Hi, Vas! You are right, thank you for the observation. However, I used sprintf to keep the current structure of the driver unchanged. I am also not completely sure that we should include this refactoring in this PR, since the current version was tested as it is. From my point of view, DPD is a fairly complex addition to the driver. Do you have any recommendation regarding this @gastmaier , @nunojsa ?

Yes, I do agree we should modernize it (because copy paste is a fairly common practice for these devices and we do not want to copy old APIs all over).

That said, it needs to be in it's own patch (a precursor patch) and it should be pretty safe to do it even in this PR. I mean, it should be a matter of s/sprintf/sysfs_emit (or using an IDE replace kind of thing).

danmois · 2025-11-17T06:49:15Z

V3

remove bin file at the remove method as well

danmois · 2025-11-17T09:14:14Z

V4

use sysfs_emit instead of sprintf

danmois · 2025-11-17T09:58:44Z

V5

fix status display

danmois · 2025-11-18T07:54:10Z

V6

address CI warnings

gastmaier

LGTM

nunojsa

Some stuff from me

nunojsa · 2025-11-18T09:28:38Z

drivers/iio/adc/adrv902x/adrv9025.c

 					   adi_adrv9025_FramerSel_e framerSel,
 					   u32 *iqRate_kHz)
 {
-	int recoveryAction = ADI_COMMON_ACT_NO_ACTION;


So, is this about checkpatch? If so, I need to complain about the commit message. So the message is pretty much just stating what the subject already says. So if I do not look at the code, I have no idea about the change itself.

Also, the git subject could be better. Example, "Address/Fix coding style issues"

Well, checkpatch does not complain about this, but GihtHub Actions does. I will update the commit title and message, thanks.

nunojsa · 2025-11-18T09:29:48Z

drivers/iio/adc/adrv902x/adrv9025.c

+			 rxQecStatus.percentComplete,
+			 rxQecStatus.selfcheckIrrDb,
+			 rxQecStatus.iterCount,
+			 rxQecStatus.updateCount);


I guess I misunderstood what you meant in teams. Why can't we use sysgs_emit() in here?

sysfs_emit works for sysfs, but not for debugfs.

Yeps, I realized we were talking debugfs later in the review

nunojsa · 2025-11-18T09:30:01Z

drivers/iio/adc/adrv902x/adrv9025.c

+			 txQecStatus.percentComplete,
+			 txQecStatus.correctionMetric,
+			 txQecStatus.iterCount,
+			 txQecStatus.updateCount);


ditto for all other places

nunojsa · 2025-11-18T09:31:54Z

drivers/iio/adc/adrv902x/adrv9025.c


 	if (!len)
-		len = snprintf(buf, sizeof(buf), "%llu\n", val);
+		len = scnprintf(buf, sizeof(buf), "%llu\n", val);


Here it's a valid place where we cannot use it.

nunojsa · 2025-11-18T09:35:38Z

drivers/iio/adc/adrv902x/adrv9025.c

+			mutex_lock(&phy->lock);
+			ret = adi_adrv9025_TxToOrxMappingGet(phy->madDevice,
+							     ADI_ADRV9025_ORX1 << (entry->cmd - DBGFS_ORX1_TO_TX),
+							     (adi_adrv9025_TxChannels_e *)&val);


The above is the typical pattern when this would fall part for big endian architectures. Yes, we do not use them and today everything is mostly little endian but the kernel does support big endian archs and a driver is supposed to work in all archs.

nunojsa · 2025-11-18T09:48:33Z

drivers/iio/adc/adrv902x/adrv9025.c

+	struct adrv9025_rf_phy *phy = iio_priv(indio_dev);
+
+	if (off)
+		return 0;


With memory_read_from_buffer() you do not need to to care about the above condition which is you hacking your way to use sysfs_emit() 😄

nunojsa · 2025-11-18T09:50:01Z

drivers/iio/adc/adrv902x/adrv9025.c

+	}
+
+	phy->dpdModelConfig->txChannelMask = 0;
+	phy->dpdModelConfig->dpdNumFeatures = dpdNumFeatures;


I'm puzzled... Here, you allow to write multiple entries for dpdFeatures but on the read side we just print one? I would say, either don't allow to read it or dump everything

nunojsa · 2025-11-18T09:51:50Z

drivers/iio/adc/adrv902x/adrv9025.c

+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(kobj_to_dev(kobj));
+	struct adrv9025_rf_phy *phy = iio_priv(indio_dev);
+


What is the maximum size this can have? Any idea? If it fits a page (minimum 4096) I would not allow multiple writes on the file. If you force doing it in one go, simplifies things a lot in terms of concurrency. So, if possible I would add this:

https://github.com/analogdevicesinc/linux/blob/main/drivers/iio/adc/navassa/adrv9002.c#L4383

nunojsa · 2025-11-18T09:52:52Z

drivers/iio/adc/adrv902x/adrv9025.c

+
+	while ((line = strsep(&ptr, "\n"))) {
+		if (line >= data + size)
+			break;


It is a simple condition. So you could allow some comments in these files:

https://github.com/analogdevicesinc/linux/blob/main/drivers/iio/adc/navassa/adrv9002.c#L4383

Could you help me out a bit with this comment? I didn’t quite get it. Could you explain it in more detail?

So I imagine you have the coefficients in a text file, right? So someone could want to add some comments on it (maybe in the beginning). Like:

# DPD coefficients for profile foo, ... ...

So # would mark a line comment (you could use something else but this one is already fairly standard) and you could put the coefficients in the other lines.

But this is kind of optional. Up to you... Just mentioned because It's something I did for adrv9002

Ok, will do that, it's a good addition, thanks.

nunojsa · 2025-11-18T09:57:25Z

drivers/iio/adc/adrv902x/adrv9025.c

+			 txDpdStatus.dpdErrorCode,
+			 txDpdStatus.dpdPercentComplete,
+			 txDpdStatus.dpdIterCount,
+			 txDpdStatus.dpdUpdateCount);


Same comment as before. In here you should use scnprintf() together with simple_read_from_buffer()

I might have wrongly said in some comments before that sysfs_emit() could be used. If it's debugfs and we want to print a string like the above, you should use either the above pattern or a seq file

nunojsa · 2025-11-18T10:04:03Z

drivers/iio/adc/adrv902x/adrv9025.c

+	char coef[10];
+	char *line;
+	int ret;
+


Forgot to mention in the review but the it seems we are writing some global device state. It seems a mutex is needed.

danmois · 2025-11-19T10:37:34Z

V7

update commits messages where required
add possibility to consume DPD model files containing comments (marked with '#')
dump DPD model when reading bin attribute
add mutexes to added functions
write masks as hexadecimal values
DPD model as static attribute
make sure the correct RF ORx channel is enabled/has its hardwaregain changed
use sysfs_emit where possible

danmois · 2025-11-19T16:59:56Z

V8

fix dpd_coef_model display (read once)

nunojsa

Still some stuff from me

nunojsa · 2025-11-25T16:06:54Z

drivers/iio/adc/adrv902x/adrv9025.c

 	struct adrv9025_rf_phy *phy = iio_priv(indio_dev);

-	u64 val;
+	u64 val = 0;


The above is a suspicious change. Is this really about coding style?

Actually, I don't know how to categorize it, this is signaled by GitHub actions as a problem.

Created a separate commit for this.

nunojsa · 2025-11-25T16:08:07Z

drivers/iio/adc/adrv902x/adrv9025.c

+			chan_no = chan->channel;
+
+			if (chan_no > CHAN_RX4) {
+				// For observation channels, determine which specific channel is enabled


Not kernel style comment

nunojsa · 2025-11-25T16:09:04Z

drivers/iio/adc/adrv902x/adrv9025.c

+			break;
+
+		if (val <= 0x0F)
+			phy->cal_mask.channelMask = (u8)val;


Do we need the cast? I would assume it's implicit

nunojsa · 2025-11-25T16:10:02Z

drivers/iio/adc/adrv902x/adrv9025.c

+
+	/* force a one write() call as it simplifies things a lot */
+	if (off) {
+		dev_err(&phy->spi->dev, "FH regions must be set in one write() call\n");


Bad copy paste :)

nunojsa · 2025-11-25T16:10:57Z

drivers/iio/adc/adrv902x/adrv9025.c

+		phy->dpdModelConfig->dpdFeatures[dpdNumFeatures].i = i;
+		phy->dpdModelConfig->dpdFeatures[dpdNumFeatures].j = j;
+		phy->dpdModelConfig->dpdFeatures[dpdNumFeatures].k = k;
+		phy->dpdModelConfig->dpdFeatures[dpdNumFeatures].lut = (adi_adrv9025_DpdLut_e)lut;


Don't think we need the cast

nunojsa · 2025-11-25T16:12:08Z

drivers/iio/adc/adrv902x/adrv9025.c

+		phy->dpdModelConfig->dpdFeatures[dpdNumFeatures].coeffReal_xM = 0;
+		phy->dpdModelConfig->dpdFeatures[dpdNumFeatures].coeffImaginary_xM = 0;
+
+		dpdNumFeatures++;


We should likely validate the above iterator for the maximum number of allowed entries

nunojsa · 2025-11-25T16:15:33Z

drivers/iio/adc/adrv902x/adrv9025.c

+	int ret;
+
+	mutex_lock(&phy->lock);
+	if (!off) {


The above does not look correct. See how to use memory_read_from_buffer() as it handles off for you. And it properly return EOF when appropriate.

nunojsa · 2025-11-25T16:16:37Z

drivers/iio/adc/adrv902x/adrv9025.c

 		return -ENOMEM;
+	phy->dpdModelConfig = kzalloc(sizeof(adi_adrv9025_DpdModelConfig_v2_t), GFP_KERNEL);
+	if (!(phy->dpdModelConfig))
+		return -ENOMEM;


Hmm, when is the above memory released?

nunojsa · 2025-11-25T16:19:06Z

drivers/iio/adc/adrv902x/adrv9025.c

 		return -ENOMEM;
+	phy->dpdTrackingConfig = kzalloc(sizeof(adi_adrv9025_DpdTrackingConfig_t), GFP_KERNEL);
+	if (!(phy->dpdTrackingConfig))
+		return -ENOMEM;


Same question about memory? Seems like a leak

danmois · 2025-11-26T08:29:27Z

V9

split first commit so that titles correctly reflect the changes made
fix comment style
remove redundant cast operations
correct error message
validate DPD feature iterator against maximum allowable value
use memory_read_from_buffer() in DPD coefficient read function
solve memory leaks

Remove variables that are not needed, initialize local variables where required, fix spacing, and remove redundant semicolons and blank lines. No functional change. Signed-off-by: George Mois <[email protected]>

Initialize the val local variable to 0. Initialize the rate local variable to 0. Remove redundant blank line. Signed-off-by: George Mois <[email protected]>

Use sysfs_emit instead of sprintf. Use safe functions for formatting buffers. Signed-off-by: George Mois <[email protected]>

Make sure that only one RF channel (ORx1 or ORx2, and ORx3 or ORx4, respectively) is enabled per ORx data path. Signed-off-by: George Mois <[email protected]>

Select the active ORx RF input for modifying the hardwaregain corresponding to one of the two ORx datapaths. Signed-off-by: George Mois <[email protected]>

Add Orx to Tx mapping setting functionality. Signed-off-by: George Mois <[email protected]>

Add external path delay initial calibration selection. Signed-off-by: George Mois <[email protected]>

Add an IIO attribute that allows changing the calibration mask. Signed-off-by: George Mois <[email protected]>

Add a bin_attribute for loading the data structure to hold Tx DPD Model initialization parameters, without floats. Signed-off-by: George Mois <[email protected]>

Add IIO attribute for selecting the target Tx channel for DPD. Signed-off-by: George Mois <[email protected]>

Add IIO attribute for resetting DPD. Signed-off-by: George Mois <[email protected]>

Add DEBUGF attribut for getting DPD status on each of the 4 Tx channels. Signed-off-by: George Mois <[email protected]>

Add tracking configuration settings. Signed-off-by: George Mois <[email protected]>

Add IIO attribute for enabling DPD. Signed-off-by: George Mois <[email protected]>

danmois requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners November 13, 2025 08:47

gastmaier requested changes Nov 13, 2025

View reviewed changes

danmois force-pushed the staging/madura_dpd branch from ddd8c1a to 1457283 Compare November 13, 2025 13:22

gastmaier reviewed Nov 14, 2025

View reviewed changes

vasbimpikasadi reviewed Nov 14, 2025

View reviewed changes

danmois force-pushed the staging/madura_dpd branch from 1457283 to 188092e Compare November 17, 2025 06:43

danmois force-pushed the staging/madura_dpd branch from 188092e to a0111c2 Compare November 17, 2025 09:13

danmois force-pushed the staging/madura_dpd branch from a0111c2 to 243ed4c Compare November 17, 2025 09:58

danmois force-pushed the staging/madura_dpd branch 3 times, most recently from db4f953 to 523346f Compare November 18, 2025 07:53

gastmaier approved these changes Nov 18, 2025

View reviewed changes

nunojsa requested changes Nov 18, 2025

View reviewed changes

nunojsa reviewed Nov 18, 2025

View reviewed changes

danmois force-pushed the staging/madura_dpd branch from 523346f to e9eefdb Compare November 19, 2025 10:25

danmois force-pushed the staging/madura_dpd branch 2 times, most recently from ecf8570 to 02cc5f1 Compare November 19, 2025 16:59

nunojsa reviewed Nov 25, 2025

View reviewed changes

danmois force-pushed the staging/madura_dpd branch 6 times, most recently from ea6da4d to e2c54e3 Compare November 26, 2025 08:23

danmois added 14 commits November 26, 2025 10:38

iio: adc: adrv902x: Address/Fix coding style issues

7362651

Remove variables that are not needed, initialize local variables where required, fix spacing, and remove redundant semicolons and blank lines. No functional change. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Initialize local variables

539b969

Initialize the val local variable to 0. Initialize the rate local variable to 0. Remove redundant blank line. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Use sysfs_emit

d72be15

Use sysfs_emit instead of sprintf. Use safe functions for formatting buffers. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Assure one chan on ORx data path

6e0c8db

Make sure that only one RF channel (ORx1 or ORx2, and ORx3 or ORx4, respectively) is enabled per ORx data path. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: ORx selection for hardwaregain

ba893a7

Select the active ORx RF input for modifying the hardwaregain corresponding to one of the two ORx datapaths. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Add ORx to Tx mapping

8d419df

Add Orx to Tx mapping setting functionality. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Add external path delay calib

cef7028

Add external path delay initial calibration selection. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Change calibration mask

db9dc2a

Add an IIO attribute that allows changing the calibration mask. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Load DPD model config

54bbbdf

Add a bin_attribute for loading the data structure to hold Tx DPD Model initialization parameters, without floats. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Select Tx DPD mask

ac8a9dd

Add IIO attribute for selecting the target Tx channel for DPD. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Enable DPD reset

bf70c23

Add IIO attribute for resetting DPD. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Add DPD status

eefd8a7

Add DEBUGF attribut for getting DPD status on each of the 4 Tx channels. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Add tracking config

74442b9

Add tracking configuration settings. Signed-off-by: George Mois <[email protected]>

iio: adc: adrv902x: Add DPD enable functionality

dce3be3

Add IIO attribute for enabling DPD. Signed-off-by: George Mois <[email protected]>

danmois force-pushed the staging/madura_dpd branch from e2c54e3 to dce3be3 Compare November 26, 2025 08:42

Madura DPD #3016

Are you sure you want to change the base?

Madura DPD #3016

Uh oh!

Conversation

danmois commented Nov 13, 2025

PR Description

PR Type

PR Checklist

Uh oh!

gastmaier left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmois commented Nov 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmois Nov 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danmois commented Nov 17, 2025

Uh oh!

danmois commented Nov 17, 2025

Uh oh!

danmois commented Nov 17, 2025

Uh oh!

danmois commented Nov 18, 2025

Uh oh!

gastmaier left a comment

Choose a reason for hiding this comment

Uh oh!

nunojsa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nunojsa Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gastmaier left a comment •

edited

Loading

danmois Nov 17, 2025 •

edited

Loading

nunojsa Nov 18, 2025 •

edited

Loading

nunojsa Nov 19, 2025 •

edited

Loading

nunojsa Nov 18, 2025 •

edited

Loading