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
feat(drivers): Driver for MAX7318 GPIO expander #1295
Conversation
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.
Thanks for working on this!
I don't really agree w/ the need for separate dirs yet for this, IMHO, can you please not mix that in w/ the new driver here?
Other than that, a few suggestions, I imagine mostly stemming from you copying our current out of tree driver that's missing some upstream QOL improvements.
app/drivers/gpio/max7318/Kconfig
Outdated
menuconfig GPIO_MAX7318 | ||
bool "MAX7318 I2C-based GPIO chip" | ||
depends on I2C | ||
select HAS_DTS_GPIO |
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.
See the 595 gpio driver I just merged for an example of how to auto-enable this if found in the DT.
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.
yep, copied it over.
|
||
#define MAX7318_INIT(inst) \ | ||
static struct max7318_config max7318_##inst##_config = { \ | ||
.i2c_dev_name = DT_INST_BUS_LABEL(inst), \ |
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 common stuff should be initialized here, see https://github.com/zmkfirmware/zmk/blob/main/app/drivers/gpio/gpio_595.c#L200 for an example.
Also, Zephyr has newer APIs for the bus stuff, which isn't incorporated into our local mcp23017 driver yet, but should be used for newer drivers. The I2C idea can be seen at https://github.com/zmkfirmware/zmk/blob/main/app/drivers/gpio/gpio_595.c#L205 but there is an equivalent macro and API for SPI.
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 think you meant s/spi/i2c/
. I hope I made the changes correctly (it compiles); will test with my board to make sure it works when I get the time (soon)
uint16_t data = sys_cpu_to_le16(value); | ||
uint16_t dev_addr = ((struct max7318_config *)dev->config)->device_addr; | ||
|
||
return i2c_burst_write(drv_data->i2c, dev_addr, reg, (uint8_t *)&data, sizeof(data)); |
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.
Move to the _dt
version of this.
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.
Done.
uint16_t dev_addr = ((struct max7318_config *)dev->config)->device_addr; | ||
|
||
uint16_t data = 0; | ||
int ret = i2c_burst_read(drv_data->i2c, dev_addr, reg, (uint8_t *)&data, sizeof(data)); |
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.
Ditto, move to the _dt
version of this.
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.
Done.
Addressed the PR comments |
@petejohanson are there any more issues preventing this from being merged? let me know what else to change. thanks! |
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.
Just one minor nitpick, probably keeping this from automatically being enabled if the DTS contains a compatible node.
app/drivers/gpio/Kconfig.max7318
Outdated
# Copyright (c) 2022 The ZMK Contributors | ||
# SPDX-License-Identifier: MIT | ||
|
||
DT_COMPAT_ZMK_GPIO_MAX7318 := zmk,gpio-max7318 |
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 doesn't match the compatible defined in the yaml binding or used in the code. Accident or intentional?
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.
Ah, my bad. The MCP23017 driver doesn't have gpio-
in front, but the 595 does -- which one should I follow here?
Thanks!
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.
Follow the mcp format is fine.
should've fixed them now! |
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.
Just one other nitpick for the naming changes.
Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Co-authored-by: Pete Johanson <peter@peterjohanson.com>
done! |
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.
Thanks!
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Add driver for max7318 i2c gpio expander Co-authored-by: Pete Johanson <peter@peterjohanson.com>
Adds a driver for the MAX7318 16-port I2C GPIO expander. Doesn't support the interrupt feature (yet?).
Has been tested on real hardware.
I've moved the existing mcp23017 driver to its own subfolder as well to keep things a little more organised.