Skip to content

Conversation

@soburi
Copy link
Member

@soburi soburi commented Oct 20, 2024

Add support for Adafruit AW9523 GPIO Expander/LED Controller

I have checked this PR with nulceo_h503rb using the equivalent board as a circuit (https://www.waveshare.com/wiki/AW9523B_IO_Expansion_Board) to Adafruit AW9523.

@soburi soburi added the DNM This PR should not be merged (Do Not Merge) label Oct 20, 2024
@soburi soburi marked this pull request as draft October 20, 2024 17:22
@soburi soburi force-pushed the awinic_aw9523b branch 2 times, most recently from b0fe39e to 5f6de7a Compare November 2, 2024 07:42
@soburi soburi changed the title boards: shields: Add support for Adafruit AW9523 board boards: shields: Add support for Adafruit AW9523B board Nov 4, 2024
@soburi soburi changed the title boards: shields: Add support for Adafruit AW9523B board boards: shields: Add support for Adafruit AW9523 board Nov 4, 2024
@soburi soburi force-pushed the awinic_aw9523b branch 3 times, most recently from ea2446a to 5066d2c Compare November 4, 2024 08:34
@soburi soburi removed the DNM This PR should not be merged (Do Not Merge) label Nov 4, 2024
@soburi soburi added this to the v4.1.0 milestone Nov 4, 2024
@soburi soburi force-pushed the awinic_aw9523b branch 3 times, most recently from 5bd20ee to e282c3f Compare November 4, 2024 11:15
@soburi soburi marked this pull request as ready for review November 4, 2024 12:59
Copy link
Contributor

@kartben kartben left a comment

Choose a reason for hiding this comment

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

Haven't reviewed the driver yet

Comment on lines 8 to 9
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate you taking the time to document this "convenience shortcut" you took but I think you would actually need to define a proper connector + conventional aliases for Qwiic since otherwise this will be problematic for people trying to use this shield on board that has both types of connectors IMO

Copy link
Member Author

Choose a reason for hiding this comment

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

I wholly revisited the description of the connector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Use webp format please. Bonus points if you can find a version with transparent background (not mandatory, but nice to have)

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the format to webp. I think the photos on the developer's page have the least copyright issues to quote, but unfortunately there were no photos with a white background here. Therefore, I did not change this.

Comment on lines 47 to 48
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This shield can only be used with a board which provides a configuration for
STEMMA QT/Qwiic or Grove.
This shield can only be used with a board which provides a configuration for
STEMMA QT/Qwiic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed it.

Copy link
Contributor

Choose a reason for hiding this comment

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

What is this supposed to mean? Maybe a copy paste error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that is correct.

@fabiobaltieri
Copy link
Member

@erwango can you take a look at the shield part of this?

@soburi soburi force-pushed the awinic_aw9523b branch 2 times, most recently from 89152c8 to c0d8a8c Compare December 3, 2024 12:57
@soburi soburi requested a review from fabiobaltieri December 3, 2024 13:35
@soburi soburi force-pushed the awinic_aw9523b branch 2 times, most recently from 176202d to 17be358 Compare December 4, 2024 06:32
Copy link
Member

@fabiobaltieri fabiobaltieri left a comment

Choose a reason for hiding this comment

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

good stuff, few more comments

Comment on lines 475 to 484
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need to generate this, can you do

static __maybe_unused void gpio_aw9523b_int_handler(const struct device *gpio_dev,
                                   struct gpio_callback *cb,
                                   uint32_t pins)
{       
        struct gpio_aw9523b_data *data = CONTAINER_OF(
                        cb, struct gpio_aw9523b_data, gpio_callback); 
                
        k_work_submit(&data->intr_worker);
}

instead? (copied it from input_paw32xx.c)

Copy link
Member Author

Choose a reason for hiding this comment

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

I worried about whether __maybe_unused can be used. It is standardized in C23, not C99.
If I can use it, it is better to use it.

Copy link
Member

@fabiobaltieri fabiobaltieri Dec 4, 2024

Choose a reason for hiding this comment

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

It's pretty widely used in the code base, I wouldn't worry about that, just use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I got it. I applied your suggestion.

Copy link
Member

@fabiobaltieri fabiobaltieri Dec 4, 2024

Choose a reason for hiding this comment

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

ok I see you added it on other callbacks but would you not use it to replace gpio_aw9523b_int_cb##inst with a common one as well? that was my main suggestion, this does not have to be generated, nothing wrong with it but it seems like unnecessary extra complexity

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I misunderstood what you said.
This is better. I've addressed it.

@soburi soburi force-pushed the awinic_aw9523b branch 3 times, most recently from ddf6b2c to c20cd23 Compare December 4, 2024 16:15
fabiobaltieri
fabiobaltieri previously approved these changes Dec 4, 2024
soburi and others added 6 commits December 5, 2024 06:31
Add a definition of a STEMMA QT connector to which I2C
devices can be connected.

Since STEMMA-QT does not specify uses other than I2C,
this is just a formal definition.
(It may be helpful when using this connector as a GPIO,
 which is not defined in STEMMA-QT)

Maybe in usual need to define i2c alias such like as,

```
stemma_qt_i2c: &i2c0 { }
```

Signed-off-by: TOKITA Hiroshi <[email protected]>
Co-authored-by: Benjamin Cabé <[email protected]>
Add support for GPIO controller feature of AW9523B.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Co-authored-by: Benjamin Cabé <[email protected]>
Add support for Adafruit AW9523 GPIO Expander/LED Controller

Co-authored-by: Benjamin Cabé <[email protected]>
Signed-off-by: TOKITA Hiroshi <[email protected]>
Add test configuration for Awinic AW9523B.

This device is a GPIO Expander, and no boards currently implement it.
Define the connection that connects it via the Arduino header and
define a fixture to enable it.

This has been confirmed with nucleo_h503rb.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add test configuration for Awinic AW9523B.

This device is a GPIO Expander, and no boards currently implement it.
Define the connection that connects it via the Arduino header and
define a fixture to enable it.

This has been confirmed with nucleo_h503rb.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add test configuration for "awinic,aw9523b".

Signed-off-by: TOKITA Hiroshi <[email protected]>
@kartben kartben merged commit 2953c38 into zephyrproject-rtos:main Dec 5, 2024
24 checks passed
@soburi soburi deleted the awinic_aw9523b branch December 5, 2024 06:59
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.

7 participants