drivers: add imx7d i2c driver #7424

diegosueiro · 2018-05-09T05:39:51Z

This pull request is to add i2c driver for mcimx7_m4 arch and colibri_imx7d_m4 board.

The implementation was tested on the colibri_imx7d_m4 board by compiling and running the samples/grove/lcd application (extra delays between messages have to be added because of the grove lcd module restrictions) using the I2C_4 bus. Another validation was made with a simple application performing reads and writes to an RTC (m41t0) located in the Toradex Colibri Evaluation board.

codecov-io · 2018-05-09T06:08:54Z

Codecov Report

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

@@           Coverage Diff           @@
##           master    #7424   +/-   ##
=======================================
  Coverage   52.24%   52.24%           
=======================================
  Files         195      195           
  Lines       24655    24655           
  Branches     5123     5123           
=======================================
  Hits        12880    12880           
  Misses       9709     9709           
  Partials     2066     2066

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 a7e2c58...84b5445. Read the comment docs.

galak

See comments, also need to add device tree support.

galak · 2018-05-09T14:07:08Z

drivers/i2c/CMakeLists.txt

Seems like we should keep this the same as the other lines:

zephyr_source_ifdef(CONFIG_I2C_IMX . i2c_imx.c)

galak · 2018-05-09T14:09:02Z

drivers/i2c/Kconfig.imx

Don't add a new Kconfig file, please model this off of what is done for i2c_mcux, use CONFIG_I2C_1, CONFIG_I2C_2, etc.

For imx we have the hardware mapped from I2C_1 to I2C_4 and in the Kconfig we have I2C_0 to I2C_3.

What is your suggestion, add the I2C_4 option to Kconfig or use 0 to 3 and consider 0 as 1, 1 as 2 and so on, for the imx?

I'm saying add I2C_4 to Kconfig, and just use I2C_1..I2C_4

galak · 2018-05-09T14:09:20Z

drivers/i2c/i2c_imx.c

Shouldn't this be your copyright?

galak · 2018-05-09T14:13:50Z

dts/bindings/i2c/nxp,imx-i2c.yaml

should be compatible = "fsl,imx7d-i2c";

@galak,

This binding can be used by other imx cores (ie. imx6sx_m4). I believe that it is the same situation of nxp,imx-gpio.yaml, that shares the same dts binding and gpio driver shim.

I even think that they can share the same i2c driver shim. Maybe @stanislav-poboril can help us in checking this.

So technically I think it should be: "fsl,imx21-i2c", this is based on the device tree used in Linux for i.mx. We should using the same compats / bindings for devices that already have one's defined by Linux.

So if you look at the imx dts you'll see something like:

compatible = "fsl,imx7d-i2c", "fsl,imx21-i2c";

So, do you think we have to change in nxp,imx-i2c.yaml to constraint: "fsl,imx7d-i2c", "fsl,imx21-i2c" and on nxp_imx7d_m4.dtsi compatible = "fsl,imx7d-i2c"?

Sorry but I got a little bit confused what needs to be changed.

If I change from "nxp,imx-i2c" to "fsl,imx21-i2c" I'll have to change NXP_IMX_I2C to FSL_IMX21_I2C in the dts.fixup file which will not follow the definition pattern of the other devices like UART and GPIO and looks strange to me.

I would like to keep the same pattern for easy maintenance.

@MaureenHelm ,
What do you think?

So, do you think we have to change in nxp,imx-i2c.yaml to constraint: "fsl,imx7d-i2c", "fsl,imx21-i2c" and on nxp_imx7d_m4.dtsi compatible = "fsl,imx7d-i2c"?

Sorry but I got a little bit confused what needs to be changed.

So we don't handle multiple compats well today with regards to the binding files. So I'm saying we use 'fsl,imx7d-i2c' so that we are matching linux (at least partially).

I understand that the pattern will differ from other items, but that is ok. This is what should be in the DTS for the i2c controller.

Ok. But when the imx6sx will be added it will have to duplicate the fsl,imx7d-i2c.yaml to fsl,imx6sl-i2c.yaml and the compatible string as well.

In the Linux kernel the common denominator for both platforms is "fsl,imx21-i2c", why we can't use "nxp,imx-i2c" as a common denominator for Zephyr?

Don't get me wrong, but I'm just trying to keep things simple, concise and avoid unnecessary code duplication.

So we don't handle multiple compats well today with regards to the binding files.

Does this mean we can define exactly one constraint in a given yaml file, but we can define multiple compatibles in the dts?

Does this mean we can define exactly one constraint in a given yaml file, but we can define multiple compatibles in the dts?

Correct, the dts is happy to have multiple compats. The yaml for the binding don't handle this well. Right now there is an expectation of 1:1 yaml to compat.

@galak ,

Devicetree compatible string suggestion addressed.

dbkinder

+1 for doc changes

diegosueiro · 2018-05-15T16:48:57Z

@galak and @MaureenHelm ,

Are you expecting to merge this before the merge window close?

diegosueiro · 2018-06-20T09:42:56Z

@galak and @MaureenHelm,

Should I rebase this PR to get it merged?

MaureenHelm · 2018-06-26T17:10:11Z

boards/arm/colibri_imx7d_m4/Kconfig.board

@galak, should this select get moved to the driver? We're inconsistent about where HAS_DTS_I2C gets selected. In some cases (NXP and STM), it's selected by the driver, in other cases it's selected by the board (Nordic, x86)

@galak,

What should I do with the HAS_DTS_I2C selection?

Seems like the driver should select this, not the board.

@MaureenHelm and @galak ,

Done.

MaureenHelm · 2018-06-26T17:10:50Z

boards/arm/colibri_imx7d_m4/colibri_imx7d_m4.dts

Replace the underscore with a hyphen

MaureenHelm · 2018-06-26T17:18:24Z

boards/arm/colibri_imx7d_m4/doc/colibri_imx7d_m4.rst

I2C4, not I2C3

MaureenHelm · 2018-06-26T17:26:54Z

drivers/i2c/CMakeLists.txt

Please move this below i2c_gpio to maintain alpha order

MaureenHelm · 2018-06-26T17:58:08Z

drivers/i2c/i2c_imx.c

I2C_MSG_RESTART applies to the current message, not the next one, so I don't think it's correct to preserve the restart flag here. Look at how it's used in i2c_burst_read in include/i2c.h

@MaureenHelm ,

I've followed the same approach implemented in i2c_bitbang.c and tested with different i2c messages combinations and it worked without any issues.Preserving the restart flag is used to decide if it needs to send and start (by setting the appropriate mode according to the data sheet) or repeat start on the bus.

MaureenHelm · 2018-07-02T16:14:19Z

drivers/i2c/i2c_imx.c

This condition will only be satisfied on the first iteration and doesn't need to be inside the do/while loop. If you move it out, then you can move flags |= msgs->flags to the beginning of the loop, and use flags consistently throughout the loop. The current mix of flags and msgs->flags is confusing.

MaureenHelm · 2018-07-02T16:19:07Z

drivers/i2c/Kconfig

Remove this line. See #8586

MaureenHelm · 2018-07-02T16:22:26Z

ext/hal/nxp/imx/Kconfig

Remove this line

Adds a shim layer around the imx i2c driver to adapt it to the Zephyr i2c master interface. Signed-off-by: Diego Sueiro <[email protected]>

Adds all necessary i2c definitions and configurations for imx7d_m4 soc. Signed-off-by: Diego Sueiro <[email protected]>

Adds definitions, configurations and device tree entries for colibri_imx7d_m4 board. Signed-off-by: Diego Sueiro <[email protected]>

comments addressed

diegosueiro requested review from MaureenHelm and dbkinder as code owners May 9, 2018 05:39

galak previously requested changes May 9, 2018

View reviewed changes

galak reviewed May 9, 2018

View reviewed changes

galak added area: I2C platform: NXP NXP labels May 9, 2018

galak requested a review from tbursztyka May 9, 2018 14:15

dbkinder approved these changes May 9, 2018

View reviewed changes

diegosueiro force-pushed the imx7d_i2c branch 2 times, most recently from bfe050a to 0799657 Compare May 11, 2018 21:21

diegosueiro force-pushed the imx7d_i2c branch from 0799657 to 61d9d0c Compare June 23, 2018 03:28

MaureenHelm requested changes Jun 26, 2018

View reviewed changes

diegosueiro force-pushed the imx7d_i2c branch 2 times, most recently from 8a15094 to 6e96a7b Compare July 2, 2018 05:48

MaureenHelm requested changes Jul 2, 2018

View reviewed changes

diegosueiro force-pushed the imx7d_i2c branch from 6e96a7b to d166287 Compare July 3, 2018 05:59

diegosueiro added 3 commits July 3, 2018 17:21

drivers: add i.MX I2C driver shim

6ec7af0

Adds a shim layer around the imx i2c driver to adapt it to the Zephyr i2c master interface. Signed-off-by: Diego Sueiro <[email protected]>

arch: Add imx7d_m4 i2c definitions

37e4660

Adds all necessary i2c definitions and configurations for imx7d_m4 soc. Signed-off-by: Diego Sueiro <[email protected]>

board: add i2c support for colibri_imx7d_m4 board

84b5445

Adds definitions, configurations and device tree entries for colibri_imx7d_m4 board. Signed-off-by: Diego Sueiro <[email protected]>

diegosueiro force-pushed the imx7d_i2c branch from d166287 to 84b5445 Compare July 3, 2018 16:21

MaureenHelm approved these changes Jul 6, 2018

View reviewed changes

MaureenHelm merged commit 618209a into zephyrproject-rtos:master Jul 6, 2018

diegosueiro deleted the imx7d_i2c branch July 19, 2018 03:08

drivers: add imx7d i2c driver #7424

drivers: add imx7d i2c driver #7424

Uh oh!

Conversation

diegosueiro commented May 9, 2018

Uh oh!

codecov-io commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegosueiro May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diegosueiro May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbkinder left a comment

Choose a reason for hiding this comment

Uh oh!

diegosueiro commented May 15, 2018

Uh oh!

diegosueiro commented Jun 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented May 9, 2018 •

edited

Loading

diegosueiro May 9, 2018 •

edited

Loading

diegosueiro May 9, 2018 •

edited

Loading