i2c support for mediatek soc
AbandonedPublic
Actions

Authored by yamori813_yahoo.co.jp on Oct 20 2017, 6:01 AM.

Details

Reviewers

sgalabov
mizhka
adrian
• ian

Summary

i2c support for rt soc.

Test Plan

user land i2c command and kernel rtc iic module on RT2880

Diff Detail

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 23838
Build 22776: arc lint + arc unit

Event Timeline

yamori813_yahoo.co.jp created this revision.Oct 20 2017, 6:01 AM

Harbormaster completed remote builds in B12136: Diff 34167.Oct 20 2017, 6:01 AM

mizhka requested changes to this revision.Oct 20 2017, 10:20 AM

mizhka added inline comments.

sys/mips/mediatek/mtk_iic.c
156–167	Could you please remove child "iicbus" if not NULL? mtx_destroy? The mtx_destroy() function is used to destroy mutex so the data associated with it may be freed or otherwise overwritten. Any mutex which is destroyed must previously have been initialized with mtx_init(). It is permissible to have a single hold count on a mutex when it is destroyed. It is not permissible to hold the mutex recursively, or have another thread blocked on the mutex when it is destroyed.
172–175	"style(9)" :)
397–399	To remove?
418	IICBUS_MINVER, IICBUS_PREFVER, IICBUS_MAXVER ?
sys/mips/mediatek/mtk_iic.h
44–65	Please use space between #define and macro name
70	As I understand, 333MHz is specific for RT2880, isn't it? For any other SoC, it can be different from 333.

This revision now requires changes to proceed.Oct 20 2017, 10:20 AM

IMO, this entire driver is wrong. Because of the way the hardware works, it cannot support the start, repeat_start, stop, read, and write methods for iicbus_if, and it must not try to "fake" that support. It must implement only callback, reset, and transfer. The transfer method should be basically just

one time, maybe in reset(), set RA_I2C_CONFIG reg to 
      I2C_CONFIG_ADDRLEN(I2C_CONFIG_ADDRLEN_8) |
      I2C_CONFIG_DEVADLEN(I2C_CONFIG_DEVADLEN_7) |
      I2C_CONFIG_ADDRDIS;

in transfer method:
 for each msg
   set RA_I2C_DEVADDR reg to msg.slave >> 1
   set RA_I2C_BYTECNT reg to msg.len
   set RA_I2C_STARTXFER reg to either OP_READ or OP_WRITE
   loop to transfer all the bytes into/out-of msg.buf

There is an example of a transfer-only driver in sys/arm/broadcom/bcm2835/bcm2835_bsc.c, but it is a bit complex because it uses interrupts, and it uses a hardware quirk to make repeat-start work.

sys/mips/mediatek/mtk_iic.h
44–65	style(9) requires a tab after #define

• ian added a reviewer: • ian.Oct 21 2017, 5:31 PM

mizhka added inline comments.Oct 21 2017, 6:54 PM

sys/mips/mediatek/mtk_iic.h
44–65	Of course, a tab. Sorry, mistype.

review refict

remove child

@sgalabov , @adrian , could you please look at?

This revision is now accepted and ready to land.Dec 7 2017, 8:13 PM

sgalabov requested changes to this revision.Dec 8 2017, 8:48 AM

sgalabov added inline comments.

sys/boot/fdt/dts/mips/fbsd-rt2880.dtsi
1 ↗	(On Diff #34273)	The dtsi file changes do not belong with this change. They should be in a separate review IMO. In any case, even then, we should not import the entire file. The rt2880.dtsi is already imported in sys/gnu/dts/mips. What should be done (and it was done for the other Ralink/Mediatek SoCs) is the following: add the following line at the end of sys/gnu/dts/mips/rt2880.dtsi: #include "fbsd-rt2880.dtsi" create the file sys/dts/fbsd-rt2880.dtsi and only include what's missing from the original sys/gnu/dts/mips/rt2880.dtsi file.
sys/mips/mediatek/mtk_iic.c
2	I agree with the comment by @ian - I think we should only implement callback, reset and transfer and we should not try to fake what the SoC cannot do.