Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable. | Date | Sat, 9 Oct 2021 08:50:12 -0700 |
| |
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
| |