Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.

[Guenter Roeck <linux@roeck-us.net>]

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