[PATCH 0/3] Support AXI FAN Control IP core

[PATCH 0/3] Support AXI FAN Control IP core

[Nuno Sá]

This series adds support for the ADI AXI FAN Control IP core. The first
patch adds some new definitions to the adi-axi-common header file. This
new `#defines` will be then used by the new hwmon driver. The rest of the
series is the usual stuff for a new device driver.

Nuno Sá (3):
  include: fpga: adi-axi-common: Define version macros
  hwmon: Support ADI Fan Control IP
  dt-bindings: hwmon: Add AXI FAN Control documentation

 .../bindings/hwmon/adi,axi-fan-control.yaml   |  58 +++
 MAINTAINERS                                   |   8 +
 drivers/hwmon/Kconfig                         |   9 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/axi-fan-control.c               | 452 ++++++++++++++++++
 include/linux/fpga/adi-axi-common.h           |   4 +
 6 files changed, 532 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
 create mode 100644 drivers/hwmon/axi-fan-control.c

-- 
2.23.0

[PATCH 1/3] include: fpga: adi-axi-common: Define version macros

[Nuno Sá]

Add commom macros to "parse" ADI HDL cores version, in terms of
major, minor and patch.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 include/linux/fpga/adi-axi-common.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/include/linux/fpga/adi-axi-common.h b/include/linux/fpga/adi-axi-common.h
index 7fc95d5c95bb..5bc5603e6bc8 100644
--- a/include/linux/fpga/adi-axi-common.h
+++ b/include/linux/fpga/adi-axi-common.h
@@ -16,4 +16,8 @@
 #define ADI_AXI_PCORE_VER(major, minor, patch)	\
 	(((major) << 16) | ((minor) << 8) | (patch))
 
+#define ADI_AXI_PCORE_VER_MAJOR(version)	(((version) >> 16) & 0xff)
+#define ADI_AXI_PCORE_VER_MINOR(version)	(((version) >> 8) & 0xff)
+#define ADI_AXI_PCORE_VER_PATCH(version)	((version) & 0xff)
+
 #endif /* ADI_AXI_COMMON_H_ */
-- 
2.23.0

Re: [PATCH 1/3] include: fpga: adi-axi-common: Define version macros

[Moritz Fischer]

Hi Nuno,

On Thu, Sep 26, 2019 at 12:39:23PM +0200, Nuno Sá wrote:
> Add commom macros to "parse" ADI HDL cores version, in terms of
> major, minor and patch.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  include/linux/fpga/adi-axi-common.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/include/linux/fpga/adi-axi-common.h b/include/linux/fpga/adi-axi-common.h
> index 7fc95d5c95bb..5bc5603e6bc8 100644
> --- a/include/linux/fpga/adi-axi-common.h
> +++ b/include/linux/fpga/adi-axi-common.h
> @@ -16,4 +16,8 @@
>  #define ADI_AXI_PCORE_VER(major, minor, patch)	\
>  	(((major) << 16) | ((minor) << 8) | (patch))
>  
> +#define ADI_AXI_PCORE_VER_MAJOR(version)	(((version) >> 16) & 0xff)
> +#define ADI_AXI_PCORE_VER_MINOR(version)	(((version) >> 8) & 0xff)
> +#define ADI_AXI_PCORE_VER_PATCH(version)	((version) & 0xff)
> +
>  #endif /* ADI_AXI_COMMON_H_ */
> -- 
> 2.23.0
> 

While implemented in an FPGA I'm not sure if this needs to go into
includelinux/fpga/.

I'd suggest to add this to the actual driver for now, and once you have
multiple users you can find a common location.

Cheers,
Moritz

Re: [PATCH 1/3] include: fpga: adi-axi-common: Define version macros

[Sa, Nuno]

Hi Moritz,

On Fri, 2019-09-27 at 08:01 -0700, Moritz Fischer wrote:
> 
> Hi Nuno,
> 
> On Thu, Sep 26, 2019 at 12:39:23PM +0200, Nuno Sá wrote:
> > Add commom macros to "parse" ADI HDL cores version, in terms of
> > major, minor and patch.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  include/linux/fpga/adi-axi-common.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/include/linux/fpga/adi-axi-common.h
> > b/include/linux/fpga/adi-axi-common.h
> > index 7fc95d5c95bb..5bc5603e6bc8 100644
> > --- a/include/linux/fpga/adi-axi-common.h
> > +++ b/include/linux/fpga/adi-axi-common.h
> > @@ -16,4 +16,8 @@
> >  #define ADI_AXI_PCORE_VER(major, minor, patch)	\
> >  	(((major) << 16) | ((minor) << 8) | (patch))
> >  
> > +#define ADI_AXI_PCORE_VER_MAJOR(version)	(((version) >> 16) &
> > 0xff)
> > +#define ADI_AXI_PCORE_VER_MINOR(version)	(((version) >> 8) &
> > 0xff)
> > +#define ADI_AXI_PCORE_VER_PATCH(version)	((version) & 0xff)
> > +
> >  #endif /* ADI_AXI_COMMON_H_ */
> > -- 
> > 2.23.0
> > 
> 
> While implemented in an FPGA I'm not sure if this needs to go into
> includelinux/fpga/.
> 
> I'd suggest to add this to the actual driver for now, and once you
> have
> multiple users you can find a common location.

Will do that.

> Cheers,
> Moritz

Thanks!
Nuno Sá

[PATCH 2/3] hwmon: Support ADI Fan Control IP

[Nuno Sá]

The purpose of this IP Core is to control the fan used for the cooling of a
Xilinx Zynq Ultrascale+ MPSoC without the need of any external temperature
sensors. To achieve this, the IP core uses the PL SYSMONE4 primitive to
obtain the PL temperature and, based on those readings, it then outputs
a PWM signal to control the fan rotation accordingly.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 MAINTAINERS                     |   7 +
 drivers/hwmon/Kconfig           |   9 +
 drivers/hwmon/Makefile          |   1 +
 drivers/hwmon/axi-fan-control.c | 452 ++++++++++++++++++++++++++++++++
 4 files changed, 469 insertions(+)
 create mode 100644 drivers/hwmon/axi-fan-control.c

diff --git a/MAINTAINERS b/MAINTAINERS
index c7035ce2460b..d775c923d23b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2853,6 +2853,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/sound/axentia,*
 F:	sound/soc/atmel/tse850-pcm5142.c
 
+AXI-FAN-CONTROL HARDWARE MONITOR DRIVER
+M:	Nuno Sá <nuno.sa@analog.com>
+W:	http://ez.analog.com/community/linux-device-drivers
+L:	linux-hwmon@vger.kernel.org
+S:	Supported
+F:	drivers/hwmon/axi-fan-control.c
+
 AXXIA I2C CONTROLLER
 M:	Krzysztof Adamski <krzysztof.adamski@nokia.com>
 L:	linux-i2c@vger.kernel.org
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 2ca5668bdb62..2396cc347c47 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -269,6 +269,15 @@ config SENSORS_ASC7621
 	  This driver can also be built as a module. If so, the module
 	  will be called asc7621.
 
+config SENSORS_AXI_FAN_CONTROL
+	tristate "Analog Devices FAN Control HDL Core driver"
+	help
+	  If you say yes here you get support for the Analog Devices
+	  AXI HDL FAN monitoring core.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called axi-fan-control
+
 config SENSORS_K8TEMP
 	tristate "AMD Athlon64/FX or Opteron temperature sensor"
 	depends on X86 && PCI
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index c86ce4d3d36b..ebb1fd2ea82b 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
 obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
 obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
 obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
+obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
 obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
 obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
 obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
new file mode 100644
index 000000000000..f86efc444753
--- /dev/null
+++ b/drivers/hwmon/axi-fan-control.c
@@ -0,0 +1,452 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Fan Control HDL CORE driver
+ *
+ * Copyright 2019 Analog Devices Inc.
+ */
+#include <linux/clk.h>
+#include <linux/fpga/adi-axi-common.h>
+#include <linux/hwmon.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+/* register map */
+#define ADI_REG_RSTN		0x0080
+#define ADI_REG_PWM_WIDTH	0x0084
+#define ADI_REG_TACH_PERIOD	0x0088
+#define ADI_REG_TACH_TOLERANCE	0x008c
+#define ADI_REG_PWM_PERIOD	0x00c0
+#define ADI_REG_TACH_MEASUR	0x00c4
+#define ADI_REG_TEMPERATURE	0x00c8
+
+#define ADI_REG_IRQ_MASK	0x0040
+#define ADI_REG_IRQ_PENDING	0x0044
+#define ADI_REG_IRQ_SRC		0x0048
+
+/* IRQ sources */
+#define ADI_IRQ_SRC_PWM_CHANGED		BIT(0)
+#define ADI_IRQ_SRC_TACH_ERR		BIT(1)
+#define ADI_IRQ_SRC_TEMP_INCREASE	BIT(2)
+#define ADI_IRQ_SRC_NEW_MEASUR		BIT(3)
+#define ADI_IRQ_SRC_MASK		GENMASK(3, 0)
+#define ADI_IRQ_MASK_OUT_ALL		0xFFFFFFFFU
+
+#define SYSFS_PWM_MAX			255
+
+struct axi_fan_control_data {
+	struct clk *clk;
+	void __iomem *base;
+	/* tacho period */
+	atomic_t tach;
+	int irq;
+	/* pulses per revolution */
+	u32 ppr;
+	bool hw_pwm_req;
+	bool update_tacho_params;
+	u8 fan_fault;
+};
+
+static inline void axi_fan_control_iowrite(const u32 val, const u32 reg,
+				const struct axi_fan_control_data *ctl)
+{
+	iowrite32(val, ctl->base + reg);
+}
+
+static inline u32 axi_fan_control_ioread(const u32 reg,
+					 const struct axi_fan_control_data *ctl)
+{
+	return ioread32(ctl->base + reg);
+}
+
+static long axi_fan_control_get_pwm_duty(const struct axi_fan_control_data *ctl)
+{
+	u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
+	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD, ctl);
+
+	return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX, pwm_period);
+}
+
+static int axi_fan_control_set_pwm_duty(const long val,
+					struct axi_fan_control_data *ctl)
+{
+	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD, ctl);
+	u32 new_width;
+	long __val = clamp_val(val, 0, SYSFS_PWM_MAX);
+
+	new_width = DIV_ROUND_CLOSEST(__val * pwm_period, SYSFS_PWM_MAX);
+
+	axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl);
+
+	return 0;
+}
+
+static long axi_fan_control_get_fan_rpm(const struct axi_fan_control_data *ctl)
+{
+	unsigned long clk_rate = clk_get_rate(ctl->clk);
+
+	if (!clk_rate)
+		return -EINVAL;
+	/*
+	 * The tacho period should be:
+	 *      TACH = 60/(ppr * rpm), where rpm is revolutions per second
+	 *      and ppr is pulses per revolution.
+	 * Given the tacho period, we can multiply it by the input clock
+	 * so that we know how many clocks we need to have this period.
+	 * From this, we can derive the RPM value.
+	 */
+	return DIV_ROUND_CLOSEST(60 * clk_rate,
+				 ctl->ppr * atomic_read(&ctl->tach));
+}
+
+static int axi_fan_control_read_temp(struct device *dev, u32 attr, long *val)
+{
+	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
+	long raw_temp;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE, ctl);
+		/*
+		 * The formula for the temperature is:
+		 *      T = (ADC * 501.3743 / 2^bits) - 273.6777
+		 * It's multiplied by 1000 to have milidegrees as
+		 * specified by the hwmon sysfs interface.
+		 */
+		*val = ((raw_temp * 501374) >> 16) - 273677;
+		return 0;
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static int axi_fan_control_read_fan(struct device *dev, u32 attr, long *val)
+{
+	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_fan_fault:
+		*val = ctl->fan_fault;
+		return 0;
+	case hwmon_fan_input:
+		*val = axi_fan_control_get_fan_rpm(ctl);
+		return 0;
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static int axi_fan_control_read_pwm(struct device *dev, u32 attr, long *val)
+{
+	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		*val = axi_fan_control_get_pwm_duty(ctl);
+		return 0;
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static int axi_fan_control_write_pwm(struct device *dev, u32 attr, long val)
+{
+	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
+
+	switch (attr) {
+	case hwmon_pwm_input:
+		return axi_fan_control_set_pwm_duty(val, ctl);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static int axi_fan_control_read_labels(struct device *dev,
+				       enum hwmon_sensor_types type,
+				       u32 attr, int channel, const char **str)
+{
+	switch (type) {
+	case hwmon_fan:
+		*str = "FAN";
+		return 0;
+	case hwmon_temp:
+		*str = "SYSMON4";
+		return 0;
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static int axi_fan_control_read(struct device *dev,
+				enum hwmon_sensor_types type,
+				u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_fan:
+		return axi_fan_control_read_fan(dev, attr, val);
+	case hwmon_pwm:
+		return axi_fan_control_read_pwm(dev, attr, val);
+	case hwmon_temp:
+		return axi_fan_control_read_temp(dev, attr, val);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static int axi_fan_control_write(struct device *dev,
+				 enum hwmon_sensor_types type,
+				 u32 attr, int channel, long val)
+{
+	switch (type) {
+	case hwmon_pwm:
+		return axi_fan_control_write_pwm(dev, attr, val);
+	default:
+		return -ENOTSUPP;
+	}
+}
+
+static umode_t axi_fan_control_fan_is_visible(const u32 attr)
+{
+	switch (attr) {
+	case hwmon_fan_input:
+	case hwmon_fan_fault:
+	case hwmon_fan_label:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static umode_t axi_fan_control_pwm_is_visible(const u32 attr)
+{
+	switch (attr) {
+	case hwmon_pwm_input:
+		return 0644;
+	default:
+		return 0;
+	}
+}
+
+static umode_t axi_fan_control_temp_is_visible(const u32 attr)
+{
+	switch (attr) {
+	case hwmon_temp_input:
+	case hwmon_temp_label:
+		return 0444;
+	default:
+		return 0;
+	}
+}
+
+static umode_t axi_fan_control_is_visible(const void *data,
+					  enum hwmon_sensor_types type,
+					  u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_fan:
+		return axi_fan_control_fan_is_visible(attr);
+	case hwmon_pwm:
+		return axi_fan_control_pwm_is_visible(attr);
+	case hwmon_temp:
+		return axi_fan_control_temp_is_visible(attr);
+	default:
+		return 0;
+	}
+}
+
+static irqreturn_t axi_fan_control_irq_handler(int irq, void *data)
+{
+	struct axi_fan_control_data *ctl = (struct axi_fan_control_data *)data;
+	u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING, ctl);
+	u32 clear_mask;
+
+	if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) {
+		u32 new_tach = axi_fan_control_ioread(ADI_REG_TACH_MEASUR,
+						      ctl);
+
+		if (ctl->update_tacho_params) {
+			/* get 25% tolerance */
+			u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, 100);
+			/* set new tacho parameters */
+			axi_fan_control_iowrite(new_tach, ADI_REG_TACH_PERIOD,
+						ctl);
+			axi_fan_control_iowrite(tach_tol,
+						ADI_REG_TACH_TOLERANCE, ctl);
+			ctl->update_tacho_params = false;
+		}
+
+		atomic_set(&ctl->tach, new_tach);
+	}
+
+	if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) {
+		/*
+		 * if the pwm changes on behalf of software,
+		 * we need to provide new tacho parameters to the core.
+		 * Wait for the next measurement for that...
+		 */
+		if (!ctl->hw_pwm_req)
+			ctl->update_tacho_params = true;
+
+		/* just set this to false even if it is already... */
+		ctl->hw_pwm_req = false;
+	}
+
+	if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
+		/* hardware requested a new pwm */
+		ctl->hw_pwm_req = true;
+
+	if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
+		ctl->fan_fault = 1;
+
+	/* clear all interrupts */
+	clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
+	axi_fan_control_iowrite(clear_mask, ADI_REG_IRQ_PENDING, ctl);
+
+	return IRQ_HANDLED;
+}
+
+static int axi_fan_control_init(struct axi_fan_control_data *ctl,
+				const struct device_node *np)
+{
+	int ret;
+
+	/* get fan pulses per revolution */
+	ret = of_property_read_u32(np, "adi,pulses-per-revolution", &ctl->ppr);
+	if (ret)
+		return ret;
+	/*
+	 * Enable all IRQs
+	 */
+	axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL &
+			~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR |
+			ADI_IRQ_SRC_PWM_CHANGED | ADI_IRQ_SRC_TEMP_INCREASE)),
+			ADI_REG_IRQ_MASK, ctl);
+
+	/* bring the device out of reset */
+	axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl);
+
+	return ret;
+}
+
+static const struct hwmon_channel_info *axi_fan_control_info[] = {
+	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT),
+	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_LABEL),
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
+	NULL
+};
+
+static const struct hwmon_ops axi_fan_control_hwmon_ops = {
+	.is_visible = axi_fan_control_is_visible,
+	.read = axi_fan_control_read,
+	.write = axi_fan_control_write,
+	.read_string = axi_fan_control_read_labels,
+};
+
+static const struct hwmon_chip_info axi_fan_control_chip_info = {
+	.ops = &axi_fan_control_hwmon_ops,
+	.info = axi_fan_control_info,
+};
+
+static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a');
+
+static const struct of_device_id axi_fan_control_of_match[] = {
+	{ .compatible = "adi,axi-fan-control-1.00.a",
+		.data = (void *)&version_1_0_0},
+	{},
+};
+MODULE_DEVICE_TABLE(of, axi_fan_control_of_match);
+
+static int axi_fan_control_probe(struct platform_device *pdev)
+{
+	struct device *hwmon_dev;
+	struct axi_fan_control_data *ctl;
+	const struct of_device_id *id;
+	struct resource *res;
+	u32 version;
+	int ret;
+
+	id = of_match_node(axi_fan_control_of_match, pdev->dev.of_node);
+	if (!id)
+		return -EINVAL;
+
+	ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL);
+	if (!ctl)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	ctl->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(ctl->base)) {
+		dev_err(&pdev->dev, "ioremap failed with %ld\n",
+			PTR_ERR(ctl->base));
+		return PTR_ERR(ctl->base);
+	}
+
+	ctl->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(ctl->clk)) {
+		dev_err(&pdev->dev, "clk_get failed with %ld\n",
+			PTR_ERR(ctl->clk));
+		return PTR_ERR(ctl->clk);
+	}
+
+	dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n",
+		(unsigned long long)res->start, ctl->base);
+
+	version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl);
+	if (ADI_AXI_PCORE_VER_MAJOR(version) !=
+	    ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) {
+		dev_err(&pdev->dev, "Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
+			ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)),
+			ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)),
+			ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)),
+			ADI_AXI_PCORE_VER_MAJOR(version),
+			ADI_AXI_PCORE_VER_MINOR(version),
+			ADI_AXI_PCORE_VER_PATCH(version));
+		return -ENODEV;
+	}
+
+	ctl->irq = platform_get_irq(pdev, 0);
+	if (ctl->irq < 0) {
+		dev_err(&pdev->dev, "platform_get_irq failed with %d\n",
+			ctl->irq);
+		return ctl->irq;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL,
+					axi_fan_control_irq_handler,
+					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
+					pdev->driver_override, ctl);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request an irq, %d", ret);
+		return ret;
+	}
+
+	ret = axi_fan_control_init(ctl, pdev->dev.of_node);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to initialize device\n");
+		return ret;
+	}
+
+	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
+						"axi_fan_control",
+						ctl,
+						&axi_fan_control_chip_info,
+						NULL);
+
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static struct platform_driver axi_fan_control_driver = {
+	.driver = {
+		.name = "axi_fan_control_driver",
+		.of_match_table = axi_fan_control_of_match,
+	},
+	.probe = axi_fan_control_probe,
+};
+module_platform_driver(axi_fan_control_driver);
+
+MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
+MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver");
+MODULE_LICENSE("GPL");
-- 
2.23.0

Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP

[Guenter Roeck]

On Thu, Sep 26, 2019 at 12:39:24PM +0200, Nuno Sá wrote:
> The purpose of this IP Core is to control the fan used for the cooling of a
> Xilinx Zynq Ultrascale+ MPSoC without the need of any external temperature
> sensors. To achieve this, the IP core uses the PL SYSMONE4 primitive to
> obtain the PL temperature and, based on those readings, it then outputs
> a PWM signal to control the fan rotation accordingly.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  MAINTAINERS                     |   7 +
>  drivers/hwmon/Kconfig           |   9 +
>  drivers/hwmon/Makefile          |   1 +
>  drivers/hwmon/axi-fan-control.c | 452 ++++++++++++++++++++++++++++++++
>  4 files changed, 469 insertions(+)
>  create mode 100644 drivers/hwmon/axi-fan-control.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c7035ce2460b..d775c923d23b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2853,6 +2853,13 @@ S:	Maintained
>  F:	Documentation/devicetree/bindings/sound/axentia,*
>  F:	sound/soc/atmel/tse850-pcm5142.c
>  
> +AXI-FAN-CONTROL HARDWARE MONITOR DRIVER
> +M:	Nuno Sá <nuno.sa@analog.com>
> +W:	http://ez.analog.com/community/linux-device-drivers
> +L:	linux-hwmon@vger.kernel.org
> +S:	Supported
> +F:	drivers/hwmon/axi-fan-control.c
> +
>  AXXIA I2C CONTROLLER
>  M:	Krzysztof Adamski <krzysztof.adamski@nokia.com>
>  L:	linux-i2c@vger.kernel.org
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 2ca5668bdb62..2396cc347c47 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -269,6 +269,15 @@ config SENSORS_ASC7621
>  	  This driver can also be built as a module. If so, the module
>  	  will be called asc7621.
>  
> +config SENSORS_AXI_FAN_CONTROL
> +	tristate "Analog Devices FAN Control HDL Core driver"
> +	help
> +	  If you say yes here you get support for the Analog Devices
> +	  AXI HDL FAN monitoring core.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called axi-fan-control
> +
>  config SENSORS_K8TEMP
>  	tristate "AMD Athlon64/FX or Opteron temperature sensor"
>  	depends on X86 && PCI
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index c86ce4d3d36b..ebb1fd2ea82b 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370)	+= as370-hwmon.o
>  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
>  obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
>  obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
> +obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
>  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
>  obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
> new file mode 100644
> index 000000000000..f86efc444753
> --- /dev/null
> +++ b/drivers/hwmon/axi-fan-control.c
> @@ -0,0 +1,452 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Fan Control HDL CORE driver
> + *
> + * Copyright 2019 Analog Devices Inc.
> + */
> +#include <linux/clk.h>
> +#include <linux/fpga/adi-axi-common.h>
> +#include <linux/hwmon.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +/* register map */
> +#define ADI_REG_RSTN		0x0080
> +#define ADI_REG_PWM_WIDTH	0x0084
> +#define ADI_REG_TACH_PERIOD	0x0088
> +#define ADI_REG_TACH_TOLERANCE	0x008c
> +#define ADI_REG_PWM_PERIOD	0x00c0
> +#define ADI_REG_TACH_MEASUR	0x00c4
> +#define ADI_REG_TEMPERATURE	0x00c8
> +
> +#define ADI_REG_IRQ_MASK	0x0040
> +#define ADI_REG_IRQ_PENDING	0x0044
> +#define ADI_REG_IRQ_SRC		0x0048
> +
> +/* IRQ sources */
> +#define ADI_IRQ_SRC_PWM_CHANGED		BIT(0)

include linux/bits.h

> +#define ADI_IRQ_SRC_TACH_ERR		BIT(1)
> +#define ADI_IRQ_SRC_TEMP_INCREASE	BIT(2)
> +#define ADI_IRQ_SRC_NEW_MEASUR		BIT(3)
> +#define ADI_IRQ_SRC_MASK		GENMASK(3, 0)
> +#define ADI_IRQ_MASK_OUT_ALL		0xFFFFFFFFU
> +
> +#define SYSFS_PWM_MAX			255
> +
> +struct axi_fan_control_data {
> +	struct clk *clk;
> +	void __iomem *base;
> +	/* tacho period */
> +	atomic_t tach;

Why exactly is this an atomic ? The value is only set, and it is set in
a single operation. Why does it matter if reqading it catches the old or
the new value ?

> +	int irq;
> +	/* pulses per revolution */
> +	u32 ppr;
> +	bool hw_pwm_req;
> +	bool update_tacho_params;
> +	u8 fan_fault;
> +};
> +
> +static inline void axi_fan_control_iowrite(const u32 val, const u32 reg,
> +				const struct axi_fan_control_data *ctl)

Multi-line alignment. Also, please consider shorter function names.
An "axi_fan_control_" prefix for static functions is really a bit excessive
and doesn't add value. "axi_" would do it just as well.

> +{
> +	iowrite32(val, ctl->base + reg);
> +}
> +
> +static inline u32 axi_fan_control_ioread(const u32 reg,
> +					 const struct axi_fan_control_data *ctl)
> +{
> +	return ioread32(ctl->base + reg);
> +}
> +
> +static long axi_fan_control_get_pwm_duty(const struct axi_fan_control_data *ctl)
> +{
> +	u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
> +	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD, ctl);
> +
> +	return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX, pwm_period);

Is pwm_period guaranteed to be != 0 ?

> +}
> +
> +static int axi_fan_control_set_pwm_duty(const long val,
> +					struct axi_fan_control_data *ctl)
> +{
> +	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD, ctl);
> +	u32 new_width;
> +	long __val = clamp_val(val, 0, SYSFS_PWM_MAX);
> +
> +	new_width = DIV_ROUND_CLOSEST(__val * pwm_period, SYSFS_PWM_MAX);
> +
> +	axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl);
> +
> +	return 0;
> +}
> +
> +static long axi_fan_control_get_fan_rpm(const struct axi_fan_control_data *ctl)
> +{
> +	unsigned long clk_rate = clk_get_rate(ctl->clk);
> +
> +	if (!clk_rate)
> +		return -EINVAL;

Unless the clock rate changes dynamically, it might make sense to read it once
in the probe function.

> +	/*
> +	 * The tacho period should be:
> +	 *      TACH = 60/(ppr * rpm), where rpm is revolutions per second
> +	 *      and ppr is pulses per revolution.
> +	 * Given the tacho period, we can multiply it by the input clock
> +	 * so that we know how many clocks we need to have this period.
> +	 * From this, we can derive the RPM value.
> +	 */
> +	return DIV_ROUND_CLOSEST(60 * clk_rate,
> +				 ctl->ppr * atomic_read(&ctl->tach));

Are ppr and tach guaranteed to be != 0 ? I don't think so,
since neither is ever evaluated.

> +}
> +
> +static int axi_fan_control_read_temp(struct device *dev, u32 attr, long *val)
> +{
> +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> +	long raw_temp;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE, ctl);
> +		/*
> +		 * The formula for the temperature is:
> +		 *      T = (ADC * 501.3743 / 2^bits) - 273.6777
> +		 * It's multiplied by 1000 to have milidegrees as

s/milidegrees/millidegrees/

> +		 * specified by the hwmon sysfs interface.
> +		 */
> +		*val = ((raw_temp * 501374) >> 16) - 273677;
> +		return 0;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int axi_fan_control_read_fan(struct device *dev, u32 attr, long *val)
> +{
> +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_fan_fault:
> +		*val = ctl->fan_fault;
> +		return 0;
> +	case hwmon_fan_input:
> +		*val = axi_fan_control_get_fan_rpm(ctl);
> +		return 0;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int axi_fan_control_read_pwm(struct device *dev, u32 attr, long *val)
> +{
> +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		*val = axi_fan_control_get_pwm_duty(ctl);
> +		return 0;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int axi_fan_control_write_pwm(struct device *dev, u32 attr, long val)
> +{
> +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> +
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		return axi_fan_control_set_pwm_duty(val, ctl);
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int axi_fan_control_read_labels(struct device *dev,
> +				       enum hwmon_sensor_types type,
> +				       u32 attr, int channel, const char **str)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		*str = "FAN";
> +		return 0;
> +	case hwmon_temp:
> +		*str = "SYSMON4";
> +		return 0;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int axi_fan_control_read(struct device *dev,
> +				enum hwmon_sensor_types type,
> +				u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		return axi_fan_control_read_fan(dev, attr, val);
> +	case hwmon_pwm:
> +		return axi_fan_control_read_pwm(dev, attr, val);
> +	case hwmon_temp:
> +		return axi_fan_control_read_temp(dev, attr, val);
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int axi_fan_control_write(struct device *dev,
> +				 enum hwmon_sensor_types type,
> +				 u32 attr, int channel, long val)
> +{
> +	switch (type) {
> +	case hwmon_pwm:
> +		return axi_fan_control_write_pwm(dev, attr, val);
> +	default:
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static umode_t axi_fan_control_fan_is_visible(const u32 attr)
> +{
> +	switch (attr) {
> +	case hwmon_fan_input:
> +	case hwmon_fan_fault:
> +	case hwmon_fan_label:
> +		return 0444;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static umode_t axi_fan_control_pwm_is_visible(const u32 attr)
> +{
> +	switch (attr) {
> +	case hwmon_pwm_input:
> +		return 0644;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static umode_t axi_fan_control_temp_is_visible(const u32 attr)
> +{
> +	switch (attr) {
> +	case hwmon_temp_input:
> +	case hwmon_temp_label:
> +		return 0444;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static umode_t axi_fan_control_is_visible(const void *data,
> +					  enum hwmon_sensor_types type,
> +					  u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_fan:
> +		return axi_fan_control_fan_is_visible(attr);
> +	case hwmon_pwm:
> +		return axi_fan_control_pwm_is_visible(attr);
> +	case hwmon_temp:
> +		return axi_fan_control_temp_is_visible(attr);
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static irqreturn_t axi_fan_control_irq_handler(int irq, void *data)
> +{
> +	struct axi_fan_control_data *ctl = (struct axi_fan_control_data *)data;
> +	u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING, ctl);
> +	u32 clear_mask;
> +
> +	if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) {
> +		u32 new_tach = axi_fan_control_ioread(ADI_REG_TACH_MEASUR,
> +						      ctl);
> +
> +		if (ctl->update_tacho_params) {
> +			/* get 25% tolerance */
> +			u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, 100);
> +			/* set new tacho parameters */
> +			axi_fan_control_iowrite(new_tach, ADI_REG_TACH_PERIOD,
> +						ctl);
> +			axi_fan_control_iowrite(tach_tol,
> +						ADI_REG_TACH_TOLERANCE, ctl);
> +			ctl->update_tacho_params = false;
> +		}
> +
> +		atomic_set(&ctl->tach, new_tach);
> +	}
> +
> +	if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) {
> +		/*
> +		 * if the pwm changes on behalf of software,
> +		 * we need to provide new tacho parameters to the core.
> +		 * Wait for the next measurement for that...
> +		 */
> +		if (!ctl->hw_pwm_req)
> +			ctl->update_tacho_params = true;
> +
> +		/* just set this to false even if it is already... */
> +		ctl->hw_pwm_req = false;
> +	}
> +
> +	if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
> +		/* hardware requested a new pwm */
> +		ctl->hw_pwm_req = true;
> +
I don't really understand the logic here. If ADI_IRQ_SRC_TEMP_INCREASE means
that hardware wants a new pwm, how is userspace informed about that request ?
And why are the tacho paramaters _not_ updated in this case later on (unless
ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both set) ?
It might be useful to describe the expected sequence of events.

> +	if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
> +		ctl->fan_fault = 1;

Is it on purpose that this bit is never reset ?

> +
> +	/* clear all interrupts */
> +	clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
> +	axi_fan_control_iowrite(clear_mask, ADI_REG_IRQ_PENDING, ctl);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int axi_fan_control_init(struct axi_fan_control_data *ctl,
> +				const struct device_node *np)
> +{
> +	int ret;
> +
> +	/* get fan pulses per revolution */
> +	ret = of_property_read_u32(np, "adi,pulses-per-revolution", &ctl->ppr);
> +	if (ret)
> +		return ret;

So all random values are acceptable, including 0 and 0xffffffff ?

> +	/*
> +	 * Enable all IRQs
> +	 */
> +	axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL &
> +			~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR |
> +			ADI_IRQ_SRC_PWM_CHANGED | ADI_IRQ_SRC_TEMP_INCREASE)),

One set of ( ) is unnecessary.

> +			ADI_REG_IRQ_MASK, ctl);
> +
> +	/* bring the device out of reset */
> +	axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl);
> +
> +	return ret;
> +}
> +
> +static const struct hwmon_channel_info *axi_fan_control_info[] = {
> +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT),
> +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_LABEL),
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
> +	NULL
> +};
> +
> +static const struct hwmon_ops axi_fan_control_hwmon_ops = {
> +	.is_visible = axi_fan_control_is_visible,
> +	.read = axi_fan_control_read,
> +	.write = axi_fan_control_write,
> +	.read_string = axi_fan_control_read_labels,
> +};
> +
> +static const struct hwmon_chip_info axi_fan_control_chip_info = {
> +	.ops = &axi_fan_control_hwmon_ops,
> +	.info = axi_fan_control_info,
> +};
> +
> +static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a');
> +
> +static const struct of_device_id axi_fan_control_of_match[] = {
> +	{ .compatible = "adi,axi-fan-control-1.00.a",
> +		.data = (void *)&version_1_0_0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, axi_fan_control_of_match);
> +
> +static int axi_fan_control_probe(struct platform_device *pdev)
> +{
> +	struct device *hwmon_dev;
> +	struct axi_fan_control_data *ctl;
> +	const struct of_device_id *id;
> +	struct resource *res;
> +	u32 version;
> +	int ret;
> +
> +	id = of_match_node(axi_fan_control_of_match, pdev->dev.of_node);
> +	if (!id)
> +		return -EINVAL;
> +
> +	ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL);
> +	if (!ctl)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	ctl->base = devm_ioremap_resource(&pdev->dev, res);

devm_platform_ioremap_resource()

> +	if (IS_ERR(ctl->base)) {
> +		dev_err(&pdev->dev, "ioremap failed with %ld\n",
> +			PTR_ERR(ctl->base));
> +		return PTR_ERR(ctl->base);
> +	}

This does not require an extra error message.

> +
> +	ctl->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(ctl->clk)) {
> +		dev_err(&pdev->dev, "clk_get failed with %ld\n",
> +			PTR_ERR(ctl->clk));
> +		return PTR_ERR(ctl->clk);
> +	}
> +
> +	dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n",
> +		(unsigned long long)res->start, ctl->base);
> +
> +	version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl);
> +	if (ADI_AXI_PCORE_VER_MAJOR(version) !=
> +	    ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) {
> +		dev_err(&pdev->dev, "Major version mismatch. Expected %d.%.2d.%c, Reported %d.%.2d.%c\n",
> +			ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)),
> +			ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)),
> +			ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)),
> +			ADI_AXI_PCORE_VER_MAJOR(version),
> +			ADI_AXI_PCORE_VER_MINOR(version),
> +			ADI_AXI_PCORE_VER_PATCH(version));
> +		return -ENODEV;
> +	}
> +
> +	ctl->irq = platform_get_irq(pdev, 0);
> +	if (ctl->irq < 0) {

This can return -EPROBE_DEFER. On top of that, it already generates an error
message, meaning another one here is unnecessary.

> +		dev_err(&pdev->dev, "platform_get_irq failed with %d\n",
> +			ctl->irq);
> +		return ctl->irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL,
> +					axi_fan_control_irq_handler,
> +					IRQF_ONESHOT | IRQF_TRIGGER_HIGH,
> +					pdev->driver_override, ctl);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request an irq, %d", ret);
> +		return ret;
> +	}
> +
> +	ret = axi_fan_control_init(ctl, pdev->dev.of_node);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Failed to initialize device\n");
> +		return ret;
> +	}
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> +						"axi_fan_control",
> +						ctl,
> +						&axi_fan_control_chip_info,
> +						NULL);

Another alignment issue. Shorter function and global variable names would help.

> +
> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static struct platform_driver axi_fan_control_driver = {
> +	.driver = {
> +		.name = "axi_fan_control_driver",
> +		.of_match_table = axi_fan_control_of_match,
> +	},
> +	.probe = axi_fan_control_probe,
> +};
> +module_platform_driver(axi_fan_control_driver);
> +
> +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
> +MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.23.0
> 

Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP

[Sa, Nuno]

On Sun, 2019-10-06 at 08:32 -0700, Guenter Roeck wrote:
> 
> On Thu, Sep 26, 2019 at 12:39:24PM +0200, Nuno Sá wrote:
> > The purpose of this IP Core is to control the fan used for the
> > cooling of a
> > Xilinx Zynq Ultrascale+ MPSoC without the need of any external
> > temperature
> > sensors. To achieve this, the IP core uses the PL SYSMONE4
> > primitive to
> > obtain the PL temperature and, based on those readings, it then
> > outputs
> > a PWM signal to control the fan rotation accordingly.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  MAINTAINERS                     |   7 +
> >  drivers/hwmon/Kconfig           |   9 +
> >  drivers/hwmon/Makefile          |   1 +
> >  drivers/hwmon/axi-fan-control.c | 452
> > ++++++++++++++++++++++++++++++++
> >  4 files changed, 469 insertions(+)
> >  create mode 100644 drivers/hwmon/axi-fan-control.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c7035ce2460b..d775c923d23b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2853,6 +2853,13 @@ S:	Maintained
> >  F:	Documentation/devicetree/bindings/sound/axentia,*
> >  F:	sound/soc/atmel/tse850-pcm5142.c
> >  
> > +AXI-FAN-CONTROL HARDWARE MONITOR DRIVER
> > +M:	Nuno Sá <nuno.sa@analog.com>
> > +W:	http://ez.analog.com/community/linux-device-drivers
> > +L:	linux-hwmon@vger.kernel.org
> > +S:	Supported
> > +F:	drivers/hwmon/axi-fan-control.c
> > +
> >  AXXIA I2C CONTROLLER
> >  M:	Krzysztof Adamski <krzysztof.adamski@nokia.com>
> >  L:	linux-i2c@vger.kernel.org
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 2ca5668bdb62..2396cc347c47 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -269,6 +269,15 @@ config SENSORS_ASC7621
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called asc7621.
> >  
> > +config SENSORS_AXI_FAN_CONTROL
> > +	tristate "Analog Devices FAN Control HDL Core driver"
> > +	help
> > +	  If you say yes here you get support for the Analog Devices
> > +	  AXI HDL FAN monitoring core.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called axi-fan-control
> > +
> >  config SENSORS_K8TEMP
> >  	tristate "AMD Athlon64/FX or Opteron temperature sensor"
> >  	depends on X86 && PCI
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index c86ce4d3d36b..ebb1fd2ea82b 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370)	+= as370-
> > hwmon.o
> >  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> >  obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
> >  obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
> > +obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> >  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
> >  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> >  obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> > diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-
> > fan-control.c
> > new file mode 100644
> > index 000000000000..f86efc444753
> > --- /dev/null
> > +++ b/drivers/hwmon/axi-fan-control.c
> > @@ -0,0 +1,452 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Fan Control HDL CORE driver
> > + *
> > + * Copyright 2019 Analog Devices Inc.
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/fpga/adi-axi-common.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* register map */
> > +#define ADI_REG_RSTN		0x0080
> > +#define ADI_REG_PWM_WIDTH	0x0084
> > +#define ADI_REG_TACH_PERIOD	0x0088
> > +#define ADI_REG_TACH_TOLERANCE	0x008c
> > +#define ADI_REG_PWM_PERIOD	0x00c0
> > +#define ADI_REG_TACH_MEASUR	0x00c4
> > +#define ADI_REG_TEMPERATURE	0x00c8
> > +
> > +#define ADI_REG_IRQ_MASK	0x0040
> > +#define ADI_REG_IRQ_PENDING	0x0044
> > +#define ADI_REG_IRQ_SRC		0x0048
> > +
> > +/* IRQ sources */
> > +#define ADI_IRQ_SRC_PWM_CHANGED		BIT(0)
> 
> include linux/bits.h

ack.

> > +#define ADI_IRQ_SRC_TACH_ERR		BIT(1)
> > +#define ADI_IRQ_SRC_TEMP_INCREASE	BIT(2)
> > +#define ADI_IRQ_SRC_NEW_MEASUR		BIT(3)
> > +#define ADI_IRQ_SRC_MASK		GENMASK(3, 0)
> > +#define ADI_IRQ_MASK_OUT_ALL		0xFFFFFFFFU
> > +
> > +#define SYSFS_PWM_MAX			255
> > +
> > +struct axi_fan_control_data {
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +	/* tacho period */
> > +	atomic_t tach;
> 
> Why exactly is this an atomic ? The value is only set, and it is set
> in
> a single operation. Why does it matter if reqading it catches the old
> or
> the new value ?

Hmm, I think that I don't even need this variable. I can just ioread
the value when getting the fan rpm and get whatever it is on the
register at that moment.
  
> > +	int irq;
> > +	/* pulses per revolution */
> > +	u32 ppr;
> > +	bool hw_pwm_req;
> > +	bool update_tacho_params;
> > +	u8 fan_fault;
> > +};
> > +
> > +static inline void axi_fan_control_iowrite(const u32 val, const
> > u32 reg,
> > +				const struct axi_fan_control_data *ctl)
> 
> Multi-line alignment. Also, please consider shorter function names.
> An "axi_fan_control_" prefix for static functions is really a bit
> excessive
> and doesn't add value. "axi_" would do it just as well.

Will do that.

> > +{
> > +	iowrite32(val, ctl->base + reg);
> > +}
> > +
> > +static inline u32 axi_fan_control_ioread(const u32 reg,
> > +					 const struct
> > axi_fan_control_data *ctl)
> > +{
> > +	return ioread32(ctl->base + reg);
> > +}
> > +
> > +static long axi_fan_control_get_pwm_duty(const struct
> > axi_fan_control_data *ctl)
> > +{
> > +	u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
> > +	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
> > ctl);
> > +
> > +	return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX,
> > pwm_period);
> 
> Is pwm_period guaranteed to be != 0 ?

Yes, It is a RO register and it is set by the core with the default of
0x4e20.
> > +}
> > +
> > +static int axi_fan_control_set_pwm_duty(const long val,
> > +					struct axi_fan_control_data
> > *ctl)
> > +{
> > +	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
> > ctl);
> > +	u32 new_width;
> > +	long __val = clamp_val(val, 0, SYSFS_PWM_MAX);
> > +
> > +	new_width = DIV_ROUND_CLOSEST(__val * pwm_period,
> > SYSFS_PWM_MAX);
> > +
> > +	axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl);
> > +
> > +	return 0;
> > +}
> > +
> > +static long axi_fan_control_get_fan_rpm(const struct
> > axi_fan_control_data *ctl)
> > +{
> > +	unsigned long clk_rate = clk_get_rate(ctl->clk);
> > +
> > +	if (!clk_rate)
> > +		return -EINVAL;
> 
> Unless the clock rate changes dynamically, it might make sense to
> read it once
> in the probe function.

Hmm, It can't so I will do as you suggest.

> > +	/*
> > +	 * The tacho period should be:
> > +	 *      TACH = 60/(ppr * rpm), where rpm is revolutions per
> > second
> > +	 *      and ppr is pulses per revolution.
> > +	 * Given the tacho period, we can multiply it by the input
> > clock
> > +	 * so that we know how many clocks we need to have this period.
> > +	 * From this, we can derive the RPM value.
> > +	 */
> > +	return DIV_ROUND_CLOSEST(60 * clk_rate,
> > +				 ctl->ppr * atomic_read(&ctl->tach));
> 
> Are ppr and tach guaranteed to be != 0 ? I don't think so,
> since neither is ever evaluated.

Nope. I should make sure ppr is never 0 and I shoud evaluate tach
before  doing the operation and return 0 
> > +}
> > +
> > +static int axi_fan_control_read_temp(struct device *dev, u32 attr,
> > long *val)
> > +{
> > +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> > +	long raw_temp;
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE,
> > ctl);
> > +		/*
> > +		 * The formula for the temperature is:
> > +		 *      T = (ADC * 501.3743 / 2^bits) - 273.6777
> > +		 * It's multiplied by 1000 to have milidegrees as
> 
> s/milidegrees/millidegrees/

ack.

> > +		 * specified by the hwmon sysfs interface.
> > +		 */
> > +		*val = ((raw_temp * 501374) >> 16) - 273677;
> > +		return 0;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_read_fan(struct device *dev, u32 attr,
> > long *val)
> > +{
> > +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_fan_fault:
> > +		*val = ctl->fan_fault;
> > +		return 0;
> > +	case hwmon_fan_input:
> > +		*val = axi_fan_control_get_fan_rpm(ctl);
> > +		return 0;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_read_pwm(struct device *dev, u32 attr,
> > long *val)
> > +{
> > +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_pwm_input:
> > +		*val = axi_fan_control_get_pwm_duty(ctl);
> > +		return 0;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_write_pwm(struct device *dev, u32 attr,
> > long val)
> > +{
> > +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_pwm_input:
> > +		return axi_fan_control_set_pwm_duty(val, ctl);
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_read_labels(struct device *dev,
> > +				       enum hwmon_sensor_types type,
> > +				       u32 attr, int channel, const
> > char **str)
> > +{
> > +	switch (type) {
> > +	case hwmon_fan:
> > +		*str = "FAN";
> > +		return 0;
> > +	case hwmon_temp:
> > +		*str = "SYSMON4";
> > +		return 0;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_read(struct device *dev,
> > +				enum hwmon_sensor_types type,
> > +				u32 attr, int channel, long *val)
> > +{
> > +	switch (type) {
> > +	case hwmon_fan:
> > +		return axi_fan_control_read_fan(dev, attr, val);
> > +	case hwmon_pwm:
> > +		return axi_fan_control_read_pwm(dev, attr, val);
> > +	case hwmon_temp:
> > +		return axi_fan_control_read_temp(dev, attr, val);
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_write(struct device *dev,
> > +				 enum hwmon_sensor_types type,
> > +				 u32 attr, int channel, long val)
> > +{
> > +	switch (type) {
> > +	case hwmon_pwm:
> > +		return axi_fan_control_write_pwm(dev, attr, val);
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static umode_t axi_fan_control_fan_is_visible(const u32 attr)
> > +{
> > +	switch (attr) {
> > +	case hwmon_fan_input:
> > +	case hwmon_fan_fault:
> > +	case hwmon_fan_label:
> > +		return 0444;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static umode_t axi_fan_control_pwm_is_visible(const u32 attr)
> > +{
> > +	switch (attr) {
> > +	case hwmon_pwm_input:
> > +		return 0644;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static umode_t axi_fan_control_temp_is_visible(const u32 attr)
> > +{
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +	case hwmon_temp_label:
> > +		return 0444;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static umode_t axi_fan_control_is_visible(const void *data,
> > +					  enum hwmon_sensor_types type,
> > +					  u32 attr, int channel)
> > +{
> > +	switch (type) {
> > +	case hwmon_fan:
> > +		return axi_fan_control_fan_is_visible(attr);
> > +	case hwmon_pwm:
> > +		return axi_fan_control_pwm_is_visible(attr);
> > +	case hwmon_temp:
> > +		return axi_fan_control_temp_is_visible(attr);
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static irqreturn_t axi_fan_control_irq_handler(int irq, void
> > *data)
> > +{
> > +	struct axi_fan_control_data *ctl = (struct axi_fan_control_data
> > *)data;
> > +	u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING,
> > ctl);
> > +	u32 clear_mask;
> > +
> > +	if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) {
> > +		u32 new_tach =
> > axi_fan_control_ioread(ADI_REG_TACH_MEASUR,
> > +						      ctl);
> > +
> > +		if (ctl->update_tacho_params) {
> > +			/* get 25% tolerance */
> > +			u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25,
> > 100);
> > +			/* set new tacho parameters */
> > +			axi_fan_control_iowrite(new_tach,
> > ADI_REG_TACH_PERIOD,
> > +						ctl);
> > +			axi_fan_control_iowrite(tach_tol,
> > +						ADI_REG_TACH_TOLERANCE,
> > ctl);
> > +			ctl->update_tacho_params = false;
> > +		}
> > +
> > +		atomic_set(&ctl->tach, new_tach);
> > +	}
> > +
> > +	if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) {
> > +		/*
> > +		 * if the pwm changes on behalf of software,
> > +		 * we need to provide new tacho parameters to the core.
> > +		 * Wait for the next measurement for that...
> > +		 */
> > +		if (!ctl->hw_pwm_req)
> > +			ctl->update_tacho_params = true;
> > +
> > +		/* just set this to false even if it is already... */
> > +		ctl->hw_pwm_req = false;
> > +	}
> > +
> > +	if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
> > +		/* hardware requested a new pwm */
> > +		ctl->hw_pwm_req = true;
> > +
> I don't really understand the logic here. If
> ADI_IRQ_SRC_TEMP_INCREASE means
> that hardware wants a new pwm, how is userspace informed about that
> request ?

It isn't. Userspace would have to read the pwm attribute and figure
that changed. Should I use something like sysfs_notify() on the pwm
attribute?

> And why are the tacho paramaters _not_ updated in this case later on
> (unless
> ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both set) ?
> It might be useful to describe the expected sequence of events.

The core can change the PWM by itself (which is when we receive
ADI_IRQ_SRC_TEMP_INCREASE) and in that case it will use predefined
values to evaluate the tacho signal (so it won't use the values on
TACH_PERIOD and TACH_TOLERANCE). Alternatively, the user can request a
new PWM by writing the pwm attribute. In this case the CORE is
expecting that TACH_PERIOD and TACH_TOLERANCE are given otherwise it
won't evaluate the tacho signal. Note that when is the user which
requests a new pwm we only get ADI_IRQ_SRC_PWM_CHANGED (and not +	
if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE), so I use that to know
when do I have to update the tacho parameters.
 
> > 
)

> > +	if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
> > +		ctl->fan_fault = 1;
> 
> Is it on purpose that this bit is never reset ?

Yes, and it is wrong. I though that the core would never clear this
alarm but it does clear it in the next temperature reading cycle (and
set it again if needed). Then, would a clear on read be a correct
approach?
> 
> > +
> > +	/* clear all interrupts */
> > +	clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
> > +	axi_fan_control_iowrite(clear_mask, ADI_REG_IRQ_PENDING, ctl);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int axi_fan_control_init(struct axi_fan_control_data *ctl,
> > +				const struct device_node *np)
> > +{
> > +	int ret;
> > +
> > +	/* get fan pulses per revolution */
> > +	ret = of_property_read_u32(np, "adi,pulses-per-revolution",
> > &ctl->ppr);
> > +	if (ret)
> > +		return ret;
> 
> So all random values are acceptable, including 0 and 0xffffffff ?

Yes, I'm aware that 1 and 2 are typical values but I'm not sure what is
the maximum that typically exists so I didn't want to put limits here
without knowing. Though at least 0 must not be accepted since then we
are always dividing by 0 when reading the FAN rpm.

> > +	/*
> > +	 * Enable all IRQs
> > +	 */
> > +	axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL &
> > +			~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR
> > |
> > +			ADI_IRQ_SRC_PWM_CHANGED |
> > ADI_IRQ_SRC_TEMP_INCREASE)),
> 
> One set of ( ) is unnecessary.

ack.

> > +			ADI_REG_IRQ_MASK, ctl);
> > +
> > +	/* bring the device out of reset */
> > +	axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct hwmon_channel_info *axi_fan_control_info[] = {
> > +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT),
> > +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT |
> > HWMON_F_LABEL),
> > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
> > +	NULL
> > +};
> > +
> > +static const struct hwmon_ops axi_fan_control_hwmon_ops = {
> > +	.is_visible = axi_fan_control_is_visible,
> > +	.read = axi_fan_control_read,
> > +	.write = axi_fan_control_write,
> > +	.read_string = axi_fan_control_read_labels,
> > +};
> > +
> > +static const struct hwmon_chip_info axi_fan_control_chip_info = {
> > +	.ops = &axi_fan_control_hwmon_ops,
> > +	.info = axi_fan_control_info,
> > +};
> > +
> > +static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a');
> > +
> > +static const struct of_device_id axi_fan_control_of_match[] = {
> > +	{ .compatible = "adi,axi-fan-control-1.00.a",
> > +		.data = (void *)&version_1_0_0},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, axi_fan_control_of_match);
> > +
> > +static int axi_fan_control_probe(struct platform_device *pdev)
> > +{
> > +	struct device *hwmon_dev;
> > +	struct axi_fan_control_data *ctl;
> > +	const struct of_device_id *id;
> > +	struct resource *res;
> > +	u32 version;
> > +	int ret;
> > +
> > +	id = of_match_node(axi_fan_control_of_match, pdev-
> > >dev.of_node);
> > +	if (!id)
> > +		return -EINVAL;
> > +
> > +	ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL);
> > +	if (!ctl)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	ctl->base = devm_ioremap_resource(&pdev->dev, res);
> 
> devm_platform_ioremap_resource()

ack.

> > +	if (IS_ERR(ctl->base)) {
> > +		dev_err(&pdev->dev, "ioremap failed with %ld\n",
> > +			PTR_ERR(ctl->base));
> > +		return PTR_ERR(ctl->base);
> > +	}
> 
> This does not require an extra error message.

ack.

> > +
> > +	ctl->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(ctl->clk)) {
> > +		dev_err(&pdev->dev, "clk_get failed with %ld\n",
> > +			PTR_ERR(ctl->clk));
> > +		return PTR_ERR(ctl->clk);
> > +	}
> > +
> > +	dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n",
> > +		(unsigned long long)res->start, ctl->base);
> > +
> > +	version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl);
> > +	if (ADI_AXI_PCORE_VER_MAJOR(version) !=
> > +	    ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) {
> > +		dev_err(&pdev->dev, "Major version mismatch. Expected
> > %d.%.2d.%c, Reported %d.%.2d.%c\n",
> > +			ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)),
> > +			ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)),
> > +			ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)),
> > +			ADI_AXI_PCORE_VER_MAJOR(version),
> > +			ADI_AXI_PCORE_VER_MINOR(version),
> > +			ADI_AXI_PCORE_VER_PATCH(version));
> > +		return -ENODEV;
> > +	}
> > +
> > +	ctl->irq = platform_get_irq(pdev, 0);
> > +	if (ctl->irq < 0) {
> 
> This can return -EPROBE_DEFER. On top of that, it already generates
> an error
> message, meaning another one here is unnecessary.

ack.

> > +		dev_err(&pdev->dev, "platform_get_irq failed with
> > %d\n",
> > +			ctl->irq);
> > +		return ctl->irq;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL,
> > +					axi_fan_control_irq_handler,
> > +					IRQF_ONESHOT |
> > IRQF_TRIGGER_HIGH,
> > +					pdev->driver_override, ctl);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to request an irq, %d",
> > ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = axi_fan_control_init(ctl, pdev->dev.of_node);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to initialize device\n");
> > +		return ret;
> > +	}
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> > +						"axi_fan_control",
> > +						ctl,
> > +						&axi_fan_control_chip_i
> > nfo,
> > +						NULL);
> 
> Another alignment issue. Shorter function and global variable names
> would help.

ack.

> > +
> > +	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static struct platform_driver axi_fan_control_driver = {
> > +	.driver = {
> > +		.name = "axi_fan_control_driver",
> > +		.of_match_table = axi_fan_control_of_match,
> > +	},
> > +	.probe = axi_fan_control_probe,
> > +};
> > +module_platform_driver(axi_fan_control_driver);
> > +
> > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
> > +MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.23.0
> > 

Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP

[Guenter Roeck]

On 10/7/19 6:52 AM, Sa, Nuno wrote:
[ ... ]
>>> +static long axi_fan_control_get_pwm_duty(const struct
>>> axi_fan_control_data *ctl)
>>> +{
>>> +	u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
>>> +	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
>>> ctl);
>>> +
>>> +	return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX,
>>> pwm_period);
>>
>> Is pwm_period guaranteed to be != 0 ?
> 
> Yes, It is a RO register and it is set by the core with the default of
> 0x4e20.

Trusting the hardware doesn't make me too comfortable. Are we sure at all
times that the HW isn't messed up ? If so, please at least add a comment
stating that the HW will never return 0. We can then fix it after we get
the first crash report from the field ;-).

[ ... ]

>>> +	if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
>>> +		/* hardware requested a new pwm */
>>> +		ctl->hw_pwm_req = true;
>>> +
>> I don't really understand the logic here. If
>> ADI_IRQ_SRC_TEMP_INCREASE means
>> that hardware wants a new pwm, how is userspace informed about that
>> request ?
> 
> It isn't. Userspace would have to read the pwm attribute and figure
> that changed. Should I use something like sysfs_notify() on the pwm
> attribute?
> 
That might make sense.

>> And why are the tacho paramaters _not_ updated in this case later on
>> (unless
>> ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both set) ?
>> It might be useful to describe the expected sequence of events.
> 
> The core can change the PWM by itself (which is when we receive
> ADI_IRQ_SRC_TEMP_INCREASE) and in that case it will use predefined
> values to evaluate the tacho signal (so it won't use the values on
> TACH_PERIOD and TACH_TOLERANCE). Alternatively, the user can request a
> new PWM by writing the pwm attribute. In this case the CORE is
> expecting that TACH_PERIOD and TACH_TOLERANCE are given otherwise it
> won't evaluate the tacho signal. Note that when is the user which
> requests a new pwm we only get ADI_IRQ_SRC_PWM_CHANGED (and not +	
> if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE), so I use that to know
> when do I have to update the tacho parameters.
>   
Wondering ... if setting the pwm requires an update of period and tolerance,
why not set update_tacho_params to true when the pwm value is written, or
update the registers directly instead of waiting for an interrupt ?

Either case, I think the above sequence of events should be explained
in the driver for future developers to understand why the code is written
the way it is.

>>>
> )
> 
>>> +	if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
>>> +		ctl->fan_fault = 1;
>>
>> Is it on purpose that this bit is never reset ?
> 
> Yes, and it is wrong. I though that the core would never clear this
> alarm but it does clear it in the next temperature reading cycle (and
> set it again if needed). Then, would a clear on read be a correct
> approach?

Not sure if there is a "correct", but I think it would make sense.

>>
>>> +
>>> +	/* clear all interrupts */
>>> +	clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
>>> +	axi_fan_control_iowrite(clear_mask, ADI_REG_IRQ_PENDING, ctl);
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int axi_fan_control_init(struct axi_fan_control_data *ctl,
>>> +				const struct device_node *np)
>>> +{
>>> +	int ret;
>>> +
>>> +	/* get fan pulses per revolution */
>>> +	ret = of_property_read_u32(np, "adi,pulses-per-revolution",
>>> &ctl->ppr);
>>> +	if (ret)
>>> +		return ret;
>>
>> So all random values are acceptable, including 0 and 0xffffffff ?
> 
> Yes, I'm aware that 1 and 2 are typical values but I'm not sure what is
> the maximum that typically exists so I didn't want to put limits here
> without knowing. Though at least 0 must not be accepted since then we
> are always dividing by 0 when reading the FAN rpm.
> 
The only values I am aware of are 2 and 4. I don't recall seeing any fans
with 1 pulse per revolution. Overall, I don't think values other than 1, 2,
and 4 make sense.

Guenter

Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP

[Sa, Nuno]

On Mon, 2019-10-07 at 07:18 -0700, Guenter Roeck wrote:

> 
> On 10/7/19 6:52 AM, Sa, Nuno wrote:
> [ ... ]
> > > > +static long axi_fan_control_get_pwm_duty(const struct
> > > > axi_fan_control_data *ctl)
> > > > +{
> > > > +	u32 pwm_width =
> > > > axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
> > > > +	u32 pwm_period =
> > > > axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
> > > > ctl);
> > > > +
> > > > +	return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX,
> > > > pwm_period);
> > > 
> > > Is pwm_period guaranteed to be != 0 ?
> > 
> > Yes, It is a RO register and it is set by the core with the default
> > of
> > 0x4e20.
> 
> Trusting the hardware doesn't make me too comfortable. Are we sure at
> all
> times that the HW isn't messed up ? If so, please at least add a
> comment
> stating that the HW will never return 0. We can then fix it after we
> get
> the first crash report from the field ;-).

Will do that.

> [ ... ]
> 
> > > > +	if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
> > > > +		/* hardware requested a new pwm */
> > > > +		ctl->hw_pwm_req = true;
> > > > +
> > > I don't really understand the logic here. If
> > > ADI_IRQ_SRC_TEMP_INCREASE means
> > > that hardware wants a new pwm, how is userspace informed about
> > > that
> > > request ?
> > 
> > It isn't. Userspace would have to read the pwm attribute and figure
> > that changed. Should I use something like sysfs_notify() on the pwm
> > attribute?
> > 
> That might make sense.
> 
> > > And why are the tacho paramaters _not_ updated in this case later
> > > on
> > > (unless
> > > ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both
> > > set) ?
> > > It might be useful to describe the expected sequence of events.
> > 
> > The core can change the PWM by itself (which is when we receive
> > ADI_IRQ_SRC_TEMP_INCREASE) and in that case it will use predefined
> > values to evaluate the tacho signal (so it won't use the values on
> > TACH_PERIOD and TACH_TOLERANCE). Alternatively, the user can
> > request a
> > new PWM by writing the pwm attribute. In this case the CORE is
> > expecting that TACH_PERIOD and TACH_TOLERANCE are given otherwise
> > it
> > won't evaluate the tacho signal. Note that when is the user which
> > requests a new pwm we only get ADI_IRQ_SRC_PWM_CHANGED (and not +	
> > if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE), so I use that to know
> > when do I have to update the tacho parameters.
> >   
> Wondering ... if setting the pwm requires an update of period and
> tolerance,
> why not set update_tacho_params to true when the pwm value is
> written, or
> update the registers directly instead of waiting for an interrupt ?

After requesting a new duty-cycle there is 5s delay on which the core
waits for the fan rotation speed to stabilize. Only after that, we have
to provide the tach parameters. Also note that we do need to use the
updated tach measurement value to derive this parameters. So, we need
to wait for the ADI_IRQ_SRC_NEW_MEASUR interrupt.

> Either case, I think the above sequence of events should be explained
> in the driver for future developers to understand why the code is
> written
> the way it is.

will do that.

> > )
> > 
> > > > +	if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
> > > > +		ctl->fan_fault = 1;
> > > 
> > > Is it on purpose that this bit is never reset ?
> > 
> > Yes, and it is wrong. I though that the core would never clear this
> > alarm but it does clear it in the next temperature reading cycle
> > (and
> > set it again if needed). Then, would a clear on read be a correct
> > approach?
> 
> Not sure if there is a "correct", but I think it would make sense.
> 
> > > > +
> > > > +	/* clear all interrupts */
> > > > +	clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
> > > > +	axi_fan_control_iowrite(clear_mask,
> > > > ADI_REG_IRQ_PENDING, ctl);
> > > > +
> > > > +	return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +static int axi_fan_control_init(struct axi_fan_control_data
> > > > *ctl,
> > > > +				const struct device_node *np)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	/* get fan pulses per revolution */
> > > > +	ret = of_property_read_u32(np, "adi,pulses-per-
> > > > revolution",
> > > > &ctl->ppr);
> > > > +	if (ret)
> > > > +		return ret;
> > > 
> > > So all random values are acceptable, including 0 and 0xffffffff ?
> > 
> > Yes, I'm aware that 1 and 2 are typical values but I'm not sure
> > what is
> > the maximum that typically exists so I didn't want to put limits
> > here
> > without knowing. Though at least 0 must not be accepted since then
> > we
> > are always dividing by 0 when reading the FAN rpm.
> > 
> The only values I am aware of are 2 and 4. I don't recall seeing any
> fans
> with 1 pulse per revolution. Overall, I don't think values other than
> 1, 2,
> and 4 make sense.

 Will use 1,2 and 4 and update dt bindings accordingly.

> Guenter

Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP

[Sa, Nuno]

Hi Guenter,

One question/doubt that I just noticed now...

On Sun, 2019-10-06 at 08:32 -0700, Guenter Roeck wrote:
> 
> On Thu, Sep 26, 2019 at 12:39:24PM +0200, Nuno Sá wrote:
> > The purpose of this IP Core is to control the fan used for the
> > cooling of a
> > Xilinx Zynq Ultrascale+ MPSoC without the need of any external
> > temperature
> > sensors. To achieve this, the IP core uses the PL SYSMONE4
> > primitive to
> > obtain the PL temperature and, based on those readings, it then
> > outputs
> > a PWM signal to control the fan rotation accordingly.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  MAINTAINERS                     |   7 +
> >  drivers/hwmon/Kconfig           |   9 +
> >  drivers/hwmon/Makefile          |   1 +
> >  drivers/hwmon/axi-fan-control.c | 452
> > ++++++++++++++++++++++++++++++++
> >  4 files changed, 469 insertions(+)
> >  create mode 100644 drivers/hwmon/axi-fan-control.c
> > 
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index c7035ce2460b..d775c923d23b 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -2853,6 +2853,13 @@ S:	Maintained
> >  F:	Documentation/devicetree/bindings/sound/axentia,*
> >  F:	sound/soc/atmel/tse850-pcm5142.c
> >  
> > +AXI-FAN-CONTROL HARDWARE MONITOR DRIVER
> > +M:	Nuno Sá <nuno.sa@analog.com>
> > +W:	http://ez.analog.com/community/linux-device-drivers
> > +L:	linux-hwmon@vger.kernel.org
> > +S:	Supported
> > +F:	drivers/hwmon/axi-fan-control.c
> > +
> >  AXXIA I2C CONTROLLER
> >  M:	Krzysztof Adamski <krzysztof.adamski@nokia.com>
> >  L:	linux-i2c@vger.kernel.org
> > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> > index 2ca5668bdb62..2396cc347c47 100644
> > --- a/drivers/hwmon/Kconfig
> > +++ b/drivers/hwmon/Kconfig
> > @@ -269,6 +269,15 @@ config SENSORS_ASC7621
> >  	  This driver can also be built as a module. If so, the module
> >  	  will be called asc7621.
> >  
> > +config SENSORS_AXI_FAN_CONTROL
> > +	tristate "Analog Devices FAN Control HDL Core driver"
> > +	help
> > +	  If you say yes here you get support for the Analog Devices
> > +	  AXI HDL FAN monitoring core.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called axi-fan-control
> > +
> >  config SENSORS_K8TEMP
> >  	tristate "AMD Athlon64/FX or Opteron temperature sensor"
> >  	depends on X86 && PCI
> > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> > index c86ce4d3d36b..ebb1fd2ea82b 100644
> > --- a/drivers/hwmon/Makefile
> > +++ b/drivers/hwmon/Makefile
> > @@ -51,6 +51,7 @@ obj-$(CONFIG_SENSORS_AS370)	+= as370-
> > hwmon.o
> >  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> >  obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
> >  obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
> > +obj-$(CONFIG_SENSORS_AXI_FAN_CONTROL) += axi-fan-control.o
> >  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
> >  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> >  obj-$(CONFIG_SENSORS_DA9055)+= da9055-hwmon.o
> > diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-
> > fan-control.c
> > new file mode 100644
> > index 000000000000..f86efc444753
> > --- /dev/null
> > +++ b/drivers/hwmon/axi-fan-control.c
> > @@ -0,0 +1,452 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Fan Control HDL CORE driver
> > + *
> > + * Copyright 2019 Analog Devices Inc.
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/fpga/adi-axi-common.h>
> > +#include <linux/hwmon.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/platform_device.h>
> > +
> > +/* register map */
> > +#define ADI_REG_RSTN		0x0080
> > +#define ADI_REG_PWM_WIDTH	0x0084
> > +#define ADI_REG_TACH_PERIOD	0x0088
> > +#define ADI_REG_TACH_TOLERANCE	0x008c
> > +#define ADI_REG_PWM_PERIOD	0x00c0
> > +#define ADI_REG_TACH_MEASUR	0x00c4
> > +#define ADI_REG_TEMPERATURE	0x00c8
> > +
> > +#define ADI_REG_IRQ_MASK	0x0040
> > +#define ADI_REG_IRQ_PENDING	0x0044
> > +#define ADI_REG_IRQ_SRC		0x0048
> > +
> > +/* IRQ sources */
> > +#define ADI_IRQ_SRC_PWM_CHANGED		BIT(0)
> 
> include linux/bits.h
> 
> > +#define ADI_IRQ_SRC_TACH_ERR		BIT(1)
> > +#define ADI_IRQ_SRC_TEMP_INCREASE	BIT(2)
> > +#define ADI_IRQ_SRC_NEW_MEASUR		BIT(3)
> > +#define ADI_IRQ_SRC_MASK		GENMASK(3, 0)
> > +#define ADI_IRQ_MASK_OUT_ALL		0xFFFFFFFFU
> > +
> > +#define SYSFS_PWM_MAX			255
> > +
> > +struct axi_fan_control_data {
> > +	struct clk *clk;
> > +	void __iomem *base;
> > +	/* tacho period */
> > +	atomic_t tach;
> 
> Why exactly is this an atomic ? The value is only set, and it is set
> in
> a single operation. Why does it matter if reqading it catches the old
> or
> the new value ?
> 
> > +	int irq;
> > +	/* pulses per revolution */
> > +	u32 ppr;
> > +	bool hw_pwm_req;
> > +	bool update_tacho_params;
> > +	u8 fan_fault;
> > +};
> > +
> > +static inline void axi_fan_control_iowrite(const u32 val, const
> > u32 reg,
> > +				const struct axi_fan_control_data *ctl)
> 
> Multi-line alignment. Also, please consider shorter function names.
> An "axi_fan_control_" prefix for static functions is really a bit
> excessive
> and doesn't add value. "axi_" would do it just as well.
> 
> > +{
> > +	iowrite32(val, ctl->base + reg);
> > +}
> > +
> > +static inline u32 axi_fan_control_ioread(const u32 reg,
> > +					 const struct
> > axi_fan_control_data *ctl)
> > +{
> > +	return ioread32(ctl->base + reg);
> > +}
> > +
> > +static long axi_fan_control_get_pwm_duty(const struct
> > axi_fan_control_data *ctl)
> > +{
> > +	u32 pwm_width = axi_fan_control_ioread(ADI_REG_PWM_WIDTH, ctl);
> > +	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
> > ctl);
> > +
> > +	return DIV_ROUND_CLOSEST(pwm_width * SYSFS_PWM_MAX,
> > pwm_period);
> 
> Is pwm_period guaranteed to be != 0 ?
> 
> > +}
> > +
> > +static int axi_fan_control_set_pwm_duty(const long val,
> > +					struct axi_fan_control_data
> > *ctl)
> > +{
> > +	u32 pwm_period = axi_fan_control_ioread(ADI_REG_PWM_PERIOD,
> > ctl);
> > +	u32 new_width;
> > +	long __val = clamp_val(val, 0, SYSFS_PWM_MAX);
> > +
> > +	new_width = DIV_ROUND_CLOSEST(__val * pwm_period,
> > SYSFS_PWM_MAX);
> > +
> > +	axi_fan_control_iowrite(new_width, ADI_REG_PWM_WIDTH, ctl);
> > +
> > +	return 0;
> > +}
> > +
> > +static long axi_fan_control_get_fan_rpm(const struct
> > axi_fan_control_data *ctl)
> > +{
> > +	unsigned long clk_rate = clk_get_rate(ctl->clk);
> > +
> > +	if (!clk_rate)
> > +		return -EINVAL;
> 
> Unless the clock rate changes dynamically, it might make sense to
> read it once
> in the probe function.
> 
> > +	/*
> > +	 * The tacho period should be:
> > +	 *      TACH = 60/(ppr * rpm), where rpm is revolutions per
> > second
> > +	 *      and ppr is pulses per revolution.
> > +	 * Given the tacho period, we can multiply it by the input
> > clock
> > +	 * so that we know how many clocks we need to have this period.
> > +	 * From this, we can derive the RPM value.
> > +	 */
> > +	return DIV_ROUND_CLOSEST(60 * clk_rate,
> > +				 ctl->ppr * atomic_read(&ctl->tach));
> 
> Are ppr and tach guaranteed to be != 0 ? I don't think so,
> since neither is ever evaluated.
> 
> > +}
> > +
> > +static int axi_fan_control_read_temp(struct device *dev, u32 attr,
> > long *val)
> > +{
> > +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> > +	long raw_temp;
> > +
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +		raw_temp = axi_fan_control_ioread(ADI_REG_TEMPERATURE,
> > ctl);
> > +		/*
> > +		 * The formula for the temperature is:
> > +		 *      T = (ADC * 501.3743 / 2^bits) - 273.6777
> > +		 * It's multiplied by 1000 to have milidegrees as
> 
> s/milidegrees/millidegrees/
> 
> > +		 * specified by the hwmon sysfs interface.
> > +		 */
> > +		*val = ((raw_temp * 501374) >> 16) - 273677;
> > +		return 0;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_read_fan(struct device *dev, u32 attr,
> > long *val)
> > +{
> > +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_fan_fault:
> > +		*val = ctl->fan_fault;
> > +		return 0;
> > +	case hwmon_fan_input:
> > +		*val = axi_fan_control_get_fan_rpm(ctl);
> > +		return 0;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_read_pwm(struct device *dev, u32 attr,
> > long *val)
> > +{
> > +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_pwm_input:
> > +		*val = axi_fan_control_get_pwm_duty(ctl);
> > +		return 0;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_write_pwm(struct device *dev, u32 attr,
> > long val)
> > +{
> > +	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
> > +
> > +	switch (attr) {
> > +	case hwmon_pwm_input:
> > +		return axi_fan_control_set_pwm_duty(val, ctl);
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_read_labels(struct device *dev,
> > +				       enum hwmon_sensor_types type,
> > +				       u32 attr, int channel, const
> > char **str)
> > +{
> > +	switch (type) {
> > +	case hwmon_fan:
> > +		*str = "FAN";
> > +		return 0;
> > +	case hwmon_temp:
> > +		*str = "SYSMON4";
> > +		return 0;
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_read(struct device *dev,
> > +				enum hwmon_sensor_types type,
> > +				u32 attr, int channel, long *val)
> > +{
> > +	switch (type) {
> > +	case hwmon_fan:
> > +		return axi_fan_control_read_fan(dev, attr, val);
> > +	case hwmon_pwm:
> > +		return axi_fan_control_read_pwm(dev, attr, val);
> > +	case hwmon_temp:
> > +		return axi_fan_control_read_temp(dev, attr, val);
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static int axi_fan_control_write(struct device *dev,
> > +				 enum hwmon_sensor_types type,
> > +				 u32 attr, int channel, long val)
> > +{
> > +	switch (type) {
> > +	case hwmon_pwm:
> > +		return axi_fan_control_write_pwm(dev, attr, val);
> > +	default:
> > +		return -ENOTSUPP;
> > +	}
> > +}
> > +
> > +static umode_t axi_fan_control_fan_is_visible(const u32 attr)
> > +{
> > +	switch (attr) {
> > +	case hwmon_fan_input:
> > +	case hwmon_fan_fault:
> > +	case hwmon_fan_label:
> > +		return 0444;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static umode_t axi_fan_control_pwm_is_visible(const u32 attr)
> > +{
> > +	switch (attr) {
> > +	case hwmon_pwm_input:
> > +		return 0644;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static umode_t axi_fan_control_temp_is_visible(const u32 attr)
> > +{
> > +	switch (attr) {
> > +	case hwmon_temp_input:
> > +	case hwmon_temp_label:
> > +		return 0444;
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static umode_t axi_fan_control_is_visible(const void *data,
> > +					  enum hwmon_sensor_types type,
> > +					  u32 attr, int channel)
> > +{
> > +	switch (type) {
> > +	case hwmon_fan:
> > +		return axi_fan_control_fan_is_visible(attr);
> > +	case hwmon_pwm:
> > +		return axi_fan_control_pwm_is_visible(attr);
> > +	case hwmon_temp:
> > +		return axi_fan_control_temp_is_visible(attr);
> > +	default:
> > +		return 0;
> > +	}
> > +}
> > +
> > +static irqreturn_t axi_fan_control_irq_handler(int irq, void
> > *data)
> > +{
> > +	struct axi_fan_control_data *ctl = (struct axi_fan_control_data
> > *)data;
> > +	u32 irq_pending = axi_fan_control_ioread(ADI_REG_IRQ_PENDING,
> > ctl);
> > +	u32 clear_mask;
> > +
> > +	if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) {
> > +		u32 new_tach =
> > axi_fan_control_ioread(ADI_REG_TACH_MEASUR,
> > +						      ctl);
> > +
> > +		if (ctl->update_tacho_params) {
> > +			/* get 25% tolerance */
> > +			u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25,
> > 100);
> > +			/* set new tacho parameters */
> > +			axi_fan_control_iowrite(new_tach,
> > ADI_REG_TACH_PERIOD,
> > +						ctl);
> > +			axi_fan_control_iowrite(tach_tol,
> > +						ADI_REG_TACH_TOLERANCE,
> > ctl);
> > +			ctl->update_tacho_params = false;
> > +		}
> > +
> > +		atomic_set(&ctl->tach, new_tach);
> > +	}
> > +
> > +	if (irq_pending & ADI_IRQ_SRC_PWM_CHANGED) {
> > +		/*
> > +		 * if the pwm changes on behalf of software,
> > +		 * we need to provide new tacho parameters to the core.
> > +		 * Wait for the next measurement for that...
> > +		 */
> > +		if (!ctl->hw_pwm_req)
> > +			ctl->update_tacho_params = true;
> > +
> > +		/* just set this to false even if it is already... */
> > +		ctl->hw_pwm_req = false;
> > +	}
> > +
> > +	if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
> > +		/* hardware requested a new pwm */
> > +		ctl->hw_pwm_req = true;
> > +
> I don't really understand the logic here. If
> ADI_IRQ_SRC_TEMP_INCREASE means
> that hardware wants a new pwm, how is userspace informed about that
> request ?
> And why are the tacho paramaters _not_ updated in this case later on
> (unless
> ADI_IRQ_SRC_PWM_CHANGED and ADI_IRQ_SRC_TEMP_INCREASE are both set) ?
> It might be useful to describe the expected sequence of events.
> 
> > +	if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
> > +		ctl->fan_fault = 1;
> 
> Is it on purpose that this bit is never reset ?
> 
> > +
> > +	/* clear all interrupts */
> > +	clear_mask = irq_pending & ADI_IRQ_SRC_MASK;
> > +	axi_fan_control_iowrite(clear_mask, ADI_REG_IRQ_PENDING, ctl);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int axi_fan_control_init(struct axi_fan_control_data *ctl,
> > +				const struct device_node *np)
> > +{
> > +	int ret;
> > +
> > +	/* get fan pulses per revolution */
> > +	ret = of_property_read_u32(np, "adi,pulses-per-revolution",
> > &ctl->ppr);
> > +	if (ret)
> > +		return ret;
> 
> So all random values are acceptable, including 0 and 0xffffffff ?
> 
> > +	/*
> > +	 * Enable all IRQs
> > +	 */
> > +	axi_fan_control_iowrite((ADI_IRQ_MASK_OUT_ALL &
> > +			~(ADI_IRQ_SRC_NEW_MEASUR | ADI_IRQ_SRC_TACH_ERR
> > |
> > +			ADI_IRQ_SRC_PWM_CHANGED |
> > ADI_IRQ_SRC_TEMP_INCREASE)),
> 
> One set of ( ) is unnecessary.
> 
> > +			ADI_REG_IRQ_MASK, ctl);
> > +
> > +	/* bring the device out of reset */
> > +	axi_fan_control_iowrite(0x01, ADI_REG_RSTN, ctl);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct hwmon_channel_info *axi_fan_control_info[] = {
> > +	HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT),
> > +	HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT | HWMON_F_FAULT |
> > HWMON_F_LABEL),
> > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
> > +	NULL
> > +};
> > +
> > +static const struct hwmon_ops axi_fan_control_hwmon_ops = {
> > +	.is_visible = axi_fan_control_is_visible,
> > +	.read = axi_fan_control_read,
> > +	.write = axi_fan_control_write,
> > +	.read_string = axi_fan_control_read_labels,
> > +};
> > +
> > +static const struct hwmon_chip_info axi_fan_control_chip_info = {
> > +	.ops = &axi_fan_control_hwmon_ops,
> > +	.info = axi_fan_control_info,
> > +};
> > +
> > +static const u32 version_1_0_0 = ADI_AXI_PCORE_VER(1, 0, 'a');
> > +
> > +static const struct of_device_id axi_fan_control_of_match[] = {
> > +	{ .compatible = "adi,axi-fan-control-1.00.a",
> > +		.data = (void *)&version_1_0_0},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, axi_fan_control_of_match);
> > +
> > +static int axi_fan_control_probe(struct platform_device *pdev)
> > +{
> > +	struct device *hwmon_dev;
> > +	struct axi_fan_control_data *ctl;
> > +	const struct of_device_id *id;
> > +	struct resource *res;
> > +	u32 version;
> > +	int ret;
> > +
> > +	id = of_match_node(axi_fan_control_of_match, pdev-
> > >dev.of_node);
> > +	if (!id)
> > +		return -EINVAL;
> > +
> > +	ctl = devm_kzalloc(&pdev->dev, sizeof(*ctl), GFP_KERNEL);
> > +	if (!ctl)
> > +		return -ENOMEM;
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	ctl->base = devm_ioremap_resource(&pdev->dev, res);
> 
> devm_platform_ioremap_resource()
> 
> > +	if (IS_ERR(ctl->base)) {
> > +		dev_err(&pdev->dev, "ioremap failed with %ld\n",
> > +			PTR_ERR(ctl->base));
> > +		return PTR_ERR(ctl->base);
> > +	}
> 
> This does not require an extra error message.
> 
> > +
> > +	ctl->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(ctl->clk)) {
> > +		dev_err(&pdev->dev, "clk_get failed with %ld\n",
> > +			PTR_ERR(ctl->clk));
> > +		return PTR_ERR(ctl->clk);
> > +	}
> > +
> > +	dev_dbg(&pdev->dev, "Re-mapped from 0x%08llX to %p\n",
> > +		(unsigned long long)res->start, ctl->base);
> > +
> > +	version = axi_fan_control_ioread(ADI_AXI_REG_VERSION, ctl);
> > +	if (ADI_AXI_PCORE_VER_MAJOR(version) !=
> > +	    ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data))) {
> > +		dev_err(&pdev->dev, "Major version mismatch. Expected
> > %d.%.2d.%c, Reported %d.%.2d.%c\n",
> > +			ADI_AXI_PCORE_VER_MAJOR((*(u32 *)id->data)),
> > +			ADI_AXI_PCORE_VER_MINOR((*(u32 *)id->data)),
> > +			ADI_AXI_PCORE_VER_PATCH((*(u32 *)id->data)),
> > +			ADI_AXI_PCORE_VER_MAJOR(version),
> > +			ADI_AXI_PCORE_VER_MINOR(version),
> > +			ADI_AXI_PCORE_VER_PATCH(version));
> > +		return -ENODEV;
> > +	}
> > +
> > +	ctl->irq = platform_get_irq(pdev, 0);
> > +	if (ctl->irq < 0) {
> 
> This can return -EPROBE_DEFER. On top of that, it already generates
> an error
> message, meaning another one here is unnecessary.

Why returning -EPROBE_DEFER? platform_get_irq() already seems to handle
EPROBE_DEFER...

> > +		dev_err(&pdev->dev, "platform_get_irq failed with
> > %d\n",
> > +			ctl->irq);
> > +		return ctl->irq;
> > +	}
> > +
> > +	ret = devm_request_threaded_irq(&pdev->dev, ctl->irq, NULL,
> > +					axi_fan_control_irq_handler,
> > +					IRQF_ONESHOT |
> > IRQF_TRIGGER_HIGH,
> > +					pdev->driver_override, ctl);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "failed to request an irq, %d",
> > ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = axi_fan_control_init(ctl, pdev->dev.of_node);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to initialize device\n");
> > +		return ret;
> > +	}
> > +
> > +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> > +						"axi_fan_control",
> > +						ctl,
> > +						&axi_fan_control_chip_i
> > nfo,
> > +						NULL);
> 
> Another alignment issue. Shorter function and global variable names
> would help.
> 
> > +
> > +	return PTR_ERR_OR_ZERO(hwmon_dev);
> > +}
> > +
> > +static struct platform_driver axi_fan_control_driver = {
> > +	.driver = {
> > +		.name = "axi_fan_control_driver",
> > +		.of_match_table = axi_fan_control_of_match,
> > +	},
> > +	.probe = axi_fan_control_probe,
> > +};
> > +module_platform_driver(axi_fan_control_driver);
> > +
> > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
> > +MODULE_DESCRIPTION("Analog Devices Fan Control HDL CORE driver");
> > +MODULE_LICENSE("GPL");
> > -- 
> > 2.23.0
> > 

Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP

[Guenter Roeck]

On Tue, Oct 08, 2019 at 03:59:49PM +0000, Sa, Nuno wrote:
[ ... ]
> > > +
> > > +	ctl->irq = platform_get_irq(pdev, 0);
> > > +	if (ctl->irq < 0) {
> > 
> > This can return -EPROBE_DEFER. On top of that, it already generates
> > an error
> > message, meaning another one here is unnecessary.
> 
> Why returning -EPROBE_DEFER? platform_get_irq() already seems to handle
> EPROBE_DEFER...
> 
... which is exactly why I am saying that you don't need another error
message, and that an unconditional error message is a bad idea.

Guenter

Re: [PATCH 2/3] hwmon: Support ADI Fan Control IP

[Sa, Nuno]

On Tue, 2019-10-08 at 13:11 -0700, Guenter Roeck wrote:
> 
> On Tue, Oct 08, 2019 at 03:59:49PM +0000, Sa, Nuno wrote:
> [ ... ]
> > > > +
> > > > +	ctl->irq = platform_get_irq(pdev, 0);
> > > > +	if (ctl->irq < 0) {
> > > 
> > > This can return -EPROBE_DEFER. On top of that, it already
> > > generates
> > > an error
> > > message, meaning another one here is unnecessary.
> > 
> > Why returning -EPROBE_DEFER? platform_get_irq() already seems to
> > handle
> > EPROBE_DEFER...
> > 
> ... which is exactly why I am saying that you don't need another
> error
> message, and that an unconditional error message is a bad idea.
> 

Ahhh, sorry. I completely misunderstood you :(

Nuno Sá

> Guenter

[PATCH 3/3] dt-bindings: hwmon: Add AXI FAN Control documentation

[Nuno Sá]

Document the AXI FAN Control IP core devicetree bindings.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../bindings/hwmon/adi,axi-fan-control.yaml   | 58 +++++++++++++++++++
 MAINTAINERS                                   |  1 +
 2 files changed, 59 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
new file mode 100644
index 000000000000..03948d09f600
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright 2019 Analog Devices Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/bindings/hwmon/adi,axi-fan-control.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AXI FAN Control Device Tree Bindings
+
+maintainers:
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |+
+  Bindings for the Analog Devices AXI FAN Control driver. Spefications of the
+  core can be found in:
+
+  https://wiki.analog.com/resources/fpga/docs/axi_fan_control
+
+properties:
+  compatible:
+    enum:
+        - adi,axi-fan-control-1.00.a
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  adi,pulses-per-revolution:
+    description: Value specifying the number of pulses per revolution of the controlled FAN.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - interrupts
+  - adi,pulses-per-revolution
+
+examples:
+  - |
+    fpga_axi: fpga-axi@0 {
+            #address-cells = <0x2>;
+            #size-cells = <0x1>;
+
+            axi_fan_control: axi-fan-control@80000000 {
+                    compatible = "adi,axi-pulse-capture-1.00.a";
+                    reg = <0x0 0x80000000 0x10000>;
+                    clocks = <&clk 71>;
+                    interrupts = <0 110 0>;
+                    adi,pulses-per-revolution = <2>;
+            };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index d775c923d23b..ecef8dc5f0b5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2859,6 +2859,7 @@ W:	http://ez.analog.com/community/linux-device-drivers
 L:	linux-hwmon@vger.kernel.org
 S:	Supported
 F:	drivers/hwmon/axi-fan-control.c
+F:	Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
 
 AXXIA I2C CONTROLLER
 M:	Krzysztof Adamski <krzysztof.adamski@nokia.com>
-- 
2.23.0