Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211009025858.3326725-1-osk@google.com> <20211009025858.3326725-2-osk@google.com>
 <0674a2d0-f0a2-d6bc-33e3-483614602bae@roeck-us.net>
In-Reply-To: <0674a2d0-f0a2-d6bc-33e3-483614602bae@roeck-us.net>
From:   Oskar Senft <osk@google.com>
Date:   Sat, 9 Oct 2021 10:50:16 -0400
Message-ID: <CABoTLcTL42a23=P501UoqNWr76A3fmEoxwjymz1-g0MNMyYPRA@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] hwmon: (nct7802) Make temperature sensors configurable.
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     Jean Delvare <jdelvare@suse.com>, Rob Herring <robh+dt@kernel.org>,
        linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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?

> 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.

> > +             *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.

> > +     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?
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?

Thanks
Oskar.