drivers:adc-dac:ad5592r: Driver Enhancement #2414

D-Disha · 2025-01-16T11:14:17Z

Added driver apis for adc range, dac range, power down,set internal reference and set adc buffer to the base driver.
Added API to enable busy indicatir on AD5592r
Removed static for spi nop API for visibilty.
Moved some macros from ad5593.c to header file for visibility.

Pull Request Description

Please replace this with a detailed description and motivation of the changes.
You can tick the checkboxes below with an 'x' between square brackets or just check them after publishing the PR.
If this PR contains a breaking change, list dependent PRs and try to push all related PRs at the same time.

PR Type

[] Bug fix (change that fixes an issue)
New feature (change that adds new functionality)
Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

I have followed the Coding style guidelines
I have performed a self-review of the changes
I have commented my code, at least hard-to-understand parts
I have build all projects affected by the changes in this PR
I have tested in hardware affected projects, at the relevant boards
I have signed off all commits from this PR
I have updated the documentation (wiki pages, ReadMe etc), if applies

buha · 2025-01-17T12:47:00Z

drivers/adc-dac/ad5592r/ad5593r.h

+#define AD5593R_MODE_GPIO_READBACK	(6 << 4)
+#define AD5593R_MODE_REG_READBACK	(7 << 4)
+
+#define STOP_BIT	1


tbh i would drop this STOP_BIT, it's pretty clear what it does, no need to have a macro for it

alternatively, prefix it, because at some point there is going to be a name clash

buha · 2025-01-17T12:47:12Z

drivers/adc-dac/ad5592r/ad5593r.h

+#define AD5593R_MODE_REG_READBACK	(7 << 4)
+
+#define STOP_BIT	1
+#define RESTART_BIT	0


same for RESTART_BIT

D-Disha · 2025-01-20T09:40:31Z

Change log: Added power down configuration to init sequence as well. Initialized dev->num_of_channels and ldac mode as it is used by the driver. Corrected typo in commit message

buha · 2025-01-20T10:59:44Z

drivers/adc-dac/ad5592r/ad5592r-base.c

+	if (!dev)
+		return -1;
+
+	ret = ad5592r_base_reg_read(dev, AD5592R_REG_PD, &temp_reg_val);


instead of this read - process - write sequence that you repeat in all these functions you could have a function named ad5592r_base_reg_update which does the sequence once and then you call it however many times needed

Updated with these changes now

buha · 2025-01-22T09:53:24Z

drivers/adc-dac/ad5592r/ad5592r-base.c

+	if (adc_range)
+		status |= AD5592R_REG_CTRL_ADC_RANGE;
+	else
+		status &= ~AD5592R_REG_CTRL_ADC_RANGE;


you can rewrite this whole part more compact as:

uint16_t status = adc_range ? AD5592R_REG_CTRL_ADC_RANGE : 0;

or even ditch the status local variable and put it all inline in the function call, although that's maybe a bit too much

buha · 2025-01-22T09:57:25Z

drivers/adc-dac/ad5592r/ad5592r-base.c

+ * Set ADC Buffer for the device
+ *
+ * @param dev - The device structure.
+ * @param status - Status to enable/disable adc buffer.


i wouldn't name it status, rename it to enable, then it's even more clear what it does

buha · 2025-01-22T09:59:11Z

also, check out cppcheck and astyle CI results

D-Disha · 2025-01-23T04:51:53Z

also, check out cppcheck and astyle CI results

Fixed all the review comments and the checks are passing now

CiprianRegus

Some comments from my side. Also, for future contributions like this one, it would be worth splitting your work into multiple commits, since the changes are not directly related to each other.

CiprianRegus · 2025-01-24T16:01:04Z

drivers/adc-dac/ad5592r/ad5592r.c

+
+	temp_reg_val = enable ? AD5592R_REG_GPIO_OUT_EN_ADC_NOT_BUSY : 0;
+
+	ret = ad5592r_base_reg_update(dev, AD5592R_REG_GPIO_OUT_EN, temp_reg_val,


Return directly

CiprianRegus · 2025-01-24T16:02:22Z

drivers/adc-dac/ad5592r/ad5592r.c

@@ -19,7 +19,7 @@
 *    contributors may be used to endorse or promote products derived from this
 *    software without specific prior written permission.
 *
- * THIS SOFTWARE IS PROVIDED BY ANALOG DEVICES, INC. “AS IS” AND ANY EXPRESS OR
+ * THIS SOFTWARE IS PROVIDED BY ANALOG DEVICES, INC. �AS IS� AND ANY EXPRESS OR


Unrelated change (may have added a white space by mistake).

I copy pasted those lines again from the preexisting files. Not sure what is wrong here. It still shows up in the diff

CiprianRegus · 2025-01-24T16:08:55Z

drivers/adc-dac/ad5592r/ad5592r-base.c

+	if (ret < 0)
+		return ret;
+
+	dev->adc_range = adc_range;


We usually avoid caching register values at the driver level, since the value could be changed by a direct register access. There are some cases where you may want to do that to avoid recurrent register reads, but it doesn't look like it applies here. Is there a reason for which you may want these values to be stored?

The adc and dac range are runtime attributes in the IIO firmware where a user can change the values on the fly. This in turn updates the scale value of the device.

Hence, I considered it best to have it as a device member instead of doing multiple reads.
I have also had it as a part of init_param member as well so the user can easily update the ranges compile time as well if needed.

CiprianRegus · 2025-01-24T16:11:00Z

drivers/adc-dac/ad5592r/ad5593r.c


+	dev = (struct ad5592r_dev *)no_os_malloc(sizeof(*dev));


no_os_calloc()

CiprianRegus · 2025-01-24T16:52:50Z

drivers/adc-dac/ad5592r/ad5592r-base.h

+	enum ad559xr_range adc_range;
+	enum ad559xr_range dac_range;
+	bool adc_buf;
+	uint16_t cached_dac[8];


I'd say having initial values for the DAC output is a bit awkward. There is already ad5592r_write_dac(), so you could use that in the application after ad5592r_init().

cached_dac was present as a device member in the existing no-os drivers already. Having it as a part of init_param as well will help user change the dac output values compile time through our user configuration file.

1. Added driver apis for reg updtae,adc range, dac range, power down,set internal reference and set adc buffer to the base driver. 2. Added API to enable busy indicator on AD5592r 3. Removed static for spi nop API for visibilty. 4. Moved some macros from ad5593.c to header file for visibility. 5. Renamed start and stop macros. Signed-off-by: Disha D <[email protected]>

buha · 2025-01-27T13:30:40Z

As @CiprianRegus said, please split work into multiple commits in the future. The reason is, if one is bad and gets merged, we can revert it and keep the other good ones for users to benefit. So if you feel like listing 5 items in the commit message, you probably have to split the commit into 5 commits.

D-Disha requested review from buha, CiprianRegus, dbogdan, amiclaus, rbolboac and danmois as code owners January 16, 2025 11:14

D-Disha force-pushed the ad559xr-updates branch from b0f99c9 to 35a883d Compare January 17, 2025 05:00

buha reviewed Jan 17, 2025

View reviewed changes

D-Disha force-pushed the ad559xr-updates branch from 35a883d to bee6889 Compare January 20, 2025 09:39

D-Disha requested a review from buha January 20, 2025 09:40

D-Disha force-pushed the ad559xr-updates branch from bee6889 to 7867222 Compare January 20, 2025 09:47

buha reviewed Jan 20, 2025

View reviewed changes

D-Disha force-pushed the ad559xr-updates branch from 7867222 to 12ede21 Compare January 21, 2025 05:29

buha reviewed Jan 22, 2025

View reviewed changes

D-Disha force-pushed the ad559xr-updates branch 3 times, most recently from b195a94 to 0465a0e Compare January 22, 2025 11:27

D-Disha requested a review from buha January 23, 2025 04:52

CiprianRegus reviewed Jan 24, 2025

View reviewed changes

D-Disha force-pushed the ad559xr-updates branch from 0465a0e to b2d8cdd Compare January 27, 2025 09:22

D-Disha force-pushed the ad559xr-updates branch from b2d8cdd to bcc4e0f Compare January 27, 2025 10:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers:adc-dac:ad5592r: Driver Enhancement #2414

drivers:adc-dac:ad5592r: Driver Enhancement #2414

D-Disha commented Jan 16, 2025

buha Jan 17, 2025

buha Jan 17, 2025

D-Disha commented Jan 20, 2025 •

edited

Loading

buha Jan 20, 2025 •

edited

Loading

D-Disha Jan 21, 2025

buha Jan 22, 2025

buha Jan 22, 2025

buha commented Jan 22, 2025

D-Disha commented Jan 23, 2025 •

edited

Loading

CiprianRegus left a comment

CiprianRegus Jan 24, 2025

CiprianRegus Jan 24, 2025

D-Disha Jan 27, 2025

CiprianRegus Jan 24, 2025

D-Disha Jan 27, 2025 •

edited

Loading

CiprianRegus Jan 24, 2025

CiprianRegus Jan 24, 2025 •

edited

Loading

D-Disha Jan 27, 2025 •

edited

Loading

buha commented Jan 27, 2025


		temp_reg_val = enable ? AD5592R_REG_GPIO_OUT_EN_ADC_NOT_BUSY : 0;

		ret = ad5592r_base_reg_update(dev, AD5592R_REG_GPIO_OUT_EN, temp_reg_val,

drivers:adc-dac:ad5592r: Driver Enhancement #2414

Are you sure you want to change the base?

drivers:adc-dac:ad5592r: Driver Enhancement #2414

Conversation

D-Disha commented Jan 16, 2025

Pull Request Description

PR Type

PR Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D-Disha commented Jan 20, 2025 • edited Loading

buha Jan 20, 2025 • edited Loading

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

buha commented Jan 22, 2025

D-Disha commented Jan 23, 2025 • edited Loading

CiprianRegus left a 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

D-Disha Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CiprianRegus Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

D-Disha Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

buha commented Jan 27, 2025

D-Disha commented Jan 20, 2025 •

edited

Loading

buha Jan 20, 2025 •

edited

Loading

D-Disha commented Jan 23, 2025 •

edited

Loading

D-Disha Jan 27, 2025 •

edited

Loading

CiprianRegus Jan 24, 2025 •

edited

Loading

D-Disha Jan 27, 2025 •

edited

Loading