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

Add LTC294X Driver #359

Merged
merged 2 commits into from May 19, 2017
Merged

Add LTC294X Driver #359

merged 2 commits into from May 19, 2017

Conversation

bradjc
Copy link
Contributor

@bradjc bradjc commented Apr 24, 2017

The LTC294X is a line of coulomb counters we use on signpost.

The 2491/2/3 have slightly different register sets and the capsule tries to do the right thing based on which is on the board. The capsule defaults to the ltc2941, and lets the actual chip be set at runtime because it may not be known when the kernel is compiled (as is the case for signpost).

LTC294X {
i2c: i2c,
interrupt_pin: interrupt_pin,
model: Cell::new(ChipModel::LTC2941),
Copy link
Member

Choose a reason for hiding this comment

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

Is the 2941 a strict subset of the 42 and 43? (Or should there be a "NoChip" default in place before users call set_model?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sort of? Everything that you can do to the 41 won't fail on the 42/43, but some of the bit fields have different meanings. Seems fine to just default to the simple one.


fn done(&self) {
self.callback.get().map(|mut cb| { cb.schedule(3, 0, 0); });
}
Copy link
Member

Choose a reason for hiding this comment

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

Could maybe re-order these functions so the callback #'s are in order?

case 4096: M = 7; break;
default: M = 4; break;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Should this have an else { return ENOSUPPORT; } for a bad model here?

@alevy
Copy link
Member

alevy commented May 10, 2017

My biggest comment for all of these PRs (including #360 and #361) are that the capsule module could use much more commenting. What is this peripheral? What is it used for? How should I instantiate it? What is the high level system call interface?

This is asking more than we have so far in most other capsules, but you know... constant improvement etc...

I've updated the CRC driver with a structure of documentation I think starts to make more sense, so worth maybe copying a bit from there. I think these drivers will differ in more focus on describing the peripheral as well as documenting the non-system-call interface where it exists.

The LTC294X is a series of coulumb counters for measuring energy use or
battery state. The interface between the chips is similar but they do
have different registers. This capsule tries to be general across the
series of chips.
@bradjc
Copy link
Contributor Author

bradjc commented May 18, 2017

I think the changes I just made are getting closer to what you are looking for. While docs are good, what keeps me up at night was that I just put example code in the comment that could be invalid in the future, even if the code in the ltc2941.rs file never changes. We have to be diligent that things get updated if that stuff changes in the future, otherwise it is worse than having no comment.

@ppannuto
Copy link
Member

To that point, rust has a doctest mechansim that we could leverage to ensure that that code at a minimum remains compile-correct, right?

@alevy
Copy link
Member

alevy commented May 18, 2017

@ppannuto in principle, yes, but we would need a way to compile and run (at least tests) on x86---we may not be that far from being able to do it though

@ppannuto
Copy link
Member

ppannuto commented May 18, 2017 via email

@alevy alevy merged commit 95f5382 into master May 19, 2017
@alevy alevy deleted the ltc2941 branch May 19, 2017 21:34
This was referenced May 19, 2017
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.

None yet

3 participants