Skip to content

drivers: dac: ad3552r : Add README documentation for AD3552R #2535

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

drivers: dac: ad3552r : Add README documentation for AD3552R #2535

wants to merge 1 commit into from

Conversation

analogcay
Copy link
Contributor

@analogcay analogcay commented Apr 8, 2025 •
edited
Loading

Pull Request Description

This is to add README documentation for AD3552R

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 complied with the Submission Checklist
  • 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

@CLAassistant
Copy link

CLAassistant commented Apr 8, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

buha
buha reviewed Apr 8, 2025
View reviewed changes
drivers/dac/ad3552r/README.rst Outdated
static struct ad3552r_init_param default_ad3552r_param = {
.chip_id = AD3552R_ID,
.spi_param = {
.device_id = SPI_DEVICE_ID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please expand these macros, this SPI_DEVICE_ID or SPI_EXTRA are app defined macros, so give an actual number or real example for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello Darius, we tried looking for the correct values for .chip_id but we cant find it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i would modify the line to something like this:

 .device_id = 1, // corresponds to SPI1

buha
buha reviewed Apr 8, 2025
View reviewed changes
drivers/dac/ad3552r/README.rst Outdated
#include "ad3552r.h"

static struct ad3552r_init_param default_ad3552r_param = {
.chip_id = AD3552R_ID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, put an actual value

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also same concern as above. Cannot find correct values for .chip_id

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pick any one you want from here for the example:

no-OS/drivers/dac/ad3552r/ad3552r.h

Line 163 in 2ab8488

enum ad3552r_id {

amiclaus
amiclaus reviewed Apr 29, 2025
View reviewed changes
Copy link
Contributor

@amiclaus amiclaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please drop the merge commit

@analogcay analogcay force-pushed the Driver-AD3552R-README branch from 4f92295 to 81824b4 Compare May 7, 2025 02:07
amiclaus
amiclaus reviewed May 9, 2025
View reviewed changes
Copy link
Contributor

@amiclaus amiclaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please squash the two commits and rebase with main branch. there are some other strange changes included in this PR.

@analogcay analogcay force-pushed the Driver-AD3552R-README branch from 81824b4 to b91a917 Compare May 14, 2025 06:54
@analogcay
Copy link
Contributor Author

please squash the two commits and rebase with main branch. there are some other strange changes included in this PR.

This was done, kindly check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@buha buha buha left review comments

@amiclaus amiclaus amiclaus left review comments

@CiprianRegus CiprianRegus Awaiting requested review from CiprianRegus CiprianRegus is a code owner

@dbogdan dbogdan Awaiting requested review from dbogdan dbogdan is a code owner

@rbolboac rbolboac Awaiting requested review from rbolboac rbolboac is a code owner

@danmois danmois Awaiting requested review from danmois danmois is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@analogcay @CLAassistant @buha @amiclaus