From: Nick Hawkins <[email protected]>
The GXP SoC can support up to 16 fans through the interface provided by
the CPLD. The fans speeds are controlled via a pwm value 0-255. The fans
are also capable of reporting if they have failed to the CPLD which in
turn reports the status to the GXP SoC. Based on previous feedback the
registers required for fan control have been regmaped individualy to fan
driver. Specifically these registers are the function 2 registers and the
programmable logic registers from the CPLD. Additionally in this patchset
there is support for the SPI driver which already exists as spi-gxp.c in
the SPI driver.
---
Changes since v1:
*Renamed fn2reg to fn2 in dtsi file and documentation
*Renamed plreg to pl in dtsi file and documentation
*Renamed fanctrl to fan-controller in dtsi file and documentation
*Adjusted base register range for fan ctrl in dtsi
*Changed commit description on fan-ctrl device-tree binding
*Changed register description on fan-ctrl device-tree binding
*Changed number of supported fans from 16 to 8 in driver code and
documentation
*Modified commit description of fan code
*Removed support for fan[0-15]_input
*Removed PWM defines in driver code
*Added gxp-fan-ctrl to hwmon's index.rst
*Removed mutex in driver code
*Added fan_enable support in fan code and documentation
*Fixed comment in driver code presents -> present
*Removed unecessary include files in fan code
*Added comments to describe what power state is and
calculations for accessing plreg in fan code
*Removed use of variable offsets in fan code
*Fixed GPL header in fan code
*Changed module description for fan controller
*Added kfree in case of failure to initialize driver
*Added missing yaml file to MAINTAINERS
Nick Hawkins (6):
hwmon: (gxp-fan-ctrl) Add GXP fan controller
ABI: sysfs-class-hwmon: add a description for fanY_fault
dt-bindings: hwmon: Add hpe,gxp-fan-ctrl
ARM: dts: add GXP Support for fans and SPI
ARM: multi_v7_defconfig: Add GXP Fan and SPI support
MAINTAINERS: add gxp fan controller and documents
Documentation/ABI/testing/sysfs-class-hwmon | 9 +
.../bindings/hwmon/hpe,gxp-fan-ctrl.yaml | 41 +++
Documentation/hwmon/gxp-fan-ctrl.rst | 28 ++
Documentation/hwmon/index.rst | 1 +
MAINTAINERS | 3 +
arch/arm/boot/dts/hpe-bmc-dl360gen10.dts | 58 ++++
arch/arm/boot/dts/hpe-gxp.dtsi | 64 ++--
arch/arm/configs/multi_v7_defconfig | 2 +
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/gxp-fan-ctrl.c | 305 ++++++++++++++++++
11 files changed, 502 insertions(+), 19 deletions(-)
create mode 100644 Documentation/devicetree/bindings/hwmon/hpe,gxp-fan-ctrl.yaml
create mode 100644 Documentation/hwmon/gxp-fan-ctrl.rst
create mode 100644 drivers/hwmon/gxp-fan-ctrl.c
--
2.17.1
From: Nick Hawkins <[email protected]>
In order for HPE platforms to be supported by linux on GXP it is
necessary for there to be fan and spi driver support. There fan driver
can support up to 16 fans that are driven by pwm through the CPLD. The
SPI driver supports access to the core flash and bios part. The SPI
driver spi-gxp was added previously to linux.
Signed-off-by: Nick Hawkins <[email protected]>
---
v2:
*No change
---
arch/arm/configs/multi_v7_defconfig | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/configs/multi_v7_defconfig b/arch/arm/configs/multi_v7_defconfig
index 12b35008571f..92a854e7b01b 100644
--- a/arch/arm/configs/multi_v7_defconfig
+++ b/arch/arm/configs/multi_v7_defconfig
@@ -445,6 +445,7 @@ CONFIG_SPI_CADENCE=y
CONFIG_SPI_DAVINCI=y
CONFIG_SPI_FSL_QUADSPI=m
CONFIG_SPI_GPIO=m
+CONFIG_SPI_GXP=m
CONFIG_SPI_FSL_DSPI=m
CONFIG_SPI_OMAP24XX=y
CONFIG_SPI_ORION=y
@@ -535,6 +536,7 @@ CONFIG_SENSORS_NTC_THERMISTOR=m
CONFIG_SENSORS_PWM_FAN=m
CONFIG_SENSORS_RASPBERRYPI_HWMON=m
CONFIG_SENSORS_INA2XX=m
+CONFIG_SENSORS_GXP_FAN_CTRL=m
CONFIG_CPU_THERMAL=y
CONFIG_DEVFREQ_THERMAL=y
CONFIG_IMX_THERMAL=y
--
2.17.1
From: Nick Hawkins <[email protected]>
The GXP SoC can support up to 16 fans through the interface provided by
the CPLD. The current support is limited to 8 fans. The fans speeds are
controlled via 8 different PWMs which can vary in value from 0-255. The
fans are also capable of reporting if they have failed to the CPLD which
in turn reports the status to the GXP SoC.
Signed-off-by: Nick Hawkins <[email protected]>
---
v2:
*Changed number of supported fans from 16 to 8 in code
*Remove last sentence of commit description
*Removed support for fan[0-15]_input in code and documentation
*Changed documentation to limit fan count to 7
*Changed documentation license
*Removed PWM defines
*Added gxp-fan-ctrl to hwmon's index.rst
*Removed mutex
*Added fan_enable support to report if the fan is enabled
*Changed presents to present
*Removed unnecessary ()
*Add comment for plreg reads and calculations
*Add comment for the use of platform power state in code
*Removed use of variable offsets and went with hardcoding instead
*Rewrote driver to use devm_hwmon_device_register_with_info()
*Remove unused header files
*Fix GPL header
*Changed module description
*Add kfree in case of failure to get regmaps or resource
---
Documentation/hwmon/gxp-fan-ctrl.rst | 28 +++
Documentation/hwmon/index.rst | 1 +
drivers/hwmon/Kconfig | 9 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/gxp-fan-ctrl.c | 305 +++++++++++++++++++++++++++
5 files changed, 344 insertions(+)
create mode 100644 Documentation/hwmon/gxp-fan-ctrl.rst
create mode 100644 drivers/hwmon/gxp-fan-ctrl.c
diff --git a/Documentation/hwmon/gxp-fan-ctrl.rst b/Documentation/hwmon/gxp-fan-ctrl.rst
new file mode 100644
index 000000000000..ae3397e81c04
--- /dev/null
+++ b/Documentation/hwmon/gxp-fan-ctrl.rst
@@ -0,0 +1,28 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Kernel driver gxp-fan-ctrl
+==========================
+
+Supported chips:
+
+ * HPE GXP SOC
+
+Author: Nick Hawkins <[email protected]>
+
+
+Description
+-----------
+
+gxp-fan-ctrl is a driver which provides fan control for the hpe gxp soc.
+The driver allows the gathering of fan status and the use of fan
+PWM control.
+
+
+Sysfs attributes
+----------------
+
+======================= ===========================================================
+pwm[0-7] Fan 0 to 7 respective PWM value (0-255)
+fan[0-7]_fault Fan 0 to 7 respective fault status: 1 fail, 0 ok
+fan[0-7]_enable Fan 0 to 7 respective enabled status: 1 enabled, 0 disabled
+======================= ===========================================================
diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index f7113b0f8b2a..b319ab173d1d 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -73,6 +73,7 @@ Hardware Monitoring Kernel Drivers
g762
gsc-hwmon
gl518sm
+ gxp-fan-ctrl
hih6130
ibmaem
ibm-cffps
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index e70d9614bec2..9e0427f20141 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -705,6 +705,15 @@ config SENSORS_GPIO_FAN
This driver can also be built as a module. If so, the module
will be called gpio-fan.
+config SENSORS_GXP_FAN_CTRL
+ tristate "HPE GXP fan controller"
+ depends on ARCH_HPE_GXP || COMPILE_TEST
+ help
+ If you say yes here you get support for GXP fan control functionality.
+
+ The GXP controls fan function via the CPLD through the use of PWM
+ registers. This driver reports status and pwm setting of the fans.
+
config SENSORS_HIH6130
tristate "Honeywell Humidicon HIH-6130 humidity/temperature sensor"
depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 007e829d1d0d..b474dcc708c4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
obj-$(CONFIG_SENSORS_GSC) += gsc-hwmon.o
obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
+obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o
obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
obj-$(CONFIG_SENSORS_I5500) += i5500_temp.o
diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c
new file mode 100644
index 000000000000..0b03d33a3a7b
--- /dev/null
+++ b/drivers/hwmon/gxp-fan-ctrl.c
@@ -0,0 +1,305 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
+
+#include <linux/err.h>
+#include <linux/hwmon.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/slab.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 {
+ struct device *dev;
+ struct device *hwmon_dev;
+ struct regmap *plreg_map; /* Programmable logic register regmap */
+ struct regmap *fn2_map; /* Function 2 regmap */
+ void __iomem *base;
+};
+
+static bool fan_installed(struct device *dev, int fan)
+{
+ struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+ u32 val;
+
+ regmap_read(drvdata->plreg_map, OFS_FAN_INST, &val);
+ if (val & BIT(fan))
+ return 1;
+ else
+ return 0;
+}
+
+static long fan_failed(struct device *dev, int fan)
+{
+ struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+ u32 val;
+
+ /*
+ * The offset for fan fail is 2 which is not word aligned.
+ * Read from fan installed which is 0 and shift value.
+ */
+
+ regmap_read(drvdata->plreg_map, OFS_FAN_INST, &val);
+
+ if ((val >> (8 * OFS_FAN_FAIL)) & BIT(fan))
+ return 1;
+ else
+ return 0;
+}
+
+static long fan_enabled(struct device *dev, int fan)
+{
+ struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+ unsigned int reg;
+
+ /*
+ * Check the power status as if the platform is off the value
+ * reported for the PWM will be incorrect. Report fan as
+ * disabled.
+ */
+ regmap_read(drvdata->fn2_map, OFS_SEVSTAT, ®);
+
+ /* If Fan is installed and the system is on it is enabled */
+ if ((reg & BIT(POWER_BIT)) && fan_installed(dev, fan))
+ return 1;
+
+ /* Platform power is off, fan is disabled */
+ return 0;
+}
+
+static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val)
+{
+ struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+
+ switch (attr) {
+ case hwmon_pwm_input:
+ if (val > 255)
+ return -EINVAL;
+ writeb(val, drvdata->base + channel);
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int gxp_fan_ctrl_write(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long val)
+{
+ switch (type) {
+ case hwmon_pwm:
+ return gxp_pwm_write(dev, attr, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int gxp_fan_read(struct device *dev, u32 attr, int channel, long *val)
+{
+ switch (attr) {
+ case hwmon_fan_enable:
+ *val = fan_enabled(dev, channel);
+ return 0;
+ case hwmon_fan_fault:
+ *val = fan_failed(dev, channel);
+ return 0;
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static int gxp_pwm_read(struct device *dev, u32 attr, int channel, long *val)
+{
+ struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
+ unsigned int 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.
+ */
+ regmap_read(drvdata->fn2_map, 0, ®);
+ if (reg & BIT(POWER_BIT)) {
+ /* If Fan present, then read it. */
+ *val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
+ } else {
+ *val = 0;
+ }
+
+ return 0;
+}
+
+static int gxp_fan_ctrl_read(struct device *dev, enum hwmon_sensor_types type,
+ u32 attr, int channel, long *val)
+{
+ switch (type) {
+ case hwmon_fan:
+ return gxp_fan_read(dev, attr, channel, val);
+ case hwmon_pwm:
+ return gxp_pwm_read(dev, attr, channel, val);
+ default:
+ return -EOPNOTSUPP;
+ }
+}
+
+static umode_t gxp_fan_ctrl_is_visible(const void *_data,
+ enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+ umode_t mode = 0;
+
+ switch (type) {
+ case hwmon_fan:
+ switch (attr) {
+ case hwmon_fan_enable:
+ case hwmon_fan_fault:
+ mode = 0444;
+ default:
+ break;
+ }
+ case hwmon_pwm:
+ switch (attr) {
+ case hwmon_pwm_input:
+ mode = 0644;
+ default:
+ break;
+ }
+ default:
+ break;
+ }
+
+ return mode;
+}
+
+static const struct hwmon_ops gxp_fan_ctrl_ops = {
+ .is_visible = gxp_fan_ctrl_is_visible,
+ .read = gxp_fan_ctrl_read,
+ .write = gxp_fan_ctrl_write,
+};
+
+static const struct hwmon_channel_info *gxp_fan_ctrl_info[] = {
+ HWMON_CHANNEL_INFO(fan,
+ HWMON_F_FAULT | HWMON_F_ENABLE,
+ HWMON_F_FAULT | HWMON_F_ENABLE,
+ HWMON_F_FAULT | HWMON_F_ENABLE,
+ HWMON_F_FAULT | HWMON_F_ENABLE,
+ HWMON_F_FAULT | HWMON_F_ENABLE,
+ HWMON_F_FAULT | HWMON_F_ENABLE,
+ HWMON_F_FAULT | HWMON_F_ENABLE,
+ HWMON_F_FAULT | HWMON_F_ENABLE),
+ HWMON_CHANNEL_INFO(pwm,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
+ HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
+ NULL
+};
+
+static const struct hwmon_chip_info gxp_fan_ctrl_chip_info = {
+ .ops = &gxp_fan_ctrl_ops,
+ .info = gxp_fan_ctrl_info,
+
+};
+
+static struct regmap *gxp_fan_ctrl_init_regmap(struct platform_device *pdev, char *reg_name)
+{
+ struct regmap_config regmap_config = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ };
+ void __iomem *base;
+
+ base = devm_platform_ioremap_resource_byname(pdev, reg_name);
+ if (IS_ERR(base))
+ return ERR_CAST(base);
+
+ regmap_config.name = reg_name;
+
+ return devm_regmap_init_mmio(&pdev->dev, base, ®map_config);
+}
+
+static int gxp_fan_ctrl_probe(struct platform_device *pdev)
+{
+ struct gxp_fan_ctrl_drvdata *drvdata;
+ struct resource *res;
+ struct device *dev = &pdev->dev;
+ int error;
+
+ drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_fan_ctrl_drvdata),
+ GFP_KERNEL);
+ if (!drvdata)
+ return -ENOMEM;
+
+ drvdata->dev = &pdev->dev;
+
+ platform_set_drvdata(pdev, drvdata);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ drvdata->base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(drvdata->base)) {
+ error = dev_err_probe(dev, PTR_ERR(drvdata->base),
+ "failed to map base\n");
+ goto free_mem;
+ }
+ drvdata->plreg_map = gxp_fan_ctrl_init_regmap(pdev, "pl");
+ if (IS_ERR(drvdata->plreg_map)) {
+ error = dev_err_probe(dev, PTR_ERR(drvdata->plreg_map),
+ "failed to map plreg_handle\n");
+ goto free_mem;
+ }
+
+ drvdata->fn2_map = gxp_fan_ctrl_init_regmap(pdev, "fn2");
+ if (IS_ERR(drvdata->fn2_map)) {
+ error = dev_err_probe(dev, PTR_ERR(drvdata->fn2_map),
+ "failed to map fn2_handle\n");
+ goto free_mem;
+ }
+
+ drvdata->hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
+ "fan_ctrl",
+ drvdata,
+ &gxp_fan_ctrl_chip_info,
+ NULL);
+
+ if (IS_ERR(drvdata->hwmon_dev)) {
+ error = dev_err_probe(dev, PTR_ERR(drvdata->hwmon_dev),
+ "failed to register fan ctrl\n");
+
+ goto free_mem;
+ }
+
+ return 0;
+
+free_mem:
+ kfree(drvdata);
+ return error;
+}
+
+static const struct of_device_id gxp_fan_ctrl_of_match[] = {
+ { .compatible = "hpe,gxp-fan-ctrl", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, gxp_fan_ctrl_of_match);
+
+static struct platform_driver gxp_fan_ctrl_driver = {
+ .probe = gxp_fan_ctrl_probe,
+ .driver = {
+ .name = "gxp-fan-ctrl",
+ .of_match_table = gxp_fan_ctrl_of_match,
+ },
+};
+module_platform_driver(gxp_fan_ctrl_driver);
+
+MODULE_AUTHOR("Nick Hawkins <[email protected]>");
+MODULE_DESCRIPTION("HPE GXP fan controller");
+MODULE_LICENSE("GPL");
--
2.17.1
On Mon, Nov 28, 2022 at 05:02:14PM -0600, [email protected] wrote:
> From: Nick Hawkins <[email protected]>
>
> The GXP SoC can support up to 16 fans through the interface provided by
> the CPLD. The current support is limited to 8 fans. The fans speeds are
> controlled via 8 different PWMs which can vary in value from 0-255. The
> fans are also capable of reporting if they have failed to the CPLD which
> in turn reports the status to the GXP SoC.
>
> Signed-off-by: Nick Hawkins <[email protected]>
> ---
>
> v2:
> *Changed number of supported fans from 16 to 8 in code
> *Remove last sentence of commit description
> *Removed support for fan[0-15]_input in code and documentation
> *Changed documentation to limit fan count to 7
> *Changed documentation license
> *Removed PWM defines
> *Added gxp-fan-ctrl to hwmon's index.rst
> *Removed mutex
> *Added fan_enable support to report if the fan is enabled
> *Changed presents to present
> *Removed unnecessary ()
> *Add comment for plreg reads and calculations
> *Add comment for the use of platform power state in code
> *Removed use of variable offsets and went with hardcoding instead
> *Rewrote driver to use devm_hwmon_device_register_with_info()
> *Remove unused header files
> *Fix GPL header
> *Changed module description
> *Add kfree in case of failure to get regmaps or resource
> ---
> Documentation/hwmon/gxp-fan-ctrl.rst | 28 +++
> Documentation/hwmon/index.rst | 1 +
> drivers/hwmon/Kconfig | 9 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/gxp-fan-ctrl.c | 305 +++++++++++++++++++++++++++
> 5 files changed, 344 insertions(+)
> create mode 100644 Documentation/hwmon/gxp-fan-ctrl.rst
> create mode 100644 drivers/hwmon/gxp-fan-ctrl.c
>
> diff --git a/Documentation/hwmon/gxp-fan-ctrl.rst b/Documentation/hwmon/gxp-fan-ctrl.rst
> new file mode 100644
> index 000000000000..ae3397e81c04
> --- /dev/null
> +++ b/Documentation/hwmon/gxp-fan-ctrl.rst
> @@ -0,0 +1,28 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +Kernel driver gxp-fan-ctrl
> +==========================
> +
> +Supported chips:
> +
> + * HPE GXP SOC
> +
> +Author: Nick Hawkins <[email protected]>
> +
> +
> +Description
> +-----------
> +
> +gxp-fan-ctrl is a driver which provides fan control for the hpe gxp soc.
> +The driver allows the gathering of fan status and the use of fan
> +PWM control.
> +
> +
> +Sysfs attributes
> +----------------
> +
> +======================= ===========================================================
> +pwm[0-7] Fan 0 to 7 respective PWM value (0-255)
> +fan[0-7]_fault Fan 0 to 7 respective fault status: 1 fail, 0 ok
> +fan[0-7]_enable Fan 0 to 7 respective enabled status: 1 enabled, 0 disabled
> +======================= ===========================================================
> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
> index f7113b0f8b2a..b319ab173d1d 100644
> --- a/Documentation/hwmon/index.rst
> +++ b/Documentation/hwmon/index.rst
> @@ -73,6 +73,7 @@ Hardware Monitoring Kernel Drivers
> g762
> gsc-hwmon
> gl518sm
> + gxp-fan-ctrl
> hih6130
> ibmaem
> ibm-cffps
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index e70d9614bec2..9e0427f20141 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -705,6 +705,15 @@ config SENSORS_GPIO_FAN
> This driver can also be built as a module. If so, the module
> will be called gpio-fan.
>
> +config SENSORS_GXP_FAN_CTRL
> + tristate "HPE GXP fan controller"
> + depends on ARCH_HPE_GXP || COMPILE_TEST
> + help
> + If you say yes here you get support for GXP fan control functionality.
> +
> + The GXP controls fan function via the CPLD through the use of PWM
> + registers. This driver reports status and pwm setting of the fans.
> +
> config SENSORS_HIH6130
> tristate "Honeywell Humidicon HIH-6130 humidity/temperature sensor"
> depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 007e829d1d0d..b474dcc708c4 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -83,6 +83,7 @@ obj-$(CONFIG_SENSORS_GL518SM) += gl518sm.o
> obj-$(CONFIG_SENSORS_GL520SM) += gl520sm.o
> obj-$(CONFIG_SENSORS_GSC) += gsc-hwmon.o
> obj-$(CONFIG_SENSORS_GPIO_FAN) += gpio-fan.o
> +obj-$(CONFIG_SENSORS_GXP_FAN_CTRL) += gxp-fan-ctrl.o
> obj-$(CONFIG_SENSORS_HIH6130) += hih6130.o
> obj-$(CONFIG_SENSORS_ULTRA45) += ultra45_env.o
> obj-$(CONFIG_SENSORS_I5500) += i5500_temp.o
> diff --git a/drivers/hwmon/gxp-fan-ctrl.c b/drivers/hwmon/gxp-fan-ctrl.c
> new file mode 100644
> index 000000000000..0b03d33a3a7b
> --- /dev/null
> +++ b/drivers/hwmon/gxp-fan-ctrl.c
> @@ -0,0 +1,305 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2022 Hewlett-Packard Enterprise Development Company, L.P. */
> +
> +#include <linux/err.h>
> +#include <linux/hwmon.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.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 {
> + struct device *dev;
> + struct device *hwmon_dev;
Both dev and hwmon_dev are unused and thus pointless.
> + struct regmap *plreg_map; /* Programmable logic register regmap */
> + struct regmap *fn2_map; /* Function 2 regmap */
> + void __iomem *base;
> +};
> +
> +static bool fan_installed(struct device *dev, int fan)
> +{
> + struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> + u32 val;
> +
> + regmap_read(drvdata->plreg_map, OFS_FAN_INST, &val);
> + if (val & BIT(fan))
> + return 1;
> + else
> + return 0;
else after return is unnecessary, and
return !!(val & BIT(fan));
would avoid the conditional.
> +}
> +
> +static long fan_failed(struct device *dev, int fan)
> +{
> + struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> + u32 val;
> +
> + /*
> + * The offset for fan fail is 2 which is not word aligned.
> + * Read from fan installed which is 0 and shift value.
> + */
> +
> + regmap_read(drvdata->plreg_map, OFS_FAN_INST, &val);
> +
> + if ((val >> (8 * OFS_FAN_FAIL)) & BIT(fan))
> + return 1;
> + else
> + return 0;
else after return is pointless, and this can be written as
return !!((val >> (8 * OFS_FAN_FAIL)) & BIT(fan));
to avoid the conditional.
> +}
> +
> +static long fan_enabled(struct device *dev, int fan)
> +{
> + struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> + unsigned int reg;
> +
> + /*
> + * Check the power status as if the platform is off the value
> + * reported for the PWM will be incorrect. Report fan as
> + * disabled.
> + */
> + regmap_read(drvdata->fn2_map, OFS_SEVSTAT, ®);
> +
> + /* If Fan is installed and the system is on it is enabled */
> + if ((reg & BIT(POWER_BIT)) && fan_installed(dev, fan))
> + return 1;
> +
> + /* Platform power is off, fan is disabled */
> + return 0;
Same as above.
> +}
> +
> +static int gxp_pwm_write(struct device *dev, u32 attr, int channel, long val)
> +{
> + struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> +
> + switch (attr) {
> + case hwmon_pwm_input:
> + if (val > 255)
> + return -EINVAL;
Should also check for values < 0.
> + writeb(val, drvdata->base + channel);
The mixed use of direct writes and regmap is odd and confusing.
Why use regmap for plreg_map and for fn2_map but not for base ?
Can this be unified ? If not, why ?
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int gxp_fan_ctrl_write(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long val)
> +{
> + switch (type) {
> + case hwmon_pwm:
> + return gxp_pwm_write(dev, attr, channel, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int gxp_fan_read(struct device *dev, u32 attr, int channel, long *val)
> +{
> + switch (attr) {
> + case hwmon_fan_enable:
> + *val = fan_enabled(dev, channel);
> + return 0;
> + case hwmon_fan_fault:
> + *val = fan_failed(dev, channel);
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static int gxp_pwm_read(struct device *dev, u32 attr, int channel, long *val)
> +{
> + struct gxp_fan_ctrl_drvdata *drvdata = dev_get_drvdata(dev);
> + unsigned int 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.
> + */
> + regmap_read(drvdata->fn2_map, 0, ®);
> + if (reg & BIT(POWER_BIT)) {
> + /* If Fan present, then read it. */
> + *val = fan_installed(dev, channel) ? readb(drvdata->base + channel) : 0;
> + } else {
> + *val = 0;
> + }
> +
> + return 0;
> +}
> +
> +static int gxp_fan_ctrl_read(struct device *dev, enum hwmon_sensor_types type,
> + u32 attr, int channel, long *val)
> +{
> + switch (type) {
> + case hwmon_fan:
> + return gxp_fan_read(dev, attr, channel, val);
> + case hwmon_pwm:
> + return gxp_pwm_read(dev, attr, channel, val);
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
> +
> +static umode_t gxp_fan_ctrl_is_visible(const void *_data,
> + enum hwmon_sensor_types type,
> + u32 attr, int channel)
> +{
> + umode_t mode = 0;
> +
> + switch (type) {
> + case hwmon_fan:
> + switch (attr) {
> + case hwmon_fan_enable:
> + case hwmon_fan_fault:
> + mode = 0444;
break; missing. Otherwise static analyzers will complain.
> + default:
> + break;
> + }
Same as above (and, in this case, prove in point why break;
should always be added even if it seems unnecessary).
> + case hwmon_pwm:
> + switch (attr) {
> + case hwmon_pwm_input:
> + mode = 0644;
Same as above.
> + default:
> + break;
> + }
Same as above.
> + default:
> + break;
> + }
> +
> + return mode;
> +}
> +
> +static const struct hwmon_ops gxp_fan_ctrl_ops = {
> + .is_visible = gxp_fan_ctrl_is_visible,
> + .read = gxp_fan_ctrl_read,
> + .write = gxp_fan_ctrl_write,
> +};
> +
> +static const struct hwmon_channel_info *gxp_fan_ctrl_info[] = {
> + HWMON_CHANNEL_INFO(fan,
> + HWMON_F_FAULT | HWMON_F_ENABLE,
> + HWMON_F_FAULT | HWMON_F_ENABLE,
> + HWMON_F_FAULT | HWMON_F_ENABLE,
> + HWMON_F_FAULT | HWMON_F_ENABLE,
> + HWMON_F_FAULT | HWMON_F_ENABLE,
> + HWMON_F_FAULT | HWMON_F_ENABLE,
> + HWMON_F_FAULT | HWMON_F_ENABLE,
> + HWMON_F_FAULT | HWMON_F_ENABLE),
> + HWMON_CHANNEL_INFO(pwm,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
> + HWMON_PWM_INPUT | HWMON_PWM_ENABLE),
> + NULL
> +};
> +
> +static const struct hwmon_chip_info gxp_fan_ctrl_chip_info = {
> + .ops = &gxp_fan_ctrl_ops,
> + .info = gxp_fan_ctrl_info,
> +
> +};
> +
> +static struct regmap *gxp_fan_ctrl_init_regmap(struct platform_device *pdev, char *reg_name)
> +{
> + struct regmap_config regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
> + };
> + void __iomem *base;
> +
> + base = devm_platform_ioremap_resource_byname(pdev, reg_name);
> + if (IS_ERR(base))
> + return ERR_CAST(base);
> +
> + regmap_config.name = reg_name;
> +
> + return devm_regmap_init_mmio(&pdev->dev, base, ®map_config);
> +}
> +
> +static int gxp_fan_ctrl_probe(struct platform_device *pdev)
> +{
> + struct gxp_fan_ctrl_drvdata *drvdata;
> + struct resource *res;
> + struct device *dev = &pdev->dev;
> + int error;
> +
> + drvdata = devm_kzalloc(&pdev->dev, sizeof(struct gxp_fan_ctrl_drvdata),
> + GFP_KERNEL);
There is a local variable (dev) pointing to &pdev->dev.
I would suggest to use it.
> + if (!drvdata)
> + return -ENOMEM;
> +
> + drvdata->dev = &pdev->dev;
> +
> + platform_set_drvdata(pdev, drvdata);
There is no platform_get_drvdata() in this code, meaning
platform_set_drvdata() is unnecessary.
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + drvdata->base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(drvdata->base)) {
> + error = dev_err_probe(dev, PTR_ERR(drvdata->base),
> + "failed to map base\n");
> + goto free_mem;
> + }
> + drvdata->plreg_map = gxp_fan_ctrl_init_regmap(pdev, "pl");
> + if (IS_ERR(drvdata->plreg_map)) {
> + error = dev_err_probe(dev, PTR_ERR(drvdata->plreg_map),
> + "failed to map plreg_handle\n");
> + goto free_mem;
> + }
> +
> + drvdata->fn2_map = gxp_fan_ctrl_init_regmap(pdev, "fn2");
> + if (IS_ERR(drvdata->fn2_map)) {
> + error = dev_err_probe(dev, PTR_ERR(drvdata->fn2_map),
> + "failed to map fn2_handle\n");
> + goto free_mem;
> + }
> +
> + drvdata->hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> + "fan_ctrl",
fan_ctrl is a bit generic. Normally this reflects the driver name.
> + drvdata,
> + &gxp_fan_ctrl_chip_info,
> + NULL);
> +
> + if (IS_ERR(drvdata->hwmon_dev)) {
> + error = dev_err_probe(dev, PTR_ERR(drvdata->hwmon_dev),
> + "failed to register fan ctrl\n");
> +
> + goto free_mem;
> + }
> +
> + return 0;
> +
> +free_mem:
> + kfree(drvdata);
drvdata was allocated with a devm function. Releasing it with kfree
results in a double free. This goto and error handling is not only
unnecessary but wrong.
> + return error;
> +}
> +
> +static const struct of_device_id gxp_fan_ctrl_of_match[] = {
> + { .compatible = "hpe,gxp-fan-ctrl", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, gxp_fan_ctrl_of_match);
> +
> +static struct platform_driver gxp_fan_ctrl_driver = {
> + .probe = gxp_fan_ctrl_probe,
> + .driver = {
> + .name = "gxp-fan-ctrl",
> + .of_match_table = gxp_fan_ctrl_of_match,
> + },
> +};
> +module_platform_driver(gxp_fan_ctrl_driver);
> +
> +MODULE_AUTHOR("Nick Hawkins <[email protected]>");
> +MODULE_DESCRIPTION("HPE GXP fan controller");
> +MODULE_LICENSE("GPL");
Hi,
I love your patch! Perhaps something to improve:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on robh/for-next linus/master v6.1-rc7 next-20221129]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/nick-hawkins-hpe-com/ARM-Add-GXP-Fan-and-SPI-controllers/20221129-070537
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20221128230219.39537-2-nick.hawkins%40hpe.com
patch subject: [PATCH v2 1/6] hwmon: (gxp-fan-ctrl) Add GXP fan controller
config: sparc-allyesconfig
compiler: sparc64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/08b4f146002742edb3de971dfd0c1df7ab9cef21
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review nick-hawkins-hpe-com/ARM-Add-GXP-Fan-and-SPI-controllers/20221129-070537
git checkout 08b4f146002742edb3de971dfd0c1df7ab9cef21
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=sparc SHELL=/bin/bash drivers/hwmon/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
drivers/hwmon/gxp-fan-ctrl.c: In function 'gxp_fan_ctrl_is_visible':
>> drivers/hwmon/gxp-fan-ctrl.c:158:17: warning: this statement may fall through [-Wimplicit-fallthrough=]
158 | switch (attr) {
| ^~~~~~
drivers/hwmon/gxp-fan-ctrl.c:165:9: note: here
165 | case hwmon_pwm:
| ^~~~
vim +158 drivers/hwmon/gxp-fan-ctrl.c
149
150 static umode_t gxp_fan_ctrl_is_visible(const void *_data,
151 enum hwmon_sensor_types type,
152 u32 attr, int channel)
153 {
154 umode_t mode = 0;
155
156 switch (type) {
157 case hwmon_fan:
> 158 switch (attr) {
159 case hwmon_fan_enable:
160 case hwmon_fan_fault:
161 mode = 0444;
162 default:
163 break;
164 }
165 case hwmon_pwm:
166 switch (attr) {
167 case hwmon_pwm_input:
168 mode = 0644;
169 default:
170 break;
171 }
172 default:
173 break;
174 }
175
176 return mode;
177 }
178
--
0-DAY CI Kernel Test Service
https://01.org/lkp
> > + writeb(val, drvdata->base + channel);
> The mixed use of direct writes and regmap is odd and confusing.
> Why use regmap for plreg_map and for fn2_map but not for base ?
> Can this be unified ? If not, why ?
I will still require three separate areas to access these registers as they
are spaced far apart (> 0x20000000). I will create a comment to
explain this. As for being unified methods I believe they can be
all __iomem's or regmaps.
Thanks,
-Nick Hawkins
On 11/29/22 09:13, Hawkins, Nick wrote:
>>> + writeb(val, drvdata->base + channel);
>
>> The mixed use of direct writes and regmap is odd and confusing.
>> Why use regmap for plreg_map and for fn2_map but not for base ?
>> Can this be unified ? If not, why ?
>
> I will still require three separate areas to access these registers as they
> are spaced far apart (> 0x20000000). I will create a comment to
> explain this. As for being unified methods I believe they can be
> all __iomem's or regmaps.
>
I understand that there are three regions. What I don't understand is
that two of them are accessed through regmap and one directly. Either
access all regions using regmap, or all regions using iomem accessors
directly. If regmap does not support access to the base region,
don't use regmap at all, explain why regmap doesn't support this kind
of access, and make sure to copy the regmap maintainer.
Thanks,
Guenter