drivers: add driver mcp23017 #7652
Conversation
|
please resolve conflict with |
|
Done, thanks @smlng |
drivers/include/mcp23017.h
Outdated
| }; | ||
|
|
||
| /** | ||
| * @brief Device descriptor for the ADXL345 sensor |
| MCP23017_ADDR_25 = 0x25, /**< I2C device address: A2=1, A1=0, A0=1 */ | ||
| MCP23017_ADDR_26 = 0x26, /**< I2C device address: A2=1, A1=1, A0=0 */ | ||
| MCP23017_ADDR_27 = 0x27, /**< I2C device address: A2=1, A1=1, A0=1 */ | ||
| }; |
There was a problem hiding this comment.
No need to list all these addresses explicitly I'd say. Having these in a comment somewhere should suffice.
| typedef struct { | ||
| gpio_t int_a_pin; /**< interrupt a pin */ | ||
| gpio_t int_b_pin; /**< interrupt b pin */ | ||
| } mcp23017_params_t; |
There was a problem hiding this comment.
This struct is incomplete and inconsistent with the actual usage below. As I see it, it should contain:
typedef struct {
i2c_t i2c_dev;
uint8_t i2c_addr;
gpio_t int_a_pin;
gpio_t int_b_pin;
} mcp23017_params_t;| uint8_t port_b_value; | ||
| uint8_t port_a_inten; | ||
| uint8_t port_b_inten; | ||
| } mcp23017_t; |
There was a problem hiding this comment.
This seems like a lot of wasted RAM. There is no need to keep all these configuration data in RAM. But instead, you should add an array for storing callback functions and arguments, so we allow for registering user defined functions for pin interrupts. The simplest (though also the most resource intensive option) would be to allow for storage of 16 callback functions and 16 arguments, so one tuple for each provided pin. Or maybe better: we only save 2 callbacks and 2 args, and allow only a single active interrupt on port A and one active one on port B.
| * @return MCP23017_OK on success | ||
| * @return MCP23017_NODEV no device found | ||
| */ | ||
| int mcp23017_set_dir(mcp23017_t *dev, uint8_t port, uint8_t pin, uint8_t dir); |
There was a problem hiding this comment.
I would strongly suggest to rework the interface with the goal to make it more similar to the periph/gpio.h driver. As a first step, I think it would make sense to go with the following:
int mcp23017_pin_init(mcp23017_t *dev, uint8_t pin, gpio_mode_t mode);
int mcp23017_pin_init_int(mcp_23017_t *dev, uint8_t pin, gpio_mode_t mode, gpio_flank_t flank, gpio_cb_t cb, void *arg);
int mcp23017_pin_read(mcp23017_t *dev, uint8_t pin);
int mcp23017_pin_set(mcp23017_t *dev, uint8_t pin);
...The pin parameter as used above is used as abstraction for the 16 internal pins, so we simply map it A0 -> 0, A1 -> 1, ..., B0 -> 8, B7 -> 15. This needs of course to be documented in the module and API description for this driver...
As of now, we are not really able to map the gpio interface 1-on-1, so we have to live with some differences (e.g. set/clear/toggle have return values, as the i2c driver could fail).
| @@ -163,6 +163,10 @@ ifneq (,$(filter lpd8808,$(USEMODULE))) | |||
| FEATURES_REQUIRED += periph_gpio | |||
| endif | |||
|
|
|||
| ifneq (,$(filter mcp23017,$(USEMODULE))) | |||
| FEATURES_REQUIRED += periph_i2c | |||
There was a problem hiding this comment.
also FEATURES_REQUIRED += periph_gpio
|
I'll continue this work any time in future and open a new PR then. |
|
@PeterKietzmann I will continue the work. I will do it also to have real implementation for the GPIO extension API in PR #9860 and #9958. I have them here as I2C and SPI version as well. |
This PR includes the driver for MCP23017 - 16-Bit I/O Expander with Serial Interface.
The aconno board ACD52832 includes this IC. #7501 and #7557 are required for testing.