Skip to content

Conversation

@dcpleung
Copy link
Member

This updates the PCAL9535A driver to the new GPIO API.

Also, rename the driver to PCA95XX since the driver can support that series of I2C-based GPIO expanders. Kconfig can be used to distinguish between variants.

Tested on the PCA9555 chip on the mec15xxevb_assy6853 board.

@dcpleung
Copy link
Member Author

Relates to #18530

@pabigot pabigot requested a review from galak November 1, 2019 10:25
Copy link
Contributor

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this really needs to be converted to use devicetree instead of Kconfig for all its features, and distinct compatibles used to identify variants, per #19904.

Looks pretty trivial: a standard i2c-device just like semtech,sx1409b but adding a has-pud boolean.

@dcpleung dcpleung requested a review from MaureenHelm as a code owner November 1, 2019 20:28
@dcpleung
Copy link
Member Author

dcpleung commented Nov 1, 2019

Updated to do DTS instead of Kconfigs. However, I cannot have multiple items in the compatible line in the binding, so I simply use "nxp,pca95xx" instead of actual chip model.

Copy link
Contributor

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still focusing on devicetree support here, since I don't have hardware to test this. But can you confirm that your testing showed that both the 1pin and basic GPIO tests pass, as well as blinky and button (if appropriate)?

The native devicetree names should be used rather than the aliases that were necessary in the Kconfig era. I've noted substitutes for the first instance, but they should be applied to all instances.

If there's actually a need for so many instances you might look a drivers/counter/counter_nrfx_rtc.c for an example of a macro that generates all the instance-specific boilerplate just given the instance number.

@pabigot
Copy link
Contributor

pabigot commented Nov 2, 2019

I cannot have multiple items in the compatible line in the binding

True; that'd require something like #20289.

You should be able to do:

compatible = "nxp,pca9555", "nxp,pca95xx";

in the devicetree source, though you'll get warnings from checkpatch until #19904 is settled.

@dcpleung dcpleung force-pushed the gpio_pcal9535_api_new branch from 130c7f3 to f2e49c9 Compare November 2, 2019 21:21
@dcpleung
Copy link
Member Author

dcpleung commented Nov 3, 2019

Updated the driver to use 'DT_INST_*`. As for testing, I have only access to the mec15xx board with PCA9555. This variant does not support pull-up/pull-down so almost all test cases in the 1 pin test are skipped (except the LED toggle which works on the board). The driver does not support any interrupts so the button test would not work. The GPIO basic test, on the other hand, passed with the pull-up/pull-down and interrupt tests skipped.

@pabigot pabigot self-requested a review November 3, 2019 13:52
Copy link
Contributor

@pabigot pabigot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code generally looks OK for the basic operations, but you should look at the sx1509b driver implementation for examples of:

  • locking to prevent corrupting the configuration or output state if two threads try to manipulate the pins or configuration at the same time;
  • checks to block operations from being initiated from within an ISR.

We don't document requirements for atomicity and interrupt-related behavior but for external controllers they can be a serious problem.

Also port-based configuration code can be dropped.

@dcpleung dcpleung force-pushed the gpio_pcal9535_api_new branch from f2e49c9 to 9890d60 Compare November 4, 2019 20:56
@zephyrbot
Copy link

zephyrbot commented Nov 4, 2019

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcpleung thanks for the update. It would be really helpful if you removed redundant usage of access_op.

@dcpleung dcpleung force-pushed the gpio_pcal9535_api_new branch 3 times, most recently from acb33c2 to 2b28844 Compare November 4, 2019 22:58
@dcpleung dcpleung force-pushed the gpio_pcal9535_api_new branch from 2b28844 to cf50384 Compare November 6, 2019 01:43
Copy link
Member

@mnkp mnkp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with a few minor comments.

@dcpleung dcpleung force-pushed the gpio_pcal9535_api_new branch 2 times, most recently from c63d3ae to c45e79c Compare November 10, 2019 10:02
Update driver code and board files to use new GPIO configuration flags
such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver
API as well as gpio_pin_interrupt_configure function.

Signed-off-by: Daniel Leung <[email protected]>
PCA95XX is a series of compatible I2C-based GPIO expanders,
with common registers on input/output, polarity and configuration.
This renames the original PCAL9535A driver to PCA95XX to indicate
that it can support this series. Additional features on variants
are guarded by kconfigs.

Signed-off-by: Daniel Leung <[email protected]>
Use I2C burst write to write 2 bytes to each pair of registers
instead of 2 separate transactions of writing 1 byte.

Signed-off-by: Daniel Leung <[email protected]>
The structures defined in the header file are only used by
the driver source file, and should not be used by others.
So roll the header file into the source file so it won't
get #include.

Signed-off-by: Daniel Leung <[email protected]>
The register pair for each port in the GPIO expander are
port 0 first then port 1. This would not work for big
endian systems with the u16_t port value. So need to
swap the byte ordering on such system.

Signed-off-by: Daniel Leung <[email protected]>
Only update the internal register cache after successful write,
or else it would get out of sync with hardware.

Signed-off-by: Daniel Leung <[email protected]>
Since the GPIO expander is a I2C device, any read/write to
the registers has high latency. Therefore, semaphore is
introduced to prevent multiple threads to manipulate
the GPIOs at the same time.

Also make sure that we are not doing I2C transactions
within ISRs.

Signed-off-by: Daniel Leung <[email protected]>
Remove support for GPIO_ACCESS_BY_PORT in the config function
as configuration by port is going away.

Signed-off-by: Daniel Leung <[email protected]>
This adds the DTS binding for the PCA95xx series I2C-based GPIO
expanders.

Signed-off-by: Daniel Leung <[email protected]>
This converts configuration of the driver to DTS.

Signed-off-by: Daniel Leung <[email protected]>
This factors the common bits of device declaration into macros
so it would be easier to add new instances.

Signed-off-by: Daniel Leung <[email protected]>
There is a GPIO expander connected to I2C0 on board.
This adds the DTS entry.

Signed-off-by: Daniel Leung <[email protected]>
@galak galak force-pushed the gpio_pcal9535_api_new branch from c45e79c to 85661d8 Compare November 13, 2019 18:19
@carlescufi carlescufi merged commit 538fae7 into zephyrproject-rtos:topic-gpio Nov 19, 2019
@dcpleung dcpleung deleted the gpio_pcal9535_api_new branch November 19, 2019 22:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants