-
Notifications
You must be signed in to change notification settings - Fork 7.3k
drivers: adc: Includes MagnTek MT6701 #77801
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: main
Are you sure you want to change the base?
drivers: adc: Includes MagnTek MT6701 #77801
Conversation
Adds support to the Magnetic Angle Sensor. Signed-off-by: Vinicius Miguel <[email protected]>
@uLipe could I have your input on this one? |
Please if this driver is not ready for review, change it to draft. |
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.
LGTM so far, I have some minor nits/questions.
@@ -0,0 +1,12 @@ | |||
# MT6701 Angular position sensor configuration option |
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.
This comment can be dropped, the menuconfig option is already self explanatory
0x37, 0x34, 0x31, 0x32, 0x13, 0x10, 0x15, 0x16, 0x1F, 0x1C, 0x19, | ||
0x1A, 0x0B, 0x08, 0x0D, 0x0E, 0x07, 0x04, 0x01, 0x02}; | ||
|
||
static uint8_t mt6701_crc_check(uint32_t input) |
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.
Zephyr already offers CRC calculators, please use to avoid duplicated code.
int err = spi_read_dt(&dev_cfg->spi_port, &rx); | ||
|
||
/* an invalid reading preserves the last good value */ | ||
if (!err) { |
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.
Zephyr by convention branch for handling error, please invert the condition and branch to perform the error handling.
* [0:13] Angle data | ||
* [14:17] Magnetic field status | ||
* [18:23] CRC | ||
*/ |
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.
Is this comment needed? If so is there a way of creating defines with proper index meaning?
|
||
dev_data->position = 0; | ||
|
||
LOG_INF("Device %s initialized", dev->name); |
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.
Please use LOG_DBG instead, this is the kind of information that are used in debugging scenarios
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 don't think we're using CODEOWNERS anymore. See #38725 (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.
ya it's actually been dropped altogether, now
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
@viniciusmiguel any update here? |
looks like this needs a rebase. But not sure you're still available to work on this, @viniciusmiguel? |
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
@kartben I will be taking this from @viniciusmiguel , I have some units of this sensor for testing. |
This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time. |
Adds support to the Magnetic Angle Sensor MT6701 from MagnTek.
Datasheet for reference:
https://www.lcsc.com/datasheet/lcsc_datasheet_2109011830_Magn-Tek-MT6701CT-STD_C2856764.pdf