Skip to content
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

Jump to bottom

I2C get_config API #39006

Merged
merged 4 commits into from Nov 2, 2021
Merged

I2C get_config API #39006

merged 4 commits into from Nov 2, 2021

Conversation

niedzwiecki-dawid
Copy link
Member

No description provided.

@github-actions github-actions bot added area: API Changes to public APIs area: I2C labels Sep 30, 2021
keith-zephyr
keith-zephyr requested changes Sep 30, 2021
View reviewed changes
include/drivers/i2c.h Outdated Show resolved Hide resolved
include/drivers/i2c.h Outdated Show resolved Hide resolved
include/drivers/i2c.h Outdated Show resolved Hide resolved
@de-nordic de-nordic added the Breaking API Change Breaking changes to stable, public APIs label Sep 30, 2021
@de-nordic
Copy link
Collaborator

Shouldn't be there handler provided, in i2c_handlers.c, for use with CONFIG_USERSPACE?

@zephyrbot zephyrbot added the platform: Nuvoton NPCX Nuvoton NPCX label Sep 30, 2021
sjg20
sjg20 requested changes Sep 30, 2021
View reviewed changes
drivers/i2c/Kconfig Outdated Show resolved Hide resolved
include/drivers/i2c.h Show resolved Hide resolved
de-nordic
de-nordic reviewed Sep 30, 2021
View reviewed changes
include/drivers/i2c.h Outdated Show resolved Hide resolved
@de-nordic
Copy link
Collaborator

I am not sure about the api call name: shouldn't it be rather get_configuration, configuration_get, or just configuration?
By looking at the get_config , as e getter,I would expect that the configuration setting function would be called set_config but it is named configure.

@sjg20
Copy link
Collaborator

sjg20 commented Sep 30, 2021

I am not sure about the api call name: shouldn't it be rather get_configuration, configuration_get, or just configuration? By looking at the get_config , as e getter,I would expect that the configuration setting function would be called set_config but it is named configure.

I think it would be better to rename 'configure' to set_config", then

@de-nordic
Copy link
Collaborator

I think it would be better to rename 'configure' to set_config", then

@sjg20 Agree: provide set_config as configure alternative, and set to deprecate the later. Inclusion of get_config is already stable API change, we can go with the other change too.

@niedzwiecki-dawid
Copy link
Member Author

Shouldn't be there handler provided, in i2c_handlers.c, for use with CONFIG_USERSPACE?

Yes, done.

@niedzwiecki-dawid
Copy link
Member Author

I think it would be better to rename 'configure' to set_config", then

@sjg20 Agree: provide set_config as configure alternative, and set to deprecate the later. Inclusion of get_config is already stable API change, we can go with the other change too.

First of all, I don't want to change the configure function because it is naming used by other APIs e.g. uart_configure, i2c_configure, can_configure, etc.

Regarding how to name the new function, I used config_get because a similar function in the UART API is uart_config_get.

@niedzwiecki-dawid
Copy link
Member Author

Is there any way to speed up the review?

@de-nordic
Copy link
Collaborator

@semihalf-niedzwiecki-dawid I think the change requires following https://docs.zephyrproject.org/latest/reference/api/api_lifecycle.html

galak
galak previously requested changes Oct 12, 2021
View reviewed changes
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

please add a test to tests/drivers/i2c for this new API.

@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Oct 13, 2021
@niedzwiecki-dawid
Copy link
Member Author

please add a test to tests/drivers/i2c for this new API.

@niedzwiecki-dawid
Copy link
Member Author

please add a test to tests/drivers/i2c for this new API.

Done.

@niedzwiecki-dawid
i2c: Add get_config to I2C API
e41afce 
Some applications need to get the current I2C configuration. Add a
proper callback to I2C API under Kconfig option not to change
applications that don't need this feature.

Signed-off-by: Dawid Niedzwiecki <dn@semihalf.com>
@niedzwiecki-dawid
i2c: npcx: Add get_config function
6a1018f 
Add get_config function to NPCX I2C driver. The master mode is hardcoded
and get the speed from a controller.

Signed-off-by: Dawid Niedzwiecki <dn@semihalf.com>
@niedzwiecki-dawid
i2c: emul: Add get_config function
47a9015 
Add get_config function to I2C emulator.

Also update tests using I2C emulator to use i2c_get_config.

Signed-off-by: Dawid Niedzwiecki <dn@semihalf.com>
@niedzwiecki-dawid
i2c: tests: update i2c_api test
85baece 
Add testing of the new i2c_get_config function to the i2c_api test.

Signed-off-by: Dawid Niedzwiecki <dn@semihalf.com>
@carlescufi
Copy link
Member

@galak and @sjg20 could you please take another look?

@niedzwiecki-dawid
Copy link
Member Author

@galak please revisit

@niedzwiecki-dawid
Copy link
Member Author

Do we have to wait for @galak?

@niedzwiecki-dawid
Copy link
Member Author

@carlescufi Is there anything I can do to merge the PR?

@galak galak dismissed their stale review November 2, 2021 15:46

tests added

@carlescufi carlescufi merged commit dc8d301 into zephyrproject-rtos:main Nov 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@de-nordic de-nordic de-nordic left review comments

@henrikbrixandersen henrikbrixandersen henrikbrixandersen approved these changes

@sjg20 sjg20 sjg20 approved these changes

@keith-zephyr keith-zephyr keith-zephyr approved these changes

@WealianLiao WealianLiao WealianLiao approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@ChiHuaL ChiHuaL Awaiting requested review from ChiHuaL ChiHuaL is a code owner

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@MulinChao MulinChao Awaiting requested review from MulinChao MulinChao is a code owner

@nashif nashif Awaiting requested review from nashif

@jackrosenthal jackrosenthal Awaiting requested review from jackrosenthal

@galak galak Awaiting requested review from galak

Assignees

@MulinChao MulinChao

Labels
area: API Changes to public APIs area: I2C area: Tests Issues related to a particular existing or missing test Breaking API Change Breaking changes to stable, public APIs platform: Nuvoton NPCX Nuvoton NPCX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet