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

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

semihalf-niedzwiecki-dawid
Copy link
Contributor

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Sep 30, 2021

No description provided.

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 Stable API Change label Sep 30, 2021
@de-nordic
Copy link
Collaborator

@de-nordic de-nordic commented Sep 30, 2021

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

drivers/i2c/i2c_npcx_port.c Outdated Show resolved Hide resolved
drivers/i2c/Kconfig Outdated Show resolved Hide resolved
include/drivers/i2c.h Show resolved Hide resolved
include/drivers/i2c.h Outdated Show resolved Hide resolved
@de-nordic
Copy link
Collaborator

@de-nordic de-nordic 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.

@sjg20
Copy link
Collaborator

@sjg20 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

@de-nordic de-nordic commented Oct 1, 2021

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.

@semihalf-niedzwiecki-dawid
Copy link
Contributor Author

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Oct 1, 2021

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

Yes, done.

@semihalf-niedzwiecki-dawid
Copy link
Contributor Author

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Oct 1, 2021

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.

drivers/i2c/i2c_npcx_port.c Outdated Show resolved Hide resolved
@semihalf-niedzwiecki-dawid
Copy link
Contributor Author

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Oct 8, 2021

Is there any way to speed up the review?

@de-nordic
Copy link
Collaborator

@de-nordic de-nordic commented Oct 8, 2021

galak
galak previously requested changes Oct 12, 2021
Copy link
Contributor

@galak galak left a comment

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

@github-actions github-actions bot added the area: Tests label Oct 13, 2021
tests/drivers/eeprom/src/main.c Show resolved Hide resolved
tests/drivers/i2c/i2c_api/src/test_i2c.c Show resolved Hide resolved
@semihalf-niedzwiecki-dawid
Copy link
Contributor Author

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Oct 18, 2021

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

@semihalf-niedzwiecki-dawid
Copy link
Contributor Author

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Oct 18, 2021

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

Done.

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>
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>
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>
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

@carlescufi carlescufi commented Oct 18, 2021

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

sjg20
sjg20 approved these changes Oct 18, 2021
@semihalf-niedzwiecki-dawid
Copy link
Contributor Author

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Oct 20, 2021

@galak please revisit

@semihalf-niedzwiecki-dawid
Copy link
Contributor Author

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Oct 22, 2021

Do we have to wait for @galak?

@semihalf-niedzwiecki-dawid
Copy link
Contributor Author

@semihalf-niedzwiecki-dawid semihalf-niedzwiecki-dawid commented Oct 26, 2021

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

@galak galak dismissed their stale review Nov 2, 2021

tests added

@carlescufi carlescufi merged commit dc8d301 into zephyrproject-rtos:main Nov 2, 2021
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API area: I2C area: Tests platform: Nuvoton NPCX Stable API Change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet