On Thu, Jan 10, 2019 at 06:40:00PM +0100, Lubomir Rintel wrote: > The XO-1 and XO-1.5 batteries apparently differ in an ability to report > ambient temperature. Add a different compatible string to the 1.5 > battery. Hi Lubomir, Thanks for the recent Cc and pointer to here. In general, I have no problem with the net changes. I do have some concerns from a reviewability and change documentation perspective. You document your intent above, but you also made several other changes to get there which aren't documented, so when reviewing they stand out as "why is this here?". >From what I can tell, this change contains: 1) Cleanup of olpc_dt_interpret() calls to avoid splitting string literals (noted in the patch history, but not called out as an intentional change) 2) Refactoring of logic and breaking out the check for the compatible property into olpc_dt_compatible_match 3) Addition of "we're running very old firmware if this is missing" checks - I'm not sure how this relates to the stated problem and the intended fix. 4) The addition of the xo1.5 compatible property. All the other things makes it very difficult to determine if this patch does what you describe - and only what you describe. In general please: a) Cleanup code b) Refactor code c) Change functionality In that order - that way the new functionality is what show up when someone does a git blame on the file (rather than a cleanup patch, which isn't as useful in defect analysis). And always document in your commit messages the approach you take to fix the reported issue. Thanks, -- Darren Hart VMware Open Source Technology Center
