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

kernel: I2C better driver for MT7621/MT7628 #831

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@j123b567
Copy link

j123b567 commented Mar 29, 2018

New supported features

  • unlimited message length
  • clock stretching
  • ACK handling - i2c-detect now works - it probably fixes bug #845
  • repeated start sequence

Signed-off-by: Jan Breuer jan.breuer@jaybee.cz

@hitech95

This comment has been minimized.

Copy link
Contributor

hitech95 commented Apr 3, 2018

Did you used the manual i2c mode?
I have checked back in time but I didn't found any documents for that!

@j123b567

This comment has been minimized.

Copy link

j123b567 commented Apr 3, 2018

Yes, I'm using I2C manual mode. Just registers are documented. I have written python script to directly access I2C registers from userspace to try how it works and later write kernel driver.

@hitech95

This comment has been minimized.

Copy link
Contributor

hitech95 commented Apr 3, 2018

Good work. I'm not an expert so I stopped after seeing that the "auto" mode had a lot of missing function like the message length.

@blogic

This comment has been minimized.

Copy link
Contributor

blogic commented Apr 4, 2018

@j123b567 so this essentially adds a new driver ? please create 2 patches,

  1. remove old driver
  2. add new driver
    as this is quite a huge change please wrte an appropriate patch description explaining the auto vs manual mode thing etc etc
@blogic

This comment has been minimized.

Copy link
Contributor

blogic commented Apr 4, 2018

and the prefix should be "ramips:"

@j123b567 j123b567 force-pushed the j123b567:feature/mt7621-mt7628-better-i2c branch from 6037dd1 to 1d3a697 Apr 4, 2018

@j123b567

This comment has been minimized.

Copy link

j123b567 commented Apr 4, 2018

I have tested this driver just on MT7688, which is the same SoC as MT7628 (except WiFi).

According to the datasheet, it should work on MT7621, because it has completely the same I2C registers, but I don't have such hardware.

@yatinvasava

This comment has been minimized.

Copy link

yatinvasava commented Apr 5, 2018

hey it can work with any devices means it can be used for other i2c devices too ???

@yatinvasava

This comment has been minimized.

Copy link

yatinvasava commented Apr 5, 2018

bcz i'm trying to connect onion omega2+ with ArduinoMEGA ..
and also i design break out for onion omega by myself...
thanks in advance...

@j123b567

This comment has been minimized.

Copy link

j123b567 commented Apr 5, 2018

@yatinvasava it is usable for example for VoCore2, Linklt Smart 7688, Onion Omega2 and Omega2+

@yatinvasava

This comment has been minimized.

Copy link

yatinvasava commented Apr 5, 2018

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Apr 5, 2018

The commits are missing Signed-off-by. Also a comment could be useful for the driver removal commit. Something like this is now obsolete.

ramips: remove i2c driver for MT7621/MT7628/MT7688 using auto mode
This driver is now obsolate.

Signed-off-by: Jan Breuer <jan.breuer@jaybee.cz>

@j123b567 j123b567 force-pushed the j123b567:feature/mt7621-mt7628-better-i2c branch from 1d3a697 to 055a1cf Apr 6, 2018

ramips: add i2c MT7621/MT7628/MT7688 driver using manual mode
I2C on MT7621/MT7628/MT7688 has two modes: "auto" and "manual". Old driver
uses "auto" mode which does not support propper ACK/NAK handling, clock
stratching, repeated start and was limited to 32 bytes per message.
This driver is using "manual" mode so it can support all these features.
Main advantages over old driver are:
 - unlimited message length
 - clock stretching
 - ACK/NAK handling (i2c-detect now works)
 - repeated start sequence

Signed-off-by: Jan Breuer <jan.breuer@jaybee.cz>

@j123b567 j123b567 force-pushed the j123b567:feature/mt7621-mt7628-better-i2c branch from 055a1cf to df110c1 Apr 6, 2018

@j123b567

This comment has been minimized.

Copy link

j123b567 commented Apr 6, 2018

@neheb It should be fixed now

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Apr 22, 2018

Kernel version 4.9 got axed from mainline and so did the patch directory.

This is a bit confusing. So the driver is the same, just modified with extra features. This is the diff I get with some tweaking: https://gist.github.com/neheb/427d2d7dea32b53b55b75442d240b9b2

I think it would be better to make the driver a part of the files-4.14 directory instead of a patch file. Look at the recent changes in the MMC driver for ramips for hints.

@j123b567

This comment has been minimized.

Copy link

j123b567 commented Apr 22, 2018

@neheb I can change any formal representation of the driver/patches (move to dir, etc.) but please in th meantime, can you comment the driver code itself? Test it on some other platform then me, etc?

This change is very radical. Only the skeleton of the original driver is reused. The core part of the communication is completely different. So no, it does not just add extra features, it uses completely different approach to work with the SoC registers and to communicate with outer world.

@neheb

This comment has been minimized.

Copy link
Contributor

neheb commented Apr 23, 2018

I don't have any ramips devices with anything connected to i2c, so I can't really test.

After looking into this a bit more, there is a mainline kernel driver for Mediatek i2c that can probably be adapted to replace the OpenWrt specific one. Although that's probably a longer term solution.

@j123b567

This comment has been minimized.

Copy link

j123b567 commented Apr 24, 2018

@neheb I can find only i2c-mt65xx driver in the mainline which is for ARM based SoC (MT7622, MT65xx, etc.) . It has completely different registers and for example it has DMA available for I2C.

It is not compatible in any way with MIPS based MT7621 and friends.

@j123b567

This comment has been minimized.

Copy link

j123b567 commented May 5, 2018

I have feedback that it does not work on all similar SoCs as expected (namely mt7628), I must investigate the problem first.

@mkresin

This comment has been minimized.

Copy link
Member

mkresin commented Jul 20, 2018

@j123b567 do you have an update for us? What doesn't actually work with mt7628?

@j123b567

This comment has been minimized.

Copy link

j123b567 commented Jul 27, 2018

@mkresin No, I don't have any update.

We are using this driver in production on mt7688. Maybe, i can limit this driver just for this cpu and it can replace old driver on other cpus later.

@blogic

This comment has been minimized.

Copy link
Contributor

blogic commented Jul 27, 2018

@j123b567 could you try to send this upstream to linux-i2c ?

@j123b567

This comment has been minimized.

Copy link

j123b567 commented Jul 27, 2018

@blogic ok, I will try to send this upstream.

@blogic

This comment has been minimized.

Copy link
Contributor

blogic commented Jul 27, 2018

@j123b567 please CC me. in general the code looks good. i am reluctant to merge it as its a huge change and we dont know if it will work on all old SoC versions. I dont have the HW to test thisyet we have units with rt305x that use i2c which will likely break. carrying 2 out of tree drivers is also very cumbersome and eats up a chunk of my time. If we manage to get this upstream, it'll be backportable easily

@blogic

This comment has been minimized.

Copy link
Contributor

blogic commented Jul 30, 2018

@j123b567 Hi Jan, I am cleaning up github, I'll close this one until it went upstream at which point we can pull a backport into the tree. please CC me on your upstream effort and if you need help send me a mail directly.
John

@blogic blogic closed this Jul 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment