Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
Date:   Fri, 8 Dec 2023 13:06:17 +0200 (EET)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Vishnu Sankar <vishnuocv@gmail.com>
cc:     Hans de Goede <hdegoede@redhat.com>,
        Mark Pearson <mpearson-lenovo@squebb.ca>,
        platform-driver-x86@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>, markgross@kernel.org
Subject: Re: [PATCH] platform/x86: thinkpad_acpi: fix for incorrect fan
 reporting on some ThinkPad systems
In-Reply-To: <2ae27a1b-a472-8a57-994e-f016cc25dafc@linux.intel.com>
Message-ID: <25f86da-2484-3cf9-ae1e-73cac2e6541a@linux.intel.com>
References: <20231206162003.92010-1-vishnuocv@gmail.com> <2ae27a1b-a472-8a57-994e-f016cc25dafc@linux.intel.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="8323329-4976533-1702033581=:1875"
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323329-4976533-1702033581=:1875
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 8BIT

On Fri, 8 Dec 2023, Ilpo J?rvinen wrote:

> On Thu, 7 Dec 2023, Vishnu Sankar wrote:
> 
> Hi Vishnu,
> 
> Thanks for the patch.
> 
> > Some ThinkPad systems ECFW use non-standard addresses for fan control
> > and reporting. This patch adds support for such ECFW so that it can report
> > the correct fan values.
> > Tested on Thinkpads L13 Yoga Gen 2 and X13 Yoga Gen 2.
> > 
> > Co-developed-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> > Signed-off-by: Vishnu Sankar <vishnuocv@gmail.com>

> > +		/* Default mode is AUTO which means controlled by EC */
> > +		if (unlikely(!acpi_ec_read(fan_status_offset_ns, &s)))
> 
> I'm skeptical that all these unlikely/likely() are useful. Some might even 
> be harmful if e.g. is some error condition keeps repeating itself and 
> the particular if handling that is marked with unlikely().
> 
> I know the code in that file is littered with them already but it would 
> be better to add into that, IMO.

So I'm missing the negative here, I meant: ... not add into that ...

-- 
 i.

--8323329-4976533-1702033581=:1875--