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: <56ed0256-c287-cf96-ddff-ba1bd5923814@roeck-us.net>
Date:   Tue, 18 Apr 2023 10:10:37 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v1 2/9] hwmon: (gxp_fan_ctrl) Give GPIO access to fan data
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,
        linux@armlinux.org.uk, linux-gpio@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-hwmon@vger.kernel.org, linux-arm-kernel@lists.infradead.org
References: <20230418152824.110823-1-nick.hawkins@hpe.com>
 <20230418152824.110823-3-nick.hawkins@hpe.com>
From:   Guenter Roeck <linux@roeck-us.net>
In-Reply-To: <20230418152824.110823-3-nick.hawkins@hpe.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 4/18/23 08:28, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> The fan driver has access to the IO that reports fan presence.
> Add the ability for other drivers such as GPIO to call in
> to get fan information. Also remove the system power check as
> the GPIO driver needs it to report power state.
> 

Sorry, I am lost right here. Why would "gpio" want to know about
or report fan information ?

Guenter

> Signed-off-by: Nick Hawkins <nick.hawkins@hpe.com>
> ---
>   drivers/hwmon/gxp-fan-ctrl.c | 58 +++++++++++++++---------------------
>   1 file changed, 24 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c
> index 0014b8b0fd41..a8fcea98cc55 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>
> @@ -11,15 +11,14 @@
>   
>   #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;
>   };
>   
> +struct gxp_fan_ctrl_drvdata *drvdata;
> +
>   static bool fan_installed(struct device *dev, int fan)
>   {
>   	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> @@ -30,6 +29,16 @@ static bool fan_installed(struct device *dev, int fan)
>   	return !!(val & BIT(fan));
>   }
>   
> +u8 get_fans_installed(void)
> +{
> +	static u8 val;
> +
> +	val = readb(drvdata->plreg + OFS_FAN_INST);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(get_fans_installed);
> +
>   static long fan_failed(struct device *dev, int fan)
>   {
>   	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> @@ -40,19 +49,19 @@ static long fan_failed(struct device *dev, int fan)
>   	return !!(val & BIT(fan));
>   }
>   
> -static long fan_enabled(struct device *dev, int fan)
> +u8 get_fans_failed(void)
>   {
> -	struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> -	u32 val;
> +	static u8 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);
> +	val = readb(drvdata->plreg + OFS_FAN_FAIL);
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(get_fans_failed);
>   
> -	return !!((val & BIT(POWER_BIT)) && fan_installed(dev, fan));
> +static long fan_enabled(struct device *dev, int fan)
> +{
> +	return !!(fan_installed(dev, fan));
>   }
>   
>   static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val)
> @@ -98,20 +107,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;
>   }
> @@ -198,7 +195,6 @@ static const struct hwmon_chip_info gxp_fan_ctrl_chip_info = {
>   
>   static int gxp_fan_ctrl_probe(struct platform_device *pdev)
>   {
> -	struct gxp_fan_ctrl_drvdata *drvdata;
>   	struct device *dev = &pdev->dev;
>   	struct device *hwmon_dev;
>   
> @@ -218,12 +214,6 @@ static int gxp_fan_ctrl_probe(struct platform_device *pdev)
>   		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,