[v2,1/6] hwmon: (gxp-fan-ctrl) Add GXP fan controller

Message ID 20221128230219.39537-2-nick.hawkins@hpe.com
State New
Headers
Series ARM: Add GXP Fan and SPI controllers |

Commit Message

Hawkins, Nick Nov. 28, 2022, 11:02 p.m. UTC
  From: Nick Hawkins <nick.hawkins@hpe.com>

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 <nick.hawkins@hpe.com>

---

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
  

Comments

Guenter Roeck Nov. 29, 2022, 4:02 a.m. UTC | #1
On Mon, Nov 28, 2022 at 05:02:14PM -0600, nick.hawkins@hpe.com wrote:
> From: Nick Hawkins <nick.hawkins@hpe.com>
> 
> 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 <nick.hawkins@hpe.com>
> ---
> 
> 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 <nick.hawkins@hpe.com>
> +
> +
> +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, &reg);
> +
> +	/* 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, &reg);
> +	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, &regmap_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 <nick.hawkins@hpe.com>");
> +MODULE_DESCRIPTION("HPE GXP fan controller");
> +MODULE_LICENSE("GPL");
  
kernel test robot Nov. 29, 2022, 6:58 a.m. UTC | #2
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 <lkp@intel.com>

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
  
Hawkins, Nick Nov. 29, 2022, 5:13 p.m. UTC | #3
> > +		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
  
Guenter Roeck Nov. 29, 2022, 6:26 p.m. UTC | #4
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
  

Patch

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 <nick.hawkins@hpe.com>
+
+
+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, &reg);
+
+	/* 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, &reg);
+	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, &regmap_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 <nick.hawkins@hpe.com>");
+MODULE_DESCRIPTION("HPE GXP fan controller");
+MODULE_LICENSE("GPL");