Skip to content

Conversation

@aurel32
Copy link
Contributor

@aurel32 aurel32 commented Jul 3, 2018

This patch series adds support for I2C bus of the STM32F7 family, enables it on the STM32F723E-DISCO board, and finally fixes an issue found while testing the I2C support in interrupt mode.

I have tested it using by communicating with the WM8994 chip on the STM32F723E-DISCO board.

@codecov-io
Copy link

codecov-io commented Jul 3, 2018

Codecov Report

Merging #8716 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8716   +/-   ##
=======================================
  Coverage   52.45%   52.45%           
=======================================
  Files         200      200           
  Lines       25121    25121           
  Branches     5244     5244           
=======================================
  Hits        13176    13176           
  Misses       9823     9823           
  Partials     2122     2122

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a159447...4dbcb0d. Read the comment docs.

Copy link
Contributor

@ydamigos ydamigos left a comment

Choose a reason for hiding this comment

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

Looks good. I am just blocking it because PR #7626 also introduces the NACK fix.

@aurel32 could you coordinate with @vaussard on this?

@aurel32
Copy link
Contributor Author

aurel32 commented Jul 4, 2018

@ydamigos, thanks for the pointer. Given there is already a PR opened about that issue, I believe it's better to discuss it there. In addition the fix is now conflicting with the changes introduced by PR#5224, and it more work is needed to understand how to handle a NACK in slave mode. I'll therefore just drop that commit from the pull request.

@aurel32
Copy link
Contributor Author

aurel32 commented Jul 4, 2018

And I have just added an additional cleanup commit to make @ulfalizer happy.

@aurel32
Copy link
Contributor Author

aurel32 commented Jul 4, 2018

@ydamigos, there was a third PR with a fix for this issue, PR #5224. Given it has just been merged, i confirm that just removing the commit from my PR was the right thing to do.

Copy link
Member

@erwango erwango left a comment

Choose a reason for hiding this comment

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

One comment.

Otherwise, could you update help message for I2C_STM32_V2. Message is not quite clear but we should be explicit that drivers now supports F7 (while previously it was 'compatible': I agree this is subtle :-))

Copy link
Member

Choose a reason for hiding this comment

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

Should be 'n' by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

@erwango We enable buses on board level when there is a peripheral (e.g. WM8994 or touch panel in this case) that needs them, aren't we?

Copy link
Member

Choose a reason for hiding this comment

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

@ydamigos : we should configure them, and this is what is done here. But activation should always be done in application and not default.
Aim is that several developers on the same board know what is the base components for the board so they only enable what they need. But they don't have to bother disabling what other developers may have enabled meanwhile.

Copy link
Contributor

Choose a reason for hiding this comment

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

@erwango We need to clean up board _defconfig files then.

Copy link
Member

Choose a reason for hiding this comment

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

@ydamigos : yes, there is some work ahead! I raised #7151 earlier, waiting #7176 to be reviewed and used a reference (which was locked waiting for new SDK version and which I hope should get some feedbacks soon)

@aurel32
Copy link
Contributor Author

aurel32 commented Jul 5, 2018

@erwango: I have just done the requested changes, i.e. I updated the Kconfig.stm32 help message to mark F7 as supported, and removed CONFIG_I2C=y from the board defconfig.

@aurel32
Copy link
Contributor Author

aurel32 commented Jul 5, 2018

@erwango, I have just pushed a new version to solve the conflict introduced by PR #8663 and to add the new clock properties introduced in PR #8759.

Copy link
Contributor

@dbkinder dbkinder left a comment

Choose a reason for hiding this comment

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

Doc changes LGTM, thanks.

@aurel32
Copy link
Contributor Author

aurel32 commented Jul 11, 2018

I have just pushed a new version to solve the conflict due the merge of USB OTG FS support. There is no code change beside solving conflicts.

aurel32 added 3 commits July 25, 2018 11:59
The STM32F7 uses the V2 version of the STM32 I2C controller. Add the
corresponding Kconfig, DTS, DTS fixup and pinmux entries.

Signed-off-by: Aurelien Jarno <[email protected]>
Enable the support for the 3 I2C buses of the STM32F723E-DISCO board,
connected respectively to the WM8994, the Arduino V3/STMOD+ and the
touch panel.

Signed-off-by: Aurelien Jarno <[email protected]>
Bool symbols implicitly default to 'n'.

A 'default n' can make sense e.g. in a Kconfig.defconfig file, if you
want to override a 'default y' on the base definition of the symbol. It
isn't used like that on any of these symbols though, and is
inconsistent.

This will make the auto-generated Kconfig documentation have "No
defaults. Implicitly defaults to n." as well, which is clearer than
'default n if ...'

This is basically reapplying commit 133a299 ("drivers: i2c: Kconfig:
Remove redundant 'default n' properties"), which has been partially
reverted in commit c7875b7 ("i2c: stm32_v2: implement slave
support").

Signed-off-by: Aurelien Jarno <[email protected]>
@aurel32
Copy link
Contributor Author

aurel32 commented Jul 25, 2018

I have just pushed a new version to solve the conflict due the merge of USB OTG HS support. There is no code change beside solving conflicts.

@nashif nashif merged commit 6c49ce1 into zephyrproject-rtos:master Jul 25, 2018
@aurel32 aurel32 deleted the stm32f7-i2c branch July 25, 2018 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants