On 10/9/21 7:50 AM, Oskar Senft wrote:
Hi Guenter
Thanks for the review!
+ return sprintf(buf, "%u\n",
+ ((mode >> MODE_BIT_OFFSET_RTD(sattr->index)) &
+ MODE_RTD_MASK) + 2);
Please split into two patches to simplify review. The changes from
constant to define are logically separate and should thus be in a
separate patch.
Ok, will do.
+ if (index >= 30 && index < 38 && /* local */
+ (reg & MODE_LTD_EN) != MODE_LTD_EN)
This is just a single bit, so "!(reg & MODE_LTD_EN)" is sufficient.
Ack.
+static bool nct7802_get_input_config(struct device *dev,
+ struct device_node *input, u8 *mode_mask, u8 *mode_val)
Please align continuation lines with "(".
Oh, even if that would result in a lot of extra lines? Or just start
the first argument on a new line?
I sincerely doubt that will happen with the 100-column limit,
but yes unless it really doesn't work.
The function should return an error code.
Ok, I'll look into that.
+ if (reg >= 1 && reg <= 3 && !of_device_is_available(input)) {
reg will always be >=1 and <=3 here.
Good catch!
+ *mode_val &= ~(MODE_RTD_MASK
+ << MODE_BIT_OFFSET_RTD(reg-1));
space around '-'
Oh yeah, I'm sorry. Is there a code formatter I should have run? I did
run "checkpatch.pl", hoping that it would catch those.
For some reason checkpatch doesn't always catch this.
+ *mode_mask |= MODE_RTD_MASK
+ << MODE_BIT_OFFSET_RTD(reg-1);
Unnecessary continuation lines. There are several more of those;
I won't comment on it further. Please only use continuation lines if
the resulting line length is otherwise > 100 columns.
Argh, yeah. After refactoring that function, I thought I caught all of
them, but obviously I didn't. According to [1] we should stay within
80 columns (and use tabs that are 8 spaces wide). I assume that still
applies? The rest of this code follows that rule.
From checkpatch, commit bdc48fa11e46 ("checkpatch/coding-style:
deprecate 80-column warning"):
Yes, staying withing 80 columns is certainly still _preferred_. But
it's not the hard limit that the checkpatch warnings imply, and other
concerns can most certainly dominate.
I prefer readability over the 80 column limit.
+ if (dev->of_node) {
+ for_each_child_of_node(dev->of_node, input) {
+ if (nct7802_get_input_config(dev, input, &mode_mask,
+ &mode_val))
+ found_input_config = true;
This is mixing errors with "dt configuration does not exist".
nct7802_get_input_config() should return an actual error if the
DT configuration is bad, and return that error to the calling code
if that is the case.
Ok, I'll change that. I wasn't sure whether we'd rather configure "as
much as we can" or fail completely without configuring anything. Shall
we allow all of the configuration to be validated before erroring out?
No, bail out on the first error.
That would make it easier to get the DT right in one pass, but makes
the code more complicated.
+ if (!found_input_config) {
+ /* Enable local temperature sensor by default */
+ mode_val |= MODE_LTD_EN;
+ mode_mask |= MODE_LTD_EN;
+ }
This can be set by default since nct7802_get_input_config()
removes it if the channel is disabled, meaning found_input_config
is really unnecessary.
Ok. Should we actually phase out the "LTD enabled by default"
completely? Or is that for a future change?
Why ? That would change code behavior and would be unexpected.
Just initialize mode_val and mode_mask variables with MODE_LTD_EN.
Thanks,
Guenter
