Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <6ef88c2c-3435-8847-0f4f-53e81cd3af5d@gmail.com>
Date:   Sun, 23 Oct 2022 15:15:36 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH] hwmon: (aquacomputer_d5next) Add support for temperature
 sensor offsets
Content-Language: en-US
To:     Guenter Roeck <linux@roeck-us.net>
Cc:     linux-hwmon@vger.kernel.org, leonard.anderweit@gmail.com,
        Jack Doan <me@jackdoan.com>, Jean Delvare <jdelvare@suse.com>,
        Jonathan Corbet <corbet@lwn.net>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20221021174834.736930-1-savicaleksa83@gmail.com>
 <20221022135720.GA739650@roeck-us.net>
From:   Aleksa Savic <savicaleksa83@gmail.com>
In-Reply-To: <20221022135720.GA739650@roeck-us.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022-10-22 15:57:20 GMT+02:00, Guenter Roeck wrote:
> Please go up to 100 columns to avoid excessive line splits.

Will fix this and other comments in v2.

> Is it really necessary to re-read the control buffer repeatedly
> to report this value ? I don't know how costly that is, but unlike
> the pwm value I would not expect the number to change.

Yes, aside from the driver userspace can also change settings on the
device using hidraw and we'd end up with stale data. Reading it is
very fast, it takes about 4ms in my testing.

> Also, is this number indeed not included in the regular reports
> sent from the controller ?

Unfortunately, it's not. The sensor report only includes final (calculated)
sensor readings.

> The driver doesn't distinguish between offsets in the control buffer
> (pwm, and now temperature sensor offset) and offsets in the report buffer,
> making it a bit difficult to determine if those are the same or not.
> Some explanation in the driver would be nice if someone finds the time
> to provide one. If the control buffer offsets are in a different number
> space, they should really be marked accordingly (for example with a
> _CTRL in the define).

I can see how it can be confusing. After this, I can send a patch to
reorder the macros & initializations and add more comments regarding
what is what.

Thanks,
Aleksa