drivers: support for Microchip MCP47xx DAC devices added#10518
drivers: support for Microchip MCP47xx DAC devices added#10518benpicco merged 4 commits intoRIOT-OS:masterfrom
Conversation
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
|
Still waiting for another PR. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions. |
1824ce8 to
60184d2
Compare
b248489 to
44f8486
Compare
benpicco
left a comment
There was a problem hiding this comment.
Looks good! Only a few clarifications:
drivers/include/mcp47xx.h
Outdated
| */ | ||
| typedef struct { | ||
| mcp47xx_params_t params; /**< device configuration parameters */ | ||
| uint16_t values[MCP47XX_CHN_NUM_MAX]; /**< contains the last values set */ |
There was a problem hiding this comment.
| uint16_t values[MCP47XX_CHN_NUM_MAX]; /**< contains the last values set */ | |
| uint16_t values[MCP47XX_CHN_NUM_MAX]; /**< contains the last values set for persistence when device is powered off */ |
right?
There was a problem hiding this comment.
Yes, will change it. May I squash directly?
drivers/include/mcp47xx.h
Outdated
| * | ||
| * @retval none | ||
| */ | ||
| void mcp47xx_dac_get(mcp47xx_t *dev, dac_t chn, uint16_t *value); |
There was a problem hiding this comment.
I'm a bit surprised you implemented this when periph/dac API does not provide any dac_get().
Feel free to keep it though :)
drivers/include/mcp47xx.h
Outdated
| typedef struct { | ||
| const char *name; /**< name of the MCP47xx device */ | ||
| unsigned int dev; /**< index of the MCP47xx device */ | ||
| dac_t channel; /**< channel of the MCP47xx device */ |
There was a problem hiding this comment.
Yes, it is the same as for the perhiph/dac. The goal was to keep it as compatible as possible with periph/dac and to handle channels in same way when we intended to introduced the DAC extension API with PR #10512.
There was a problem hiding this comment.
But dac_t would refer to a unique DAC device, not a channel of a DAC.
Just using uint8_t here would be less confusing IMHO (there also is no saul_dac_params_t yet)
There was a problem hiding this comment.
No, dac_t refers a line, that is device + channel. It is also used in periph/dac to set a certain channel of a device. In this sense, it also makes sense to use it here.
There was a problem hiding this comment.
When we started to introduce the extension API, we used exactly the same types for extenders to refer devices and channels.
There was a problem hiding this comment.
Unfortunatly, we had not much luck with the extension interfaces as suggested in PR #10512 til now.
There was a problem hiding this comment.
However, we can of course also use uint8_t, if there is an extension API at some point in future, the type dac_t can still be mapped to dev + chn.
There was a problem hiding this comment.
We can still change it later if we go forward with the extension API. 😉
To an unsuspecting user like me the dac_t type here just adds confusion.
|
Please squash! |
3197bd2 to
739fbf4
Compare
Contribution description
This PR adds a driver which supports different Microchip MCP47xx DAC variants. Multiple MCP47xx DAC devices and different variants can be used at the same time.
The following features of MCP47xx DAC devices are not supported at the moment:
The driver interface is kept as compatible as possible with the peripheral DAC interface. The only differences are that
mcp47xx_andTesting procedure
Please refer the test application in
tests/driver_mcp47xxfor an example on how to use the driverIssues/PRs references