diff mbox series

[v2,4/5] hwmon: (gxp_fan_ctrl) Provide fan info via gpio

Message ID 20230531151918.105223-5-nick.hawkins@hpe.com
State New
Headers show
Series ARM: Add GPIO support | expand

Commit Message

Hawkins, Nick May 31, 2023, 3:19 p.m. UTC 
From: Nick Hawkins <nick.hawkins@hpe.com>

The fan driver now receives fan data from GPIO via a shared variable.
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

Comments

Hawkins, Nick June 1, 2023, 3:47 p.m. UTC | #1 
> If the host wants to own the fan status from gpio pins, it has to live up to
> it and own it entirely. The kernel hwmon driver does not have access in that
> case.

> In a more "normal" world, the hwmon driver would "own" the gpio pin(s)
> and user space would listen to associated hwmon attribute events (presumably
> fan_enable and fan_fault), either by listening for sysfs attribute events
> or via udev or both. Again, if you don't want to do that, and want user space
> to have access to the raw gpio pins, you'll have to live with the consequences.
> I don't see the need to bypass existing mechanisms just because user space
> programmers want direct access to gpio pins.

Greetings Guenter,

Thank you for your valuable feedback with the solutions you have provided.
Before I proceed though I have a quick query about the fan driver.
If I were to let the user space "own" gpio pins, would it be permissible for
the userspace to feed a kernel driver data via sysfs?

Ex:
GPIO Driver -> (OpenBMC) -> Fandriver (sysfs).

Here the GPIO driver would provide fan presence information to OpenBMC
and then OpenBMC would provide fan presence info to the fan driver.

If it were permissible to provide data to the driver via this method I could
apply it to the PSU driver as well. the PSU driver which requires presence
info to verify a PSU is inserted / removed.

Thanks,

-Nick Hawkins
Linus Walleij June 1, 2023, 5:11 p.m. UTC | #2 
On Thu, Jun 1, 2023 at 5:48 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:

> Thank you for your valuable feedback with the solutions you have provided.
> Before I proceed though I have a quick query about the fan driver.
> If I were to let the user space "own" gpio pins, would it be permissible for
> the userspace to feed a kernel driver data via sysfs?
>
> Ex:
> GPIO Driver -> (OpenBMC) -> Fandriver (sysfs).
>
> Here the GPIO driver would provide fan presence information to OpenBMC
> and then OpenBMC would provide fan presence info to the fan driver.

But why? Don't be so obsessed about userspace doing stuff using
sysfs, usually it is a better idea to let the kernel handle hardware.

I think this is a simple thermal zone you can define in the device
tree as indicated in my previous comment.

> If it were permissible to provide data to the driver via this method I could
> apply it to the PSU driver as well. the PSU driver which requires presence
> info to verify a PSU is inserted / removed.

It feels like you are looking for a way for two drivers to communicate
with each other.

This can be done several ways, the most straight-forward is notifiers.
include/linux/notifier.h

Yours,
Linus Walleij
Guenter Roeck June 1, 2023, 5:45 p.m. UTC | #3 
On 6/1/23 10:11, Linus Walleij wrote:
> On Thu, Jun 1, 2023 at 5:48 PM Hawkins, Nick <nick.hawkins@hpe.com> wrote:
> 
>> Thank you for your valuable feedback with the solutions you have provided.
>> Before I proceed though I have a quick query about the fan driver.
>> If I were to let the user space "own" gpio pins, would it be permissible for
>> the userspace to feed a kernel driver data via sysfs?
>>
>> Ex:
>> GPIO Driver -> (OpenBMC) -> Fandriver (sysfs).
>>
>> Here the GPIO driver would provide fan presence information to OpenBMC
>> and then OpenBMC would provide fan presence info to the fan driver.
> 
> But why? Don't be so obsessed about userspace doing stuff using
> sysfs, usually it is a better idea to let the kernel handle hardware.
> 
> I think this is a simple thermal zone you can define in the device
> tree as indicated in my previous comment.
> 
>> If it were permissible to provide data to the driver via this method I could
>> apply it to the PSU driver as well. the PSU driver which requires presence
>> info to verify a PSU is inserted / removed.
> 
> It feels like you are looking for a way for two drivers to communicate
> with each other.
> 
> This can be done several ways, the most straight-forward is notifiers.
> include/linux/notifier.h
> 

This is all unnecessary. The hwmon driver could register a gpio pin,
including interrupt, and then report state changes to userspace with
sysfs or udev events on the registered hwmon sysfs attributes.

If they really want to use userspace for everything, they should
just use userspace for everything and not bother with a kernel driver.

Guenter
Hawkins, Nick June 2, 2023, 3:02 p.m. UTC | #4 
> > 
> > This can be done several ways, the most straight-forward is notifiers.
> > include/linux/notifier.h
> > 


> This is all unnecessary. The hwmon driver could register a gpio pin,
> including interrupt, and then report state changes to userspace with
> sysfs or udev events on the registered hwmon sysfs attributes.


> If they really want to use userspace for everything, they should
> just use userspace for everything and not bother with a kernel driver.

Greetings Guenter and Linus,

Thank you for your feedback and assistance. I discussed this with my
team and the direction they are leaning is that they want to own the
GPIOs in user space. The fan driver it would still need to be used
to set and read PWMs as they are kernel protected registers. It will
also need to be there to coordinate the proper offset in the GXP
registers to control a particular fans PWM.

For hot pluggable devices such as fans and psu they will need to
bind/unbind the hwmon driver of the device as it is inserted/removed.

Is this an acceptable path forward?

If it is I will revise this patchset once more to make the fan independent
of the GPIO driver.

Thanks again for all the guidance,

-Nick Hawkins
diff mbox series

Patch

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
 	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));
 }
 
 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;
+
+#endif /* __GPIO_GXP_H__ */