lkml.org 
[lkml]   [2021]   [Oct]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.
Date
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

\
 
 \ /
  Last update: 2021-10-09 17:50    [W:0.046 / U:0.888 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site