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

Extend clock timeout, fix modprobe baudrate parameter. #1241

Merged
merged 1 commit into from
Dec 30, 2015

Conversation

DFyson
Copy link

@DFyson DFyson commented Dec 27, 2015

  • Set the BSC_CLKT clock streching timeout to 35ms as per SMBus specs.
  • Increase priority of baudrate parameter passed to modprobe (in /etc/modprobe.d/*.conf or command line). Currently custom baudrates don't work because they are overridden by clock-frequency in the platform_device passed to the function. And editing the /boot/config.txt is clunky since a reboot is required.

Both these problems seem to be reoccurring around the web for example here and here.

I noticed this was already addressed in #211 but the problem still exists. The default value of 64 clock cycle should not be used since the timeout depends on the baudrate. It should be a constant time no matter the baudrate for all the i2c slaves which require a delay (such as many humidity sensors). The clock stretch timeout could also be disabled by writing 0 to the CLKT thus making i2c compliant but I assume SMBus specs should be fine.

@popcornmix
Copy link
Collaborator

@pelwell @P33M ?

@pelwell
Copy link
Contributor

pelwell commented Dec 29, 2015

I'm OK with the patches in principle, but I have a few reservations about the implementation:

  1. I'd like a single patch, since the second is a correction to the first.
  2. Why do you bounds-check clk_tout in bcm2708_bsc_setup when it could be done in bcm2708_i2c_probe (c.f. cdiv)?
  3. A very minor one, but I think if (!baudrate) { is more Linux than if (baudrate == 0){ (note the additional space as well).

@DFyson
Copy link
Author

DFyson commented Dec 29, 2015

2,3: good points, fixed those.
I don't quite get what you mean that setting a custom baudrate is a fix for clock stretch timeout. I know some people have used it as a workaround by slowing down the clock to increase the timeout length but there are many other reasons to change the baud rate.

@pelwell
Copy link
Contributor

pelwell commented Dec 30, 2015

I don't quite get what you mean that setting a custom baudrate is a fix for clock stretch timeout.

I didn't say that. Yes, your patch set does two things, as evidenced by your two bullet points, but I have no problem with that. What I objected to is that there were two commits (now four), the second of which only corrected the timeout value to 35ms.

Please squash all four commits down to one, correcting the 32 in the initial commit message, then git push -f to update the pull request.

@DFyson
Copy link
Author

DFyson commented Dec 30, 2015

Oh sorry I thought you were referring to my points.
I'm still trying to figure out github, is there no way to do that here? Otherwise it sounds like I would have to download the whole repo with git and I have a painfully slow internet connection.

@pelwell
Copy link
Contributor

pelwell commented Dec 30, 2015

Disappointingly there doesn't seem to be a way of doing it using the github GUI. If you want, I can squash and merge thern for you - you will still get the credit. Alternatively, you can clone with --depth=1 which should be much quicker than downloading the whole repo.

@P33M
Copy link
Contributor

P33M commented Dec 30, 2015

Github has always been terrible at support for the Git features that rewrite history - i.e. rebase/squashing and forced updates.

One thing of note: if you clone with --depth=1 then pretty much every action involving manipulating commit history will fail - currently the only realistic option is a full repo clone and command-line manipulation.

Set the BSC_CLKT clock streching timeout to 35ms as per SMBus specs.\n- Increase priority of baudrate parameter passed to modprobe (in /etc/modprobe.d/*.conf or command line). Currently custom baudrates don't work because they are overridden by clock-frequency in the platform_device passed to the function.
@DFyson
Copy link
Author

DFyson commented Dec 30, 2015

Ok managed to get that sorted. Thanks for the help.

pelwell added a commit that referenced this pull request Dec 30, 2015
Extend clock timeout, fix modprobe baudrate parameter.
@pelwell pelwell merged commit c361a20 into raspberrypi:rpi-4.1.y Dec 30, 2015
@shiftleftplusone
Copy link

just to mention FYI:

FTM, Arduinos (AVR: Uno, Mega, Nano) can not be plugged as i2c slaves, I2c bus hangs up then.
Arduino DUE (ARM) works fine though.

The Issue has been reported and discussed in this topic:

https://www.raspberrypi.org/forums/viewtopic.php?f=33&t=133334&start=25#p896363

https://www.raspberrypi.org/forums/viewtopic.php?f=33&t=133334&start=50#p904479

So IMO it would be wishful to have a new Raspbian Jessie Kernel which is I2C clock-stretching compliant, especially for Arduino AVRs.

ref.: https://www.raspberrypi.org/forums/viewtopic.php?f=66&t=136219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants