Message ID | 20230704131715.44454-6-gregkh@linuxfoundation.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:9f45:0:b0:3ea:f831:8777 with SMTP id v5csp1229106vqx; Tue, 4 Jul 2023 06:38:45 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ5mQJbj2+IJO0Bfpd6OwDwejh543pBKBiLFvrQhK5ESUkxwJFiJcdRfl5XG4ZhQYsXcZFAA X-Received: by 2002:a05:6a20:4325:b0:12c:dac0:a9a4 with SMTP id h37-20020a056a20432500b0012cdac0a9a4mr13352986pzk.5.1688477924777; Tue, 04 Jul 2023 06:38:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1688477924; cv=none; d=google.com; s=arc-20160816; b=W9xQw9LVXxggTBa2pU6wqazLoWJ0KjCRO/sLJywDwmjuYWkeaO765g0yyfmAIbF/tx fBvbYoKgREo1mo+syKWYQB7EL27HxxTvl/2xbpXN0EI51ysks05V5xpO9QZJdHP1YtQX Gcjw9HIpM849C36mmRDEnpXchHI5i8WlT00mJ6WmQ+99UZ/B/kH5i0p2yVFkYlkDB025 7cGaYDLztU9mecH1FlU9UuXNLw/xTnJ3PrJWovkt49qRWy1XGfLQ1w0SVUgl0m7WNEHe J/IvmB8eruF0KV2kvQVh2G+rdfbxPTClYEp2ewP5iOUG5RzeoqWZMnUEaJPQN+kR1OMs Kuug== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=hnrvGD2syj0Xcf3Jax901nblFM0+DjFtPy371PgpeQ0=; fh=H64Y6M62JWA4fTPzNFFUvEDY7mbvNKrFe8iroNBXZTg=; b=IkaA+ccRUXF7TvfGqCbQpjjQHQgeo+OMW8bqR0h7s1MBLcpOtyP8HfkuXq39EhFEf5 unCLIv/IEi9vLB57ZwHt3wAI0hFo9GEwIRqy0Ubp/LC0kat+sd7NS5Sn+zCyuDGmIkeQ saQn65TlkX7N2YapjRvBl69yPsrOE8s4mQ1V6Blj5gBVnSERDeVFK+VQ/i82IrMAn+CD emYMaTszQSxbOfMxRmKkdnKxO46c1I2a32fMrPqpDXsCuSyHX1UWAiUuMTcCaBjd6DxF g1sgDhjeV1ImCkvVat7tUcWiNIBAUWsjL38G1py0Fc8ZzyOLsO4DJog98UkhsiouvBWj T+sA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b=MMkozYtK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id r7-20020aa79887000000b0068259969675si9686691pfl.297.2023.07.04.06.38.29; Tue, 04 Jul 2023 06:38:44 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b=MMkozYtK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230334AbjGDNTA (ORCPT <rfc822;tebrre53rla2o@gmail.com> + 99 others); Tue, 4 Jul 2023 09:19:00 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39218 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231375AbjGDNSd (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 4 Jul 2023 09:18:33 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B218510F5; Tue, 4 Jul 2023 06:18:05 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 8DEB861259; Tue, 4 Jul 2023 13:17:42 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A639AC433C7; Tue, 4 Jul 2023 13:17:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1688476662; bh=9vUu33erJrLKcQGqMGFnvE8WQvh13xozOrZ4w+HY/xI=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=MMkozYtK84+lmlgpbelApGQ2e9158q3VW5g6YbjU2H9bkeVi2unsT+vKksWV6gGRs qwY+HAnOz8ysLFA2ykcZl4sUO41XWUKujKVZBaQ6Kkj8DjnTaBmw3zgi/w6nNxiVZM YhSms/ySgKUNcglef9czFRC0Y2oAz7Q8zdhmb14Q= From: Greg Kroah-Hartman <gregkh@linuxfoundation.org> To: linux-hwmon@vger.kernel.org Cc: linux@roeck-us.net, samsagax@gmail.com, linux-kernel@vger.kernel.org, Greg Kroah-Hartman <gregkh@linuxfoundation.org> Subject: [PATCH 1/3] hwmon: (oxp-sensors): remove static board variable Date: Tue, 4 Jul 2023 14:17:17 +0100 Message-ID: <20230704131715.44454-6-gregkh@linuxfoundation.org> X-Mailer: git-send-email 2.41.0 In-Reply-To: <20230704131715.44454-5-gregkh@linuxfoundation.org> References: <20230704131715.44454-5-gregkh@linuxfoundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 X-Developer-Signature: v=1; a=openpgp-sha256; l=4033; i=gregkh@linuxfoundation.org; h=from:subject; bh=9vUu33erJrLKcQGqMGFnvE8WQvh13xozOrZ4w+HY/xI=; b=owGbwMvMwCRo6H6F97bub03G02pJDClLpO9JLeFbElY84UbQEw37ayfvfYs/X1Htn/5WVXMNa 3MBz/1LHbEsDIJMDLJiiixftvEc3V9xSNHL0PY0zBxWJpAhDFycAjCRv+kM80xt5s5yC9svxt5S /tzk1cX04sLPDxnmqT1Re+wmMlfw54nCxVMOK0jsWJCoBgA= X-Developer-Key: i=gregkh@linuxfoundation.org; a=openpgp; fpr=F4B60CC5BF78C2214A313DCB3147D40DDB2DFB29 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1770497428524634084?= X-GMAIL-MSGID: =?utf-8?q?1770497428524634084?= |
Series |
driver core: remove final user of devm_device_add_groups()
|
|
Commit Message
Greg KH
July 4, 2023, 1:17 p.m. UTC
Drivers should not have a single static variable for the type of device
they are bound to. While this driver is really going to only have one
device at a time in the system, remove the static variable and instead,
look up the device type when needed.
Cc: Joaquín Ignacio Aramendía <samsagax@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hwmon/oxp-sensors.c | 47 ++++++++++++++++++++-----------------
1 file changed, 25 insertions(+), 22 deletions(-)
Comments
On 7/4/23 06:17, Greg Kroah-Hartman wrote: > Drivers should not have a single static variable for the type of device > they are bound to. While this driver is really going to only have one > device at a time in the system, remove the static variable and instead, > look up the device type when needed. > This is expensive. I think it would be much better to just move the board type detection into the init code and not instantiate the driver in the fist place if the board type is unknown. We can handle the static variable separately if it really bothers you that much. > Cc: Joaquín Ignacio Aramendía <samsagax@gmail.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: linux-hwmon@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > --- > drivers/hwmon/oxp-sensors.c | 47 ++++++++++++++++++++----------------- > 1 file changed, 25 insertions(+), 22 deletions(-) > > diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c > index e1a907cae820..3bcba0c476c4 100644 > --- a/drivers/hwmon/oxp-sensors.c > +++ b/drivers/hwmon/oxp-sensors.c > @@ -48,10 +48,9 @@ enum oxp_board { > oxp_mini_amd, > oxp_mini_amd_a07, > oxp_mini_amd_pro, > + UNKNOWN, > }; > > -static enum oxp_board board; > - > /* Fan reading and PWM */ > #define OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */ > #define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */ > @@ -136,6 +135,24 @@ static const struct dmi_system_id dmi_table[] = { > {}, > }; > > +static enum oxp_board get_board_type(void) > +{ > + const struct dmi_system_id *dmi_entry; > + > + /* > + * Have to check for AMD processor here because DMI strings are the > + * same between Intel and AMD boards, the only way to tell them apart > + * is the CPU. > + * Intel boards seem to have different EC registers and values to > + * read/write. > + */ > + dmi_entry = dmi_first_match(dmi_table); > + if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > + return UNKNOWN; > + > + return (enum oxp_board)(unsigned long)dmi_entry->driver_data; > +} > + > /* Helper functions to handle EC read/write */ > static int read_from_ec(u8 reg, int size, long *val) > { > @@ -182,7 +199,7 @@ static int tt_toggle_enable(void) > u8 reg; > u8 val; > > - switch (board) { > + switch (get_board_type()) { > case oxp_mini_amd_a07: > reg = OXP_OLD_TURBO_SWITCH_REG; > val = OXP_OLD_TURBO_TAKE_VAL; > @@ -203,7 +220,7 @@ static int tt_toggle_disable(void) > u8 reg; > u8 val; > > - switch (board) { > + switch (get_board_type()) { > case oxp_mini_amd_a07: > reg = OXP_OLD_TURBO_SWITCH_REG; > val = OXP_OLD_TURBO_RETURN_VAL; > @@ -249,7 +266,7 @@ static ssize_t tt_toggle_show(struct device *dev, > u8 reg; > long val; > > - switch (board) { > + switch (get_board_type()) { > case oxp_mini_amd_a07: > reg = OXP_OLD_TURBO_SWITCH_REG; > break; > @@ -315,7 +332,7 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type, > ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val); > if (ret) > return ret; > - switch (board) { > + switch (get_board_type()) { > case aya_neo_2: > case aya_neo_air: > case aya_neo_air_pro: > @@ -357,7 +374,7 @@ static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type, > case hwmon_pwm_input: > if (val < 0 || val > 255) > return -EINVAL; > - switch (board) { > + switch (get_board_type()) { > case aya_neo_2: > case aya_neo_air: > case aya_neo_air_pro: > @@ -412,25 +429,11 @@ static const struct hwmon_chip_info oxp_ec_chip_info = { > /* Initialization logic */ > static int oxp_platform_probe(struct platform_device *pdev) > { > - const struct dmi_system_id *dmi_entry; > struct device *dev = &pdev->dev; > struct device *hwdev; > int ret; > > - /* > - * Have to check for AMD processor here because DMI strings are the > - * same between Intel and AMD boards, the only way to tell them apart > - * is the CPU. > - * Intel boards seem to have different EC registers and values to > - * read/write. > - */ > - dmi_entry = dmi_first_match(dmi_table); > - if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > - return -ENODEV; > - > - board = (enum oxp_board)(unsigned long)dmi_entry->driver_data; > - > - switch (board) { > + switch (get_board_type()) { This now always registers the hwmon device, which really should not happen. Guenter > case aok_zoe_a1: > case oxp_mini_amd_a07: > case oxp_mini_amd_pro:
On Tue, Jul 04, 2023 at 06:39:07AM -0700, Guenter Roeck wrote: > On 7/4/23 06:17, Greg Kroah-Hartman wrote: > > Drivers should not have a single static variable for the type of device > > they are bound to. While this driver is really going to only have one > > device at a time in the system, remove the static variable and instead, > > look up the device type when needed. > > > > This is expensive. I think it would be much better to just move > the board type detection into the init code and not instantiate > the driver in the fist place if the board type is unknown. The board type detection is all over the place in the driver, it's not just for "unknown" types, so how about just saving the board type at probe time and using it then for all other places? > We can handle the static variable separately if it really bothers > you that much. I did this change to make patch 2/3 more "obvious" what is happening when the in_visible() callback happens, so that you don't have to worry about the saved value or not. But this whole patch isn't really needed if you don't mind the lookup just happening in the in_visible() callback for the first time. thanks, greg k-h
On 7/4/23 06:44, Greg Kroah-Hartman wrote: > On Tue, Jul 04, 2023 at 06:39:07AM -0700, Guenter Roeck wrote: >> On 7/4/23 06:17, Greg Kroah-Hartman wrote: >>> Drivers should not have a single static variable for the type of device >>> they are bound to. While this driver is really going to only have one >>> device at a time in the system, remove the static variable and instead, >>> look up the device type when needed. >>> >> >> This is expensive. I think it would be much better to just move >> the board type detection into the init code and not instantiate >> the driver in the fist place if the board type is unknown. > > The board type detection is all over the place in the driver, it's not > just for "unknown" types, so how about just saving the board type at > probe time and using it then for all other places? > I must be missing something. The current code detects the board type only once, in the probe function. Otherwise the static variable is used. You are replacing it with repeated calls to get_board_type(). The whole point of the static variable is to avoid the cost of repeated calls to dmi_first_match(). >> We can handle the static variable separately if it really bothers >> you that much. > > I did this change to make patch 2/3 more "obvious" what is happening > when the in_visible() callback happens, so that you don't have to worry > about the saved value or not. But this whole patch isn't really needed > if you don't mind the lookup just happening in the in_visible() callback > for the first time. > That would at least be a minimal change, and just add one extra lookup which is only called once (or zero, if it is used to save the board type). As I said, my solution would be to move the board type detection into the init function and not instantiate the driver in the first place if the probe function would bail out anyway. Personally I'd keep the static variable for simplicity, but if you really dislike it that much, we could pass it around in platform and later driver data. But it seems to me that this could (and should) be a separate patch that doesn't have to be hurried in. Thanks, Guenter
On Tue, Jul 04, 2023 at 07:14:54AM -0700, Guenter Roeck wrote: > On 7/4/23 06:44, Greg Kroah-Hartman wrote: > > On Tue, Jul 04, 2023 at 06:39:07AM -0700, Guenter Roeck wrote: > > > On 7/4/23 06:17, Greg Kroah-Hartman wrote: > > > > Drivers should not have a single static variable for the type of device > > > > they are bound to. While this driver is really going to only have one > > > > device at a time in the system, remove the static variable and instead, > > > > look up the device type when needed. > > > > > > > > > > This is expensive. I think it would be much better to just move > > > the board type detection into the init code and not instantiate > > > the driver in the fist place if the board type is unknown. > > > > The board type detection is all over the place in the driver, it's not > > just for "unknown" types, so how about just saving the board type at > > probe time and using it then for all other places? > > > > I must be missing something. The current code detects the board type > only once, in the probe function. Otherwise the static variable is used. > You are replacing it with repeated calls to get_board_type(). > The whole point of the static variable is to avoid the cost of repeated > calls to dmi_first_match(). Ah, ok, yes, I was refering to the fact that the driver relies on the detection of the device type in lots of different places (and doesn't ever error out from the detection call.) > > > We can handle the static variable separately if it really bothers > > > you that much. > > > > I did this change to make patch 2/3 more "obvious" what is happening > > when the in_visible() callback happens, so that you don't have to worry > > about the saved value or not. But this whole patch isn't really needed > > if you don't mind the lookup just happening in the in_visible() callback > > for the first time. > > > > That would at least be a minimal change, and just add one extra lookup > which is only called once (or zero, if it is used to save the board type). Ok, I'll switch it up, but really, it's just a simple table lookup loop, and none of the detection calls are on a "hot path" that I can determine. Or am I missing something? > As I said, my solution would be to move the board type detection > into the init function and not instantiate the driver in the first > place if the probe function would bail out anyway. That's not the case today, the only way the probe function would fail today is if the registering of the sysfs files fail. It does not matter if the board detection call passes or not. > Personally I'd keep > the static variable for simplicity, but if you really dislike it > that much, we could pass it around in platform and later driver data. > But it seems to me that this could (and should) be a separate patch > that doesn't have to be hurried in. Fair enough, let me rework this and resend a v2 series when I get a chance soon. thanks for the review. greg k-h
On 7/4/23 09:14, Greg Kroah-Hartman wrote: > On Tue, Jul 04, 2023 at 07:14:54AM -0700, Guenter Roeck wrote: >> On 7/4/23 06:44, Greg Kroah-Hartman wrote: >>> On Tue, Jul 04, 2023 at 06:39:07AM -0700, Guenter Roeck wrote: >>>> On 7/4/23 06:17, Greg Kroah-Hartman wrote: >>>>> Drivers should not have a single static variable for the type of device >>>>> they are bound to. While this driver is really going to only have one >>>>> device at a time in the system, remove the static variable and instead, >>>>> look up the device type when needed. >>>>> >>>> >>>> This is expensive. I think it would be much better to just move >>>> the board type detection into the init code and not instantiate >>>> the driver in the fist place if the board type is unknown. >>> >>> The board type detection is all over the place in the driver, it's not >>> just for "unknown" types, so how about just saving the board type at >>> probe time and using it then for all other places? >>> >> >> I must be missing something. The current code detects the board type >> only once, in the probe function. Otherwise the static variable is used. >> You are replacing it with repeated calls to get_board_type(). >> The whole point of the static variable is to avoid the cost of repeated >> calls to dmi_first_match(). > > Ah, ok, yes, I was refering to the fact that the driver relies on the > detection of the device type in lots of different places (and doesn't > ever error out from the detection call.) > I am lost again. Current code: dmi_entry = dmi_first_match(dmi_table); if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) return -ENODEV; >>>> We can handle the static variable separately if it really bothers >>>> you that much. >>> >>> I did this change to make patch 2/3 more "obvious" what is happening >>> when the in_visible() callback happens, so that you don't have to worry >>> about the saved value or not. But this whole patch isn't really needed >>> if you don't mind the lookup just happening in the in_visible() callback >>> for the first time. >>> >> >> That would at least be a minimal change, and just add one extra lookup >> which is only called once (or zero, if it is used to save the board type). > > Ok, I'll switch it up, but really, it's just a simple table lookup loop, > and none of the detection calls are on a "hot path" that I can > determine. Or am I missing something? > >> As I said, my solution would be to move the board type detection >> into the init function and not instantiate the driver in the first >> place if the probe function would bail out anyway. > > That's not the case today, the only way the probe function would fail > today is if the registering of the sysfs files fail. It does not matter > if the board detection call passes or not. > Again, dmi_entry = dmi_first_match(dmi_table); if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) return -ENODEV; ^^^^^^^^^^^^^^^ What am I missing ? Guenter
On Tue, Jul 04, 2023 at 09:43:39AM -0700, Guenter Roeck wrote: > On 7/4/23 09:14, Greg Kroah-Hartman wrote: > > On Tue, Jul 04, 2023 at 07:14:54AM -0700, Guenter Roeck wrote: > > > On 7/4/23 06:44, Greg Kroah-Hartman wrote: > > > > On Tue, Jul 04, 2023 at 06:39:07AM -0700, Guenter Roeck wrote: > > > > > On 7/4/23 06:17, Greg Kroah-Hartman wrote: > > > > > > Drivers should not have a single static variable for the type of device > > > > > > they are bound to. While this driver is really going to only have one > > > > > > device at a time in the system, remove the static variable and instead, > > > > > > look up the device type when needed. > > > > > > > > > > > > > > > > This is expensive. I think it would be much better to just move > > > > > the board type detection into the init code and not instantiate > > > > > the driver in the fist place if the board type is unknown. > > > > > > > > The board type detection is all over the place in the driver, it's not > > > > just for "unknown" types, so how about just saving the board type at > > > > probe time and using it then for all other places? > > > > > > > > > > I must be missing something. The current code detects the board type > > > only once, in the probe function. Otherwise the static variable is used. > > > You are replacing it with repeated calls to get_board_type(). > > > The whole point of the static variable is to avoid the cost of repeated > > > calls to dmi_first_match(). > > > > Ah, ok, yes, I was refering to the fact that the driver relies on the > > detection of the device type in lots of different places (and doesn't > > ever error out from the detection call.) > > > > I am lost again. Current code: > > dmi_entry = dmi_first_match(dmi_table); > if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > return -ENODEV; > > > > > > > > We can handle the static variable separately if it really bothers > > > > > you that much. > > > > > > > > I did this change to make patch 2/3 more "obvious" what is happening > > > > when the in_visible() callback happens, so that you don't have to worry > > > > about the saved value or not. But this whole patch isn't really needed > > > > if you don't mind the lookup just happening in the in_visible() callback > > > > for the first time. > > > > > > > > > > That would at least be a minimal change, and just add one extra lookup > > > which is only called once (or zero, if it is used to save the board type). > > > > Ok, I'll switch it up, but really, it's just a simple table lookup loop, > > and none of the detection calls are on a "hot path" that I can > > determine. Or am I missing something? > > > > > As I said, my solution would be to move the board type detection > > > into the init function and not instantiate the driver in the first > > > place if the probe function would bail out anyway. > > > > That's not the case today, the only way the probe function would fail > > today is if the registering of the sysfs files fail. It does not matter > > if the board detection call passes or not. > > > > Again, > dmi_entry = dmi_first_match(dmi_table); > if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) > return -ENODEV; > ^^^^^^^^^^^^^^^ > > What am I missing ? Nothing, I'm missing it, sorry. Been a long day, let me redo this... greg k-h
Hello. > > On 7/4/23 06:17, Greg Kroah-Hartman wrote: > > Drivers should not have a single static variable for the type of device > > they are bound to. While this driver is really going to only have one > > device at a time in the system, remove the static variable and instead, > > look up the device type when needed. > > > > This is expensive. I think it would be much better to just move > the board type detection into the init code and not instantiate > the driver in the fist place if the board type is unknown. I meant to do this, since the probe function is called only once and is meant for hotpluggable things. But since it works as it is and only loads in supported hardware it didn't seem necessary. Modify what you see fit. And thanks for the attention to the quality of the driver. Joaquín
diff --git a/drivers/hwmon/oxp-sensors.c b/drivers/hwmon/oxp-sensors.c index e1a907cae820..3bcba0c476c4 100644 --- a/drivers/hwmon/oxp-sensors.c +++ b/drivers/hwmon/oxp-sensors.c @@ -48,10 +48,9 @@ enum oxp_board { oxp_mini_amd, oxp_mini_amd_a07, oxp_mini_amd_pro, + UNKNOWN, }; -static enum oxp_board board; - /* Fan reading and PWM */ #define OXP_SENSOR_FAN_REG 0x76 /* Fan reading is 2 registers long */ #define OXP_SENSOR_PWM_ENABLE_REG 0x4A /* PWM enable is 1 register long */ @@ -136,6 +135,24 @@ static const struct dmi_system_id dmi_table[] = { {}, }; +static enum oxp_board get_board_type(void) +{ + const struct dmi_system_id *dmi_entry; + + /* + * Have to check for AMD processor here because DMI strings are the + * same between Intel and AMD boards, the only way to tell them apart + * is the CPU. + * Intel boards seem to have different EC registers and values to + * read/write. + */ + dmi_entry = dmi_first_match(dmi_table); + if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) + return UNKNOWN; + + return (enum oxp_board)(unsigned long)dmi_entry->driver_data; +} + /* Helper functions to handle EC read/write */ static int read_from_ec(u8 reg, int size, long *val) { @@ -182,7 +199,7 @@ static int tt_toggle_enable(void) u8 reg; u8 val; - switch (board) { + switch (get_board_type()) { case oxp_mini_amd_a07: reg = OXP_OLD_TURBO_SWITCH_REG; val = OXP_OLD_TURBO_TAKE_VAL; @@ -203,7 +220,7 @@ static int tt_toggle_disable(void) u8 reg; u8 val; - switch (board) { + switch (get_board_type()) { case oxp_mini_amd_a07: reg = OXP_OLD_TURBO_SWITCH_REG; val = OXP_OLD_TURBO_RETURN_VAL; @@ -249,7 +266,7 @@ static ssize_t tt_toggle_show(struct device *dev, u8 reg; long val; - switch (board) { + switch (get_board_type()) { case oxp_mini_amd_a07: reg = OXP_OLD_TURBO_SWITCH_REG; break; @@ -315,7 +332,7 @@ static int oxp_platform_read(struct device *dev, enum hwmon_sensor_types type, ret = read_from_ec(OXP_SENSOR_PWM_REG, 1, val); if (ret) return ret; - switch (board) { + switch (get_board_type()) { case aya_neo_2: case aya_neo_air: case aya_neo_air_pro: @@ -357,7 +374,7 @@ static int oxp_platform_write(struct device *dev, enum hwmon_sensor_types type, case hwmon_pwm_input: if (val < 0 || val > 255) return -EINVAL; - switch (board) { + switch (get_board_type()) { case aya_neo_2: case aya_neo_air: case aya_neo_air_pro: @@ -412,25 +429,11 @@ static const struct hwmon_chip_info oxp_ec_chip_info = { /* Initialization logic */ static int oxp_platform_probe(struct platform_device *pdev) { - const struct dmi_system_id *dmi_entry; struct device *dev = &pdev->dev; struct device *hwdev; int ret; - /* - * Have to check for AMD processor here because DMI strings are the - * same between Intel and AMD boards, the only way to tell them apart - * is the CPU. - * Intel boards seem to have different EC registers and values to - * read/write. - */ - dmi_entry = dmi_first_match(dmi_table); - if (!dmi_entry || boot_cpu_data.x86_vendor != X86_VENDOR_AMD) - return -ENODEV; - - board = (enum oxp_board)(unsigned long)dmi_entry->driver_data; - - switch (board) { + switch (get_board_type()) { case aok_zoe_a1: case oxp_mini_amd_a07: case oxp_mini_amd_pro: