drivers/pca95xx: support for I2C GPIO expanders #9054
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Current progress:
|
|
Reading and writing now works. Verified by multimeter. |
bergzand
added
Type: new feature
|
Interrupts now work when #8951 is applied. |
|
Saul is now working. Everything now seems to be working properly. tests/driver_pca95xxexamples/defaultWith pin set to read (high-Z): With pin set to output (push-pull): With pin set to output (open-drain): |
aabadie
requested changes
May 7, 2018
There was a problem hiding this comment.
I like this one but I think the API should be slightly reworked.
The initialization of the I2C device could be factorized in a single function and GPIO manipulations (default init, init with int) could be handled in separate functions.
I also don't like split between regular GPIO parameters and interrupt pin parameters, only a single one should be used.
Also passing pin and flags in initialization parameters restricts the device use to a single pin, whereas it's able the control multiple ones from a single I2C bus.
What about having the device to manipulate a list of GPIO ?
#define PCA95XX_PARAM_PINS { { .pin = 0, .flags = xx, .int_pin = GPIO_UNDEF},}
#define PCA95XX_PARAMS { .i2c = PCA95XX_PARAM_I2C, \
.addr = PCA95XX_PARAM_ADDR, \
.pins = PCA95XX_PARAM_PINS, }
drivers/include/pca95xx.h
Outdated
| i2c_t i2c; /**< i2c device */ | ||
| uint8_t addr; /**< i2c address */ | ||
| gpio_t int_pin; /**< interrupt pin (GPIO_UNDEF if not connected) */ | ||
| } pca95xx_int_params_t; |
There was a problem hiding this comment.
This parameter struct could be merged with pca95xx_params struct
drivers/include/pca95xx.h
Outdated
| * | ||
| * @return zero on successful initialization, non zero on error | ||
| */ | ||
| int pca95xx_int_init(pca95xx_int_t *dev, const pca95xx_int_params_t *params); |
There was a problem hiding this comment.
I think it's better to pass the callback and related arguments in the initialization function
tests/driver_pca95xx/main.c
Outdated
| return 1; | ||
| } | ||
|
|
||
| pca95xx_dev.params.pin = atoi(argv[1]); |
There was a problem hiding this comment.
normally, the params attribute correspond to initialization parameters and should not updated afterward. Would it make sense to introduce a new function in the API to initialize a specific pin instead of reinitializing the whole device?
tests/driver_pca95xx/main.c
Outdated
|
|
||
| static int init_out(int argc, char **argv) | ||
| { | ||
| pca95xx_dev.params.flags |= PCA95XX_HIGH_DRIVE | PCA95XX_LOW_DRIVE; |
There was a problem hiding this comment.
I would rather introduce a set_flags function
tests/driver_pca95xx/main.c
Outdated
|
|
||
| return 0; | ||
| } | ||
|
|
There was a problem hiding this comment.
blank line could be removed
|
@aabadie A number of the changes you request are things that I am dissatisfied with as well, but cause problems when trying to maintain SAUL compatibility. Combining the pin and interrupt parameters into one struct means that the interrupt parameters are replicated for every pin used by SAUL (up to 16, in this case). Also, placing the pin settings inside params maintains compatibility with SAUL param defines for reads/writes, but is a bit of a hack to modify it in other code. Manipulating multiple pins together would be reasonable on the backend, but would be very cumbersome to use on the CLI with SAUL. I don't quite know how to remove these hacks while maintaining reasonable and compatible behavior. What SAUL really needs is a way of easily manipulating arrays of devices that have identical params except for an index value. |
Yeah, I forgot to put SAUL related things in my initial comment. The actual design in SAUL doesn't fit well with this kind of device I think. What is missing in SAUL is some kind of read/write function with a contextual parameter (could be the pin here). |
|
Perhaps I should do a partial rewrite under the following assumptions:
|
24 tasks
added 9 commits
May 16, 2018 22:10
|
Partial rewrite almost done. Reading and writing are working, but interrupts are not yet functional. |
|
@aabadie My partial rewrite is complete; everything is working; and it is ready for a re-review. Test output (with debugging enabled): |
This was referenced Nov 23, 2018
gschorcht
reviewed
Nov 26, 2018
|
|
||
| /* Get input register */ | ||
| reg_addr = _get_reg(0, DFLAGS, PCA95XX_INPUT_ADDR); | ||
| i2c_read_reg(I2C, ADDR, reg_addr, ®[0]); |
There was a problem hiding this comment.
As I already explained in PR #10430, this I2C read access might collide with an I2C write access to an output port in the case where ports of different expander devices at the same I2C bus are interconnected. This is at least the case for PCF857x I/O expanders. I don't know whether it is the same for PCA95xx expanders, but I would guess yes.
The reason is that an I2C write access to an output port of one device may cause an interrupt by an input port of another device before the I2C write access to the first device has been finished. The handling of the interrupt of the second device requires I2C read access. If the devices are connected to the same I2C bus, the current I2C write access to the first device avoids the required I2C read access from interrupt handler to the second device. The I2C read access fails and the interrupt is lost.
Because the interrupt handling occurs in the same thread context as the I2C write access which is interrupted, the mutex-based i2c_aquire does not help to prevent this conflict.
Even though mutex_lock sees a mutex that is locked it returns with continuation of the thread that has locked the mutex. Unfortunatly, this is the same thread that is in interrupt handling now 😟 That is, it starts to try the I2C read access even though the I2C bus is occupied.
There was a problem hiding this comment.
Interesting problem! Let's find a solution in #10430, so that we are not splitting the discussion. I will then apply a similar fix here.
5 tasks
|
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. |
stale
bot
added
the
State: stale
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This adds support for a variety of I2C GPIO expanders.
Specific parts:
PCA9534, PCA9535, PCA9536, PCA9538, PCA9539, PCA9554, PCA9555, PCA9957, and variants such as PCA6408 and PCA6416, and TCA versions of these components. Probably some others that I didn't see on Digikey. Most of these are also available from multiple manufacturers. I am using the TCA9539.
Todo:
Full testing, fixes, and polish.