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

[Guenter Roeck]

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