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;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <07b2a2f7-5ddc-0f10-6b1f-184dc21fa580@roeck-us.net>
Date:   Wed, 31 May 2023 09:42:48 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Content-Language: en-US
To:     nick.hawkins@hpe.com, verdun@hpe.com, linus.walleij@linaro.org,
        brgl@bgdev.pl, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, jdelvare@suse.com,
        andy.shevchenko@gmail.com, linux-gpio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hwmon@vger.kernel.org
References: <20230531151918.105223-1-nick.hawkins@hpe.com>
 <20230531151918.105223-5-nick.hawkins@hpe.com>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v2 4/5] hwmon: (gxp_fan_ctrl) Provide fan info via gpio
In-Reply-To: <20230531151918.105223-5-nick.hawkins@hpe.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/31/23 08:19, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The fan driver now receives fan data from GPIO via a shared variable.

No, it is not necessary. The driver can and should get the GPIO data using
the gpio API.

> This is a necessity as the Host and the driver need access to the same
> information. Part of the changes includes removing a system power check
> as the GPIO driver needs it to report power state to host.
> 
> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> 
> ---
> 
> v2:
>   *Removed use of shared functions to GPIO in favor of a shared variable
>   *Added build dependency on GXP GPIO driver.
> ---
>   drivers/hwmon/Kconfig        |  2 +-
>   drivers/hwmon/gxp-fan-ctrl.c | 61 +++++-------------------------------
>   drivers/hwmon/gxp-gpio.h     | 13 ++++++++
>   3 files changed, 21 insertions(+), 55 deletions(-)
>   create mode 100644 drivers/hwmon/gxp-gpio.h
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 5b3b76477b0e..5c606bea31f9 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -716,7 +716,7 @@ config SENSORS_GPIO_FAN
>   
>   config SENSORS_GXP_FAN_CTRL
>   	tristate "HPE GXP fan controller"
> -	depends on ARCH_HPE_GXP || COMPILE_TEST
> +	depends on (ARCH_HPE_GXP  && GPIO_GXP_PL) || COMPILE_TEST

Compile test will fail badly unless those external variables
are declared.


>   	help
>   	  If you say yes here you get support for GXP fan control functionality.
>   
> diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c
> index 0014b8b0fd41..8555231328d7 100644
> --- a/drivers/hwmon/gxp-fan-ctrl.c
> +++ b/drivers/hwmon/gxp-fan-ctrl.c
> @@ -1,5 +1,5 @@
>   // SPDX-License-Identifier: GPL-2.0-only
> -/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
> +/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
>   
>   #include <linux/bits.h>
>   #include <linux/err.h>
> @@ -8,51 +8,28 @@
>   #include <linux/module.h>
>   #include <linux/of_device.h>
>   #include <linux/platform_device.h>
> +#include "gxp-gpio.h"
>   
>   #define OFS_FAN_INST	0 /* Is 0 because plreg base will be set at INST */
>   #define OFS_FAN_FAIL	2 /* Is 2 bytes after base */
> -#define OFS_SEVSTAT	0 /* Is 0 because fn2 base will be set at SEVSTAT */
> -#define POWER_BIT	24
>   
>   struct gxp_fan_ctrl_drvdata {
> -	void __iomem	*base;
> -	void __iomem	*plreg;
> -	void __iomem	*fn2;
> +	void __iomem *base;
>   };
>   
>   static bool fan_installed(struct device *dev, int fan)
>   {
> -	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> -	u8 val;
> -
> -	val = readb(drvdata->plreg + OFS_FAN_INST);
> -
> -	return !!(val & BIT(fan));
> +	return !!(fan_presence & BIT(fan));
>   }
>   
>   static long fan_failed(struct device *dev, int fan)
>   {
> -	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> -	u8 val;
> -
> -	val = readb(drvdata->plreg + OFS_FAN_FAIL);
> -
> -	return !!(val & BIT(fan));
> +	return !!(fan_fail & BIT(fan));
>   }
>   
>   static long fan_enabled(struct device *dev, int fan)
>   {
> -	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> -	u32 val;
> -
> -	/*
> -	 * Check the power status as if the platform is off the value
> -	 * reported for the PWM will be incorrect. Report fan as
> -	 * disabled.
> -	 */
> -	val = readl(drvdata->fn2 + OFS_SEVSTAT);
> -
> -	return !!((val & BIT(POWER_BIT)) && fan_installed(dev, fan));
> +	return !!(fan_installed(dev, fan));

Unnecessary () around function call.

>   }
>   
>   static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val)
> @@ -98,20 +75,8 @@ static int gxp_fan_read(struct device *dev, u32 attr, int channel, long *val)
>   static int gxp_pwm_read(struct device *dev, u32 attr, int channel, long *val)
>   {
>   	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> -	u32 reg;
>   
> -	/*
> -	 * Check the power status of the platform. If the platform is off
> -	 * the value reported for the PWM will be incorrect. In this case
> -	 * report a PWM of zero.
> -	 */
> -
> -	reg = readl(drvdata->fn2 + OFS_SEVSTAT);
> -
> -	if (reg & BIT(POWER_BIT))
> -		*val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
> -	else
> -		*val = 0;
> +	*val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
>   
>   	return 0;
>   }
> @@ -212,18 +177,6 @@ static int gxp_fan_ctrl_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, PTR_ERR(drvdata->base),
>   				     "failed to map base\n");
>   
> -	drvdata->plreg = devm_platform_ioremap_resource_byname(pdev,
> -							       "pl");
> -	if (IS_ERR(drvdata->plreg))
> -		return dev_err_probe(dev, PTR_ERR(drvdata->plreg),
> -				     "failed to map plreg\n");
> -
> -	drvdata->fn2 = devm_platform_ioremap_resource_byname(pdev,
> -							     "fn2");
> -	if (IS_ERR(drvdata->fn2))
> -		return dev_err_probe(dev, PTR_ERR(drvdata->fn2),
> -				     "failed to map fn2\n");
> -
>   	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
>   							 "hpe_gxp_fan_ctrl",
>   							 drvdata,
> diff --git a/drivers/hwmon/gxp-gpio.h b/drivers/hwmon/gxp-gpio.h
> new file mode 100644
> index 000000000000..88abe60bbe83
> --- /dev/null
> +++ b/drivers/hwmon/gxp-gpio.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/* Copyright (C) 2023 Hewlett-Packard Enterprise Development Company, L.P. */
> +
> +#ifndef __GPIO_GXP_H__
> +#define __GPIO_GXP_H__
> +
> +#include <linux/err.h>
> +
> +/* Data provided by GPIO */
> +extern u8 fan_presence;
> +extern u8 fan_fail;
> +

This is not acceptable. It is way too generic for a global variable, and it
does not use the gpio API. Besides, the variables would have to be declared
in an include file associated with the code introducing them.

If you want to use gpio pins in the hwmon driver, use the gpio API
([devm_]gpiod_get() and associated functions). There are lots of examples
in the kernel showing how to do that.

Guenter

> +#endif /* __GPIO_GXP_H__ */