-
Notifications
You must be signed in to change notification settings - Fork 916
Max22007 dev #3015
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.
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
base: rpi-6.12.y
Are you sure you want to change the base?
Max22007 dev #3015
Conversation
4ab9538 to
bd8314f
Compare
gastmaier
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jansunil overall looks good, just some comments from my side.
I believe we can drop the crc feature and really streamline the driver reg access as a consequence.
Best regards,
|
I'll wait this getting out of draft to jump in :) |
bd8314f to
39b2409
Compare
nunojsa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it goes my first round. Couple more points:
- In the bindings patch do not day "...Add MAX22007 DAC bindings". I mean, dont mention "bindings". Its already obvious from the prefix. So instead say something like "Document MAX22007 DAC". Also refactor a bit the message and add a small description of the device.
- Also refactor the driver patch commit message. See git log for other examples. Give a small description of the device. It is also a fairly simple driver so I do not think there is any meaningful to state about the implementation in the commit message.
Regarding process, note that I expect this PR to be opened against main (without the overlay). Then after merged with main, you can open a PR against the PI branch only with the overlay.
ae84b9d to
bb72180
Compare
|
Changelog V2: |
bb72180 to
6e82321
Compare
machschmitt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @jansunil ,
Commenting mostly about the dt docs and proposed ABI. Only superficially looked into the driver code as that might change significantly depending on the discussion about the dt properties and IIO ABI.
| adi,crc-disable: | ||
| type: boolean | ||
| description: | ||
| Disable CRC8 error checking for SPI communications. By default, CRC8 is | ||
| enabled for data integrity verification. Set this property to disable it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see in the data sheet that SPI transfers can run with CRC disabled (set CRC_EN field to 0x0 within the configuration register (0x03) ). For max22007, CRC is a runtime configuration and has nothing to do with how hardware is set up. Drop this property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The property here was added so that it enables the user to take a decision on the state of the CRC and not hardcoding it in the driver. Do you still suggest removing the CRC property and hardcoding it in the driver?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments from me. An additional note regarding commits. Your git subject for the bindings is nok. Please run git log --oneline Documentation/devicetree/bindings/ and follow the sytle.
b39e6d8 to
7d8c82d
Compare
Devicetree bindings for MAX22007 4-channel 12-bit DAC that drives a voltage or current output on each channel Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Add support for the MAX22007 4 channel DAC that drives a voltage or current output on each channel. Signed-off-by: Janani Sunil <janani.sunil@analog.com>
Add documentation for MAX22007 driver which describes how the user can access the driver using dtoverlays Signed-off-by: Janani Sunil <janani.sunil@analog.com>
7d8c82d to
54d7925
Compare
PR Description
necessary to understand them. List any dependencies required for this change.
any space), or simply check them after publishing the PR.
description and try to push all related PRs simultaneously.
PR Type
PR Checklist