Initial implementation of driver

[KajbaM]

Description

This PR provides us with initial experimental implementation of T5838 driver.

Driver is based on existing zephyr NRF PDM driver and is to be used as mentioned driver replacement. Driver functionality should be identical to existing driver, but with added support for handling of T5838 AAD modes and custom PDM sampling frequency on both NRF52 and NRF53 series of chips (original driver does not allow us to use all desired pdm frequency options on nrf52).

Driver does support all T5838 AAD modes, but only AAD A mode was hardware tested (that is the mode we are most interested in). Other modes should work as well, but are not yet tested. See related issues to this PR for more information and concerns that should be addressed in further development of this driver.

Communication with microphone for setting AAD modes is done in bit-banging fashion, as microphone uses some kind of proprietary protocol for it. Replacing original driver was necessary since we need control of PDM CLK pin that is normally controlled by original driver and clock frequency should be configurable beyond what original driver supports on nrf52 platform.

Closes #1

Related #2 #3 #4 #5 #6 #7 #8

Areas of interest for the reviewer

• drivers/t5838/t5838_nrfx_pdm.c - This is the main driver file, it is based on existing zephyr nrf pdm driver with added functionality to use AAD modes and allows us to set custom pdm sampling frequency on nrf52 chips (that is only possible with nrf53 with original driver).
• drivers/t5838/t5838.h - Header file for the drivers added functionality. Inside this file you can find added functionality for AAD modes, dmic compliant API is not changed and should work as with any zephyr compliant dmic implementation.
• samples/dmic/ - This is modified zephyr dmic sample, it is modified to use T5838 driver instead of original nrf pdm driver. Please see boards subdirectory for example how device tree should be configured to use T5838 driver and prj.conf for required kconfig options.
• README.md - See repository README for more information about the driver and how to use it.

Checklist

• My code follows the style guidelines as defined by IRNAS.
• I have performed a self-review of my code.
• My changes generate no new warnings.
• I added/updated source code documentation for all newly added or changed
  functions.
• I updated all customer-facing technical documentation. - Does not yet exist/partially done.

After-review steps

• I will merge PR by myself.

[KajbaM]

Current dependencies on/for this PR:

• main
  • PR Initial implementation of driver #9 👈
    • PR Add trigger sample #11

This stack of pull requests is managed by Graphite.

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[MarkoSagadin]

I have looked only through the t5838.h file. I have marked some specific things, however there are some general things that should be fixed in the entire file.

I didn't yet review yet the t5838_nrfx_pdm.c file. Because the original file (dmic_nrfx_pdm.c) was marked as deleted in the git and a new file was created as a copy, I don't see what were the new changes in the t5838_nrfx_pdm.c file. I just see the new file and I have to do the diff by myself (not really a great way to review stuff).

What you can do to fix that:

1. Soft reset the latest commit.
2. Store the contents of the t5838_nrfx_pdm.c somewhere.
3. Restore the old dmic_nrfx_pdm.c , so that I present in the directory as it was before.
4. Run git mv dmic_nrfx_pdm.c t5838_nrfx_pdm.c
5. Overwrite the contents with the previous backup and commit it.

That way the change will be presented correctly.

---

1. all enum, struct and function names should be lower case (as per our guidelines). Fix this in the header file and all files that use this.
2. All enums, struct should have a doxygen comment in below style:

/**
 * @brief Low pass filter frequency enums for AAD A mode.
 *
 * This settings are interesting for this and that reason, as described in the datasheet, on page XX, etc.
 */

If there are any things related to a specific enumerator or struct member you can write that in a comment above it.

3. A few times you start a doxygen comment with "note". You can use @note instead.
4. I would suggest you to remove t5838_wake_clear function. As discussed IRL, this can be implemented by the application. If multiple interrupt triggers are a concern than I suggest you to implement debounce timer logic. That way the application can still decide if it wants to use the debouncing or not.
5. I would suggest you to split the t5838_configure_AAD into several configure functions, for each AAD mode. Each function then takes a list of parameters (or a specific struct) it needs instead of passing in a config that contains everything.

Please reread the source code documentation and doxygen guidelines sections in our guidelines: https://github.com/IRNAS/irnas-guidelines-docs/blob/main/docs/developer_guidelines.md#coding-standards-
There is some form and general intent that should be followed regiarding the code documentation so that we are all on the same page about what some code does and that the time needed to understand that is minimized as much as possible.

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[KajbaM]

4. I would suggest you to remove t5838_wake_clear function. As discussed IRL, this can be implemented by the application. If multiple interrupt triggers are a concern than I suggest you to implement debounce timer logic. That way the application can still decide if it wants to use the debouncing or not.

I will first verify how interrupt triggering works with other modes of AAD operation before changing this. I will open up an issue regarding this and probably do some more and some other functionality and configuration to interrupt functionality to make it more versatile and also be more intuitive to use.

For now I would like to keep this functionality as is.

5. I would suggest you to split the t5838_configure_AAD into several configure functions, for each AAD mode. Each function then takes a list of parameters (or a specific struct) it needs instead of passing in a config that contains everything.

We could perhaps split this into one function for configuring AAD A and AAD D1/D2 combined (since they are practically same, one with pdm out and one without). I don't really see much benefit from doing this and feel like this might just introduce some confusion in case different modes get used at different points of time in same application.

User might think they can use AAD A and AAD D at the same time, which is not the case. Having same function for configuring both implies that selecting one mode in configuration will turn of the other.

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[MarkoSagadin]

@KajbaM

We could perhaps split this into one function for configuring AAD A and AAD D1/D2 combined (since they are practically same, one with pdm out and one without).

This is actually the preferred solution. Different AAD modes require different sets of parameters. If the driver had several different AAD modes, but all of those would have the same configuration parameters, I would be OK with a single function (it would just take a single enum that represents a AAD mode and configuration struct, or even just the latter).

Since this is not the case here the cleaner interface is a separate function for each AAD mode. Each function configures and enables that mode.

That way the configuration parameters are not contained in a single cfg struct, where user has to guess what is being used and what is redundant (this is my biggest issue with the current interface design).

I don't really see much benefit from doing this and feel like this might just introduce some confusion in case different modes get used at different points of time in same application.

See below example API, I do not think that there is any confusion in regards to what mode is active (if D1 and D2 have the same exact parameters it might make sense to join them).

t5838_aad_a_mode_set(dev, &aad_a_cfg);
/*AAD A mode is active. */

t5838_aad_d1_mode_set(dev, &aad_d1_cfg);
/*AAD D1 mode is active. */

t5838_aad_d2_mode_set(dev, &aad_d2_cfg);
/*AAD D2 mode is active. */

t5838_aad_mode_disable(dev);
/*AAD is disable. */

User might think they can use AAD A and AAD D at the same time, which is not the case. Having same function for configuring both implies that selecting one mode in configuration will turn of the other.

Again, not with the above example design.

[MarkoSagadin]

Went through the documentation part, still didn't get to the source code.

[MarkoSagadin]

Review this line, if it is correct, I don't fully understand it.

[KajbaM]

I do need to further review this, the thing about these two DT parameters is that DTS expects full 4byte reg value if we don't set that for any sub peripheral. I will look further into this.

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[MarkoSagadin]

Requested quite a few things. Please make them.

Also remove the lib and renode_example folders.

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[MarkoSagadin]

Another pass was done, this looks much better now, good job :).
I marked a few things, please fix them.

I also marked some of the things that weren't fixed from the previous review, it seems that they slipped through.
Please address the marked changes first, before doing any refactoring work or at least go through them before you re-request my review.

It takes mental effort and time to go through my previous review and see if you addressed my feedback, either in code or with a comment.

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14

[MarkoSagadin]

I approve this, very good job :)

I marked a minor thing, fix it and you can merge.

[github-actions]

Coverage after merging feature/t5838-driver into main will be

75.00%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
lib/example_lib
example_lib.c	71.43%	50%	100%	75%	10, 14