From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90BCCC4338F for ; Sat, 14 Aug 2021 11:46:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 698E860EB4 for ; Sat, 14 Aug 2021 11:46:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S238227AbhHNLqy (ORCPT ); Sat, 14 Aug 2021 07:46:54 -0400 Received: from out1-smtp.messagingengine.com ([66.111.4.25]:55939 "EHLO out1-smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S238185AbhHNLqx (ORCPT ); Sat, 14 Aug 2021 07:46:53 -0400 Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id 156C05C00E3; Sat, 14 Aug 2021 07:46:25 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute6.internal (MEProxy); Sat, 14 Aug 2021 07:46:25 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-proxy :x-me-proxy:x-me-sender:x-me-sender:x-sasl-enc; s=fm3; bh=QzUIRd aYTQ9jmi1Wxf/eLSw7zIX+FKz+RZjpUwwoIZY=; b=mbnrw8GiRS19Go/KzCOsdQ eV13re8IxqaGsI+bAyAsaaUkIrfrhdikb+ZsJOv9Th3CspqsTE2GDAMhC47dSOQV +da40QCjXu/2QBLJyYlxCdPY3mCDU7E4OhQ/0CXZaEOT7N1iSOsIQYV4GU5FWzmO m0EJ1Sb0zgzH9e9r3DEVKDbaAd8t5CkJlOtW0eaRUaHWa9/wgYCjAV7qX4vinfyr +kvcukdUH5xL4VvqXEVnG/CYHmnDn8qLouSjUzvrO57cHnlh9IdcjWi7W2KfxTh+ cCRW8jmKlcPqNZ4EXLrIXJ6h+Rc1y3OpMnHr2kOKcWAQZ50UzbBQnPxn7T7+UpTg == X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvtddrkeejgdeghecutefuodetggdotefrodftvf curfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfghnecu uegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmdenuc fjughrpeffhffuvffkjghfofggtgesthdtredtredtvdenucfhrhhomhepnfhukhgvucfl ohhnvghsuceolhhukhgvsehljhhonhgvshdruggvvheqnecuggftrfgrthhtvghrnhepgf effedufffhgfeuheegffffgeegveeifeeutefhieejffetudfgueevteehtdetnecuvehl uhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomheplhhukhgvsehljh honhgvshdruggvvh X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Sat, 14 Aug 2021 07:46:20 -0400 (EDT) Date: Sat, 14 Aug 2021 23:46:06 +1200 From: Luke Jones Subject: Re: [PATCH v3 1/1] asus-wmi: Add support for platform_profile To: Andy Shevchenko Cc: Linux Kernel Mailing List , Hans de Goede , Bastien Nocera , Platform Driver Message-Id: In-Reply-To: References: <20210814043103.2535842-1-luke@ljones.dev> <20210814043103.2535842-2-luke@ljones.dev> X-Mailer: geary/40.0 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Precedence: bulk List-ID: X-Mailing-List: platform-driver-x86@vger.kernel.org Hi Andy, thanks for the feedback. All is addressed, and some inline comment/reply. New version incoming pending pr_info() vs dev_info() On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko wrote: > On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones wrote: >> >> Add initial support for platform_profile where the support is >> based on availability of ASUS_THROTTLE_THERMAL_POLICY. >> >> Because throttle_thermal_policy is used by platform_profile and is >> writeable separately to platform_profile any userspace changes to >> throttle_thermal_policy need to notify platform_profile. >> >> In future throttle_thermal_policy sysfs should be removed so that >> only one method controls the laptop power profile. > > Some comments below. > > ... > >> + /* >> + * Ensure that platform_profile updates userspace with the >> change to ensure >> + * that platform_profile and throttle_thermal_policy_mode >> are in sync > > Missed period here and in other multi-line comments. > >> + */ > > ... > >> + /* All possible toggles like throttle_thermal_policy here */ >> + if (asus->throttle_thermal_policy_available) { >> + tp = asus->throttle_thermal_policy_mode; >> + } else { >> + return -1; >> + } >> + >> + if (tp < 0) >> + return tp; > > This will be better in a form > > if (!..._available) > return -ENODATA; // what -1 means? > I wasn't sure what the best return was here. On your suggestion I've changed to ENODATA > tp = ...; > if (tp < 0) > return tp; > > ... > >> + /* All possible toggles like throttle_thermal_policy here */ >> + if (!asus->throttle_thermal_policy_available) { >> + return -1; > > See above. > >> + } > > ... > >> + if (asus->throttle_thermal_policy_available) { >> + pr_info("Using throttle_thermal_policy for >> platform_profile support\n"); > > Why pr_*()? That seemed to be the convention? I see there is also dev_info(), so I've switched to that as it seems more appropriate. > >> + } else { >> + /* >> + * Not an error if a component platform_profile >> relies on is unavailable >> + * so early return, skipping the setup of >> platform_profile. >> + */ >> + return 0; > > Do it other way around, > > /* > * Comment > */ > if (!...) > return 0; Thanks, I think I was transliterating thought process to code as I went along. All updated now. > >> + } > > > -- > With Best Regards, > Andy Shevchenko