Skip to content
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

drivers: dac: add max22017 support #76017

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

drivers: dac: add max22017 support #76017

wants to merge 3 commits into from

Conversation

granquet
Copy link

This series add support for the MAX22017 DAC.

The MAX22017 is a two-channel industrial-grade software-configurable analog output device that can be used in either voltage or current output mode.

The DAC also has support for six configurable GPIO.

@github-actions GitHub Actions
Copy link

Hello @granquet, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

@carlocaione carlocaione self-requested a review July 17, 2024 16:09
@granquet granquet marked this pull request as ready for review July 18, 2024 11:08
@zephyrbot zephyrbot added area: DAC Digital-to-Analog Converter platform: ADI Analog Devices, Inc. area: GPIO labels Jul 18, 2024
@granquet granquet marked this pull request as draft July 18, 2024 11:34
@henrikbrixandersen henrikbrixandersen removed their request for review August 20, 2024 07:48
@granquet granquet marked this pull request as ready for review September 5, 2024 15:04
@zephyrbot zephyrbot requested review from ozersa and ttmut September 5, 2024 15:04
martinjaeger
martinjaeger reviewed Sep 9, 2024
View reviewed changes
Copy link
Member

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

Thanks @granquet for your PR and sorry for the late response. It got lost in my notifications for some reason.

As far as I can see, the DAC, ADC and GPIO features of this device are independent of each other. Can you please split out the GPIO driver from this PR and create a dedicated PR for it? This will help with reviews, as the GPIO and DAC drivers are maintained by different persons.

I will do a thorough review once the GPIO sections are taken out.

drivers/dac/Kconfig.max22017 Outdated Show resolved Hide resolved
MaureenHelm
MaureenHelm requested changes Sep 9, 2024
View reviewed changes
Copy link
Member

@MaureenHelm MaureenHelm left a comment

Choose a reason for hiding this comment

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

Need to add a node with this compatible to tests/drivers/build_all/dac/app.overlay

drivers/dac/dac_max22017.c Outdated Show resolved Hide resolved
ttmut
ttmut reviewed Sep 9, 2024
View reviewed changes
drivers/dac/dac_max22017.c Outdated Show resolved Hide resolved
@granquet granquet force-pushed the adi/dac/max22017 branch 4 times, most recently from 88663dd to 4f674c2 Compare September 11, 2024 09:46
@granquet granquet dismissed stale reviews from martinjaeger and ttmut via dd23c39 October 15, 2024 13:52
@granquet
Copy link
Author

Design changed to an MFD to accomodate the GPIO and DAC functions of the device as per requested in the previous (now closed PR #78898 ) GPIO PR.

martinjaeger
martinjaeger requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@martinjaeger martinjaeger left a comment

Choose a reason for hiding this comment

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

Thanks for your patience and the rework as a MFD.

Please find some minor comments below.

I tried to find a datasheet for the MAX22017, but couldn't find anything. Do you have a link?

tests/drivers/build_all/gpio/app.overlay Outdated Show resolved Hide resolved
tests/drivers/build_all/gpio/app.overlay Outdated Show resolved Hide resolved
dts/bindings/mfd/adi,max22017.yaml Outdated Show resolved Hide resolved
drivers/gpio/Kconfig Outdated Show resolved Hide resolved
drivers/gpio/Kconfig.adi_max22017 Outdated

config GPIO_MAX22017_INT_QUIRK
bool "MAX22017 GPIO Interrupt quirk"
default n
Copy link
Member

Choose a reason for hiding this comment

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

As far as I understand, the driver doesn't work properly without the quirk, correct? If that's the case, should it not be default y? Otherwise, if you prefer to keep default n, this line should be deleted, as n is always the default.

Copy link
Author

@granquet granquet Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think it mostly comes from my testing setup as I don't have a clean debounced input to try it... so any attempt at testing this out generates a storm of interrupts.
it "works" but sometimes can get stuck with a pending interrupt to read.
"polling" the interrupt register is the only way I have found to get out of that "stuck" situation.

maybe someone with a clean input signal will never reproduce the issues I'm seeing? that's what I'm hoping for tbh, and is the rationale for the quirk beeing disabled by default.

@granquet
Copy link
Author

Thanks for your patience and the rework as a MFD.

Please find some minor comments below.

I tried to find a datasheet for the MAX22017, but couldn't find anything. Do you have a link?

I have access to a "preliminary" datasheet that I've been provided by ADI directly.
I'm not able to find something public either.
@MaureenHelm , @ttmut : any pointers to a public datasheet ?

@granquet granquet force-pushed the adi/dac/max22017 branch 2 times, most recently from 5489f5c to b1ef23b Compare October 16, 2024 12:12
@ttmut
Copy link
Collaborator

ttmut commented Oct 23, 2024

Thanks for your patience and the rework as a MFD.
Please find some minor comments below.
I tried to find a datasheet for the MAX22017, but couldn't find anything. Do you have a link?

I have access to a "preliminary" datasheet that I've been provided by ADI directly. I'm not able to find something public either. @MaureenHelm , @ttmut : any pointers to a public datasheet ?

It does not seem to be publicly available, sadly.

@mmahadevan108
Copy link
Collaborator

@granquet , we are only merging bug fixes and documentation updates for the 4.0 release. I am changing the milestone to 4.1

@mmahadevan108 mmahadevan108 modified the milestones: v4.0.0, v4.1.0 Oct 30, 2024
@kartben
Copy link
Collaborator

kartben commented Dec 20, 2024

@granquet this would need to be rebased on main as it has merge conflicts

@granquet
drivers: mfd: add MAX22017 DAC/GPIO MFD
2e70ed8 
The MAX22017 DAC provides two 16 Channel Analog outputs and 6 GPIOs.

Signed-off-by: Guillaume Ranquet <[email protected]>
@granquet
drivers: dac: Add support for MAX22017 DAC
5d4ddc1 
The MAX22017 is a two-channel industrial-grade software-configurable
analog output device that can be used in either voltage or current output
mode.

Signed-off-by: Guillaume Ranquet <[email protected]>
@zephyrbot zephyrbot added the area: Samples Samples label Dec 24, 2024
@granquet
Copy link
Author

@granquet this would need to be rebased on main as it has merge conflicts

Thx for notifying me :)

@zephyrbot zephyrbot requested review from kartben and nashif December 24, 2024 23:37
@kartben
Copy link
Collaborator

kartben commented Dec 25, 2024

@granquet this would need to be rebased on main as it has merge conflicts

Thx for notifying me :)

Np :) you also have "interesting" commits in this PR that look like they don't want to be there :)

@granquet
drivers: gpio: add MAX22017 gpio support
8d03ec1 
MAX22017 is a DAC with support for 6 GPIOs

Signed-off-by: Guillaume Ranquet <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlocaione carlocaione Awaiting requested review from carlocaione

@galak galak Awaiting requested review from galak

@microbuilder microbuilder Awaiting requested review from microbuilder

@mnkp mnkp Awaiting requested review from mnkp

@ozersa ozersa Awaiting requested review from ozersa

@aasinclair aasinclair Awaiting requested review from aasinclair

@gmarull gmarull Awaiting requested review from gmarull

@martinjaeger martinjaeger Awaiting requested review from martinjaeger

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@ttmut ttmut Awaiting requested review from ttmut

@kartben kartben Awaiting requested review from kartben

@nashif nashif Awaiting requested review from nashif

Requested changes must be addressed to merge this pull request.

Assignees

@martinjaeger martinjaeger

Labels
area: DAC Digital-to-Analog Converter area: GPIO area: MFD area: Samples Samples platform: ADI Analog Devices, Inc.
Projects
None yet
Milestone
v4.1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@granquet @ttmut @MaureenHelm @mmahadevan108 @kartben @martinjaeger @zephyrbot