On 3/17/20 9:17 PM, Alexandru Lazar wrote:
Hello! This patch series adds support for the Maxim MAX1241, a 12-bit, single-channel, SPI-connected ADC. It's a pretty sturdy ADC that I've used for some time now and I figured my driver may be useful to others as well. I originally thought I might get this under the max1027 driver but the 124x family is different enough that shoehorning it in there didn't seem like a good idea. The 1241 has a single channel, no FIFO, uses a different mechanism to signal conversion start and has a low-power operation mode which it uses in a pretty different way. This is actually closer to MAX111x, but those also have a pretty different signaling convention. Needless to say, though, if anyone who is more familiar with this situation disagrees and wants to point me in the direction of the appropriate driver, I'm happy to make the changes there instead of providing a separate driver.
I think your approach is the best approach here. Trying to build "generic" drivers that support lots of only partially related devices can get messy real quick.
Also please note that I am somewhat unfamiliar with the idioms of the IIO subsystem's tree. Jonathan Cameron was kind enough to give me a few pointers but there are a few questions I didn't bother him with (so I'm guess I'm gonna bother you instead now). 1. There are two ADCs in this family, MAX1240 and MAX1241. This driver only supports the MAX1241. The scaffolding to get MAX1240 support is in there, but I didn't have access to a MAX1240 and I didn't want to submit untested code for review. Can we add MAX1240 support later, or should I do it from the very beginning? Either way is fine by me (I'm just a little concerned about how quickly I might *get* a MAX1240 these days...)
Can be added later. Its always good to start with a small tested baseline. But either way it is up to you, no problem with having it supported by the initial patch either.
2. Looking at other drivers, it seems to me that, on ADCs that require an external reference, it's customary to make a voltage regulator a required property in the DT bindings. Am I correct here?
Yes.
3. checkpatch.pl warns me that the MAINTAINERS file might need updating. I'm not sure what the appropriate thing to do is here -- I can maintain this driver indefinitely (I am actively using it anways) but it's a 200-line driver, does that warrant inclusion in MAINTAINERS?
Different people will have different options on this. My opinion is that it is not needed for small drivers. get_maintainers.pl will list your name anyway since you are the author for the commit.
My apologies if anything here is distinctly bad -- this isn't the first time I'm writing kernel code but it's definitely the first time I'm sending ay upstream. Any bugs are due to my own incompetence, everything else is just temporary ignorance :-).
The driver's code looks very clean. Top job! - Lars
