[RFC PATCH 0/6] AXI FAN new features and improvements

[RFC PATCH 0/6] AXI FAN new features and improvements

[Nuno Sá]

This series adds some new features to the axi-fan-control driver. On top
of that, the HW had some changes (basically it now starts automatically
out of reset) so that the driver needed some minor refactoring. The
reason I'm sending this as RFC, is mainly because of the last patch
("hwmon: axi-fan-control: support temperature vs pwm points"). The core
has some predefined values which define a temperature vs pwm curve [1].
It also exposes registers so that users can change it according to their
needs. As I could not find standard attributes in the subsystem, I'm
proposing some "raw" sysfs files. Looking at [2], the pwm_auto_point
stuff looked to be what I want. Obviously I might be wrong :). If this
is accepted, I will add a proper sysfs DOC file describing the new files
(being lazy in the RFC).

For patch 5 ("hwmon: axi-fan-control: clear the fan fault irq at startup"),
it's also arguable if we really need it. The main reason I have it is
because of some userland apps that might take some drastic measures by
just reading 1 fan_fault alarm. Obviously, we can argue that the problem
is in the app and not in the driver. Though it's such a minimal change
that I decided to include it (I'm more than fine in dropping the patch).

[1]: https://wiki.analog.com/resources/fpga/docs/axi_fan_control
[2]: https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface

Nuno Sá (6):
  hwmon: axi-fan-control: make sure the clock is enabled
  hwmon: axi-fan-control: add tacho devicetree properties
  dt-bindings: axi-fan-control: add tacho properties
  hwmon: axi-fan-control: handle irqs in natural order
  hwmon: axi-fan-control: clear the fan fault irq at startup
  hwmon: axi-fan-control: support temperature vs pwm points

 .../bindings/hwmon/adi,axi-fan-control.yaml   |  12 ++
 drivers/hwmon/axi-fan-control.c               | 160 ++++++++++++++++--
 2 files changed, 156 insertions(+), 16 deletions(-)

-- 
2.32.0

[RFC PATCH 1/6] hwmon: axi-fan-control: make sure the clock is enabled

[Nuno Sá]

The core will only work if it's clock is enabled. This patch is a
minor enhancement to make sure that's the case.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/hwmon/axi-fan-control.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
index e3f6b03e6764..901d1588234d 100644
--- a/drivers/hwmon/axi-fan-control.c
+++ b/drivers/hwmon/axi-fan-control.c
@@ -351,6 +351,11 @@ static int axi_fan_control_init(struct axi_fan_control_data *ctl,
 	return ret;
 }
 
+static void axi_fan_control_clk_disable(void *clk)
+{
+	clk_disable_unprepare(clk);
+}
+
 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),
@@ -406,6 +411,14 @@ static int axi_fan_control_probe(struct platform_device *pdev)
 		return PTR_ERR(clk);
 	}
 
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(&pdev->dev, axi_fan_control_clk_disable, clk);
+	if (ret)
+		return ret;
+
 	ctl->clk_rate = clk_get_rate(clk);
 	if (!ctl->clk_rate)
 		return -EINVAL;
-- 
2.32.0

[RFC PATCH 2/6] hwmon: axi-fan-control: add tacho devicetree properties

[Nuno Sá]

This core is capable to run without any user interaction and when it
does, it uses some predefined values in order to evaluate the tacho
signal from the FAN. These values depend on the attached FAN, so that
the core now exposes registers to change them accordingly. The values
are:

 * adi,tacho-25-us: Expected tacho signal when the PWM is 25%;
 * adi,tacho-50-us: Expected tacho signal when the PWM is 50%;
 * adi,tacho-75-us: Expected tacho signal when the PWM is 75%;
 * adi,tacho-100-us: Expected tacho signal when the PWM is 100%.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/hwmon/axi-fan-control.c | 34 +++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
index 901d1588234d..aa5a922f684d 100644
--- a/drivers/hwmon/axi-fan-control.c
+++ b/drivers/hwmon/axi-fan-control.c
@@ -23,6 +23,14 @@
 #define ADI_REG_PWM_PERIOD	0x00c0
 #define ADI_REG_TACH_MEASUR	0x00c4
 #define ADI_REG_TEMPERATURE	0x00c8
+#define ADI_REG_TACH_25		0x0140
+#define ADI_REG_TACH_50		0x0144
+#define ADI_REG_TACH_75		0x0148
+#define ADI_REG_TACH_100	0x014c
+#define ADI_REG_TACH_25_TOL	0x0150
+#define ADI_REG_TACH_50_TOL	0x0154
+#define ADI_REG_TACH_75_TOL	0x0158
+#define ADI_REG_TACH_100_TOL	0x015c
 
 #define ADI_REG_IRQ_MASK	0x0040
 #define ADI_REG_IRQ_PENDING	0x0044
@@ -328,6 +336,7 @@ static int axi_fan_control_init(struct axi_fan_control_data *ctl,
 				const struct device_node *np)
 {
 	int ret;
+	u32 tacho_val;
 
 	/* get fan pulses per revolution */
 	ret = of_property_read_u32(np, "pulses-per-revolution", &ctl->ppr);
@@ -337,6 +346,31 @@ static int axi_fan_control_init(struct axi_fan_control_data *ctl,
 	/* 1, 2 and 4 are the typical and accepted values */
 	if (ctl->ppr != 1 && ctl->ppr != 2 && ctl->ppr != 4)
 		return -EINVAL;
+
+	if (!of_property_read_u32(np, "adi,tacho-25-us", &tacho_val)) {
+		tacho_val = DIV_ROUND_CLOSEST_ULL((u64)tacho_val * ctl->clk_rate, 1000000);
+		axi_iowrite(tacho_val, ADI_REG_TACH_25, ctl);
+		axi_iowrite(DIV_ROUND_CLOSEST(tacho_val * 25, 100), ADI_REG_TACH_25_TOL, ctl);
+	}
+
+	if (!of_property_read_u32(np, "adi,tacho-50-us", &tacho_val)) {
+		tacho_val = DIV_ROUND_CLOSEST_ULL((u64)tacho_val * ctl->clk_rate, 1000000);
+		axi_iowrite(tacho_val, ADI_REG_TACH_50, ctl);
+		axi_iowrite(DIV_ROUND_CLOSEST(tacho_val * 25, 100), ADI_REG_TACH_50_TOL, ctl);
+	}
+
+	if (!of_property_read_u32(np, "adi,tacho-75-us", &tacho_val)) {
+		tacho_val = DIV_ROUND_CLOSEST_ULL((u64)tacho_val * ctl->clk_rate, 1000000);
+		axi_iowrite(tacho_val, ADI_REG_TACH_75, ctl);
+		axi_iowrite(DIV_ROUND_CLOSEST(tacho_val * 25, 100), ADI_REG_TACH_75_TOL, ctl);
+	}
+
+	if (!of_property_read_u32(np, "adi,tacho-100-us", &tacho_val)) {
+		tacho_val = DIV_ROUND_CLOSEST_ULL((u64)tacho_val * ctl->clk_rate, 1000000);
+		axi_iowrite(tacho_val, ADI_REG_TACH_100, ctl);
+		axi_iowrite(DIV_ROUND_CLOSEST(tacho_val * 25, 100), ADI_REG_TACH_100_TOL, ctl);
+	}
+
 	/*
 	 * Enable all IRQs
 	 */
-- 
2.32.0

[RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Nuno Sá]

Add the bindings for the tacho signal evaluation parameters which depend
on the FAN being used.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../bindings/hwmon/adi,axi-fan-control.yaml          | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
index 6747b870f297..0481eb34d9f1 100644
--- a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
+++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
@@ -37,6 +37,18 @@ properties:
     $ref: /schemas/types.yaml#/definitions/uint32
     enum: [1, 2, 4]
 
+  adi,tacho-25-us:
+    description: Expected tacho signal when the PWM is at 25%.
+
+  adi,tacho-50-us:
+    description: Expected tacho signal when the PWM is at 50%.
+
+  adi,tacho-75-us:
+    description: Expected tacho signal when the PWM is at 75%.
+
+  adi,tacho-100-us:
+    description: Expected tacho signal when the PWM is at 100%.
+
 required:
   - compatible
   - reg
-- 
2.32.0

[RFC PATCH 4/6] hwmon: axi-fan-control: handle irqs in natural order

[Nuno Sá]

The core will now start out of reset at boot as soon as clocking is
available. Hence, by the time we unmask the interrupts we already might
have some of them set. Thus, it's important to handle them in the
natural order the core generates them. Otherwise, we could process
'ADI_IRQ_SRC_PWM_CHANGED' before 'ADI_IRQ_SRC_TEMP_INCREASE' and
erroneously set 'update_tacho_params' to true.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/hwmon/axi-fan-control.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
index aa5a922f684d..d4b62d54375b 100644
--- a/drivers/hwmon/axi-fan-control.c
+++ b/drivers/hwmon/axi-fan-control.c
@@ -291,18 +291,9 @@ static irqreturn_t axi_fan_control_irq_handler(int irq, void *data)
 	u32 irq_pending = axi_ioread(ADI_REG_IRQ_PENDING, ctl);
 	u32 clear_mask;
 
-	if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) {
-		if (ctl->update_tacho_params) {
-			u32 new_tach = axi_ioread(ADI_REG_TACH_MEASUR, ctl);
-
-			/* get 25% tolerance */
-			u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, 100);
-			/* set new tacho parameters */
-			axi_iowrite(new_tach, ADI_REG_TACH_PERIOD, ctl);
-			axi_iowrite(tach_tol, ADI_REG_TACH_TOLERANCE, ctl);
-			ctl->update_tacho_params = 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_PWM_CHANGED) {
 		/*
@@ -318,9 +309,18 @@ static irqreturn_t axi_fan_control_irq_handler(int irq, void *data)
 		}
 	}
 
-	if (irq_pending & ADI_IRQ_SRC_TEMP_INCREASE)
-		/* hardware requested a new pwm */
-		ctl->hw_pwm_req = true;
+	if (irq_pending & ADI_IRQ_SRC_NEW_MEASUR) {
+		if (ctl->update_tacho_params) {
+			u32 new_tach = axi_ioread(ADI_REG_TACH_MEASUR, ctl);
+			/* get 25% tolerance */
+			u32 tach_tol = DIV_ROUND_CLOSEST(new_tach * 25, 100);
+
+			/* set new tacho parameters */
+			axi_iowrite(new_tach, ADI_REG_TACH_PERIOD, ctl);
+			axi_iowrite(tach_tol, ADI_REG_TACH_TOLERANCE, ctl);
+			ctl->update_tacho_params = false;
+		}
+	}
 
 	if (irq_pending & ADI_IRQ_SRC_TACH_ERR)
 		ctl->fan_fault = 1;
-- 
2.32.0

[RFC PATCH 5/6] hwmon: axi-fan-control: clear the fan fault irq at startup

[Nuno Sá]

The core might pull itself out of reset automatically which means it can
run with invalid tacho evaluation parameters for some time. Thus, it will
trigger a FAN FAULT interrupt as soon as we unmask it. Some userland apps
might be sensitive to this and act drastically. Hence, we will clear it
here and if there's something really wrong with the FAN or the evaluation
parameters, we'll get that interrupt again...

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/hwmon/axi-fan-control.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
index d4b62d54375b..59c9babb3850 100644
--- a/drivers/hwmon/axi-fan-control.c
+++ b/drivers/hwmon/axi-fan-control.c
@@ -379,6 +379,15 @@ static int axi_fan_control_init(struct axi_fan_control_data *ctl,
 		      ADI_IRQ_SRC_PWM_CHANGED | ADI_IRQ_SRC_TEMP_INCREASE),
 		    ADI_REG_IRQ_MASK, ctl);
 
+	/*
+	 * The core might pull itself out of reset automatically which means it can run with
+	 * invalid tacho evaluation parameters for some time. Thus, it will trigger a FAN
+	 * FAULT interrupt as soon as we unmask it(and some userland apps might be sensitive to
+	 * this). Hence, we will clear it here and if there's something really wrong with the
+	 * FAN or the evaluation parameters, we'll get that interrupt again...
+	 */
+	axi_iowrite(ADI_IRQ_SRC_TACH_ERR, ADI_REG_IRQ_PENDING, ctl);
+
 	/* bring the device out of reset */
 	axi_iowrite(0x01, ADI_REG_RSTN, ctl);
 
-- 
2.32.0

[RFC PATCH 6/6] hwmon: axi-fan-control: support temperature vs pwm points

[Nuno Sá]

The HW has some predefined points where it will associate a PWM value.
However some users might want to better set these points to their
usecases. This patch exposes these points as pwm auto_points:

 * pwm1_auto_point1_temp: temperature threshold below which PWM should be
   0%;
 * pwm1_auto_point2_temp: temperature threshold above which PWM should be
   25%;
 * pwm1_auto_point3_temp: temperature threshold below which PWM should be
   25%;
 * pwm1_auto_point4_temp: temperature threshold above which PWM should be
   50%;
 * pwm1_auto_point5_temp: temperature threshold below which PWM should be
   50%;
 * pwm1_auto_point6_temp: temperature threshold above which PWM should be
   75%;
 * pwm1_auto_point7_temp: temperature threshold below which PWM should be
   75%;
 * pwm1_auto_point8_temp: temperature threshold above which PWM should be
   100%;

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/hwmon/axi-fan-control.c | 74 ++++++++++++++++++++++++++++++++-
 1 file changed, 73 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/axi-fan-control.c b/drivers/hwmon/axi-fan-control.c
index 59c9babb3850..80b216448f47 100644
--- a/drivers/hwmon/axi-fan-control.c
+++ b/drivers/hwmon/axi-fan-control.c
@@ -8,6 +8,7 @@
 #include <linux/clk.h>
 #include <linux/fpga/adi-axi-common.h>
 #include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
@@ -31,6 +32,14 @@
 #define ADI_REG_TACH_50_TOL	0x0154
 #define ADI_REG_TACH_75_TOL	0x0158
 #define ADI_REG_TACH_100_TOL	0x015c
+#define ADI_REG_TEMP_00_H	0x0100
+#define ADI_REG_TEMP_25_L	0x0104
+#define ADI_REG_TEMP_25_H	0x0108
+#define ADI_REG_TEMP_50_L	0x010c
+#define ADI_REG_TEMP_50_H	0x0110
+#define ADI_REG_TEMP_75_L	0x0114
+#define ADI_REG_TEMP_75_H	0x0118
+#define ADI_REG_TEMP_100_L	0x011c
 
 #define ADI_REG_IRQ_MASK	0x0040
 #define ADI_REG_IRQ_PENDING	0x0044
@@ -70,6 +79,39 @@ static inline u32 axi_ioread(const u32 reg,
 	return ioread32(ctl->base + reg);
 }
 
+/*
+ * The core calculates the temperature as:
+ *	T = /raw * 509.3140064 / 65535) - 280.2308787
+ */
+static ssize_t axi_fan_control_show(struct device *dev, struct device_attribute *da, char *buf)
+{
+	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	u32 temp = axi_ioread(attr->index, ctl);
+
+	temp = DIV_ROUND_CLOSEST_ULL(temp * 509314ULL, 65535) - 280230;
+
+	return sprintf(buf, "%u\n", temp);
+}
+
+static ssize_t axi_fan_control_store(struct device *dev, struct device_attribute *da,
+				     const char *buf, size_t count)
+{
+	struct axi_fan_control_data *ctl = dev_get_drvdata(dev);
+	struct sensor_device_attribute *attr = to_sensor_dev_attr(da);
+	u32 temp;
+	int ret;
+
+	ret = kstrtou32(buf, 10, &temp);
+	if (ret)
+		return ret;
+
+	temp = DIV_ROUND_CLOSEST_ULL((temp + 280230) * 65535ULL, 509314);
+	axi_iowrite(temp, attr->index, ctl);
+
+	return count;
+}
+
 static long axi_fan_control_get_pwm_duty(const struct axi_fan_control_data *ctl)
 {
 	u32 pwm_width = axi_ioread(ADI_REG_PWM_WIDTH, ctl);
@@ -418,6 +460,36 @@ static const struct hwmon_chip_info axi_chip_info = {
 	.info = axi_fan_control_info,
 };
 
+/* temperature threshold below which PWM should be 0% */
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_temp, axi_fan_control, ADI_REG_TEMP_00_H);
+/* temperature threshold above which PWM should be 25% */
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point2_temp, axi_fan_control, ADI_REG_TEMP_25_L);
+/* temperature threshold below which PWM should be 25% */
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point3_temp, axi_fan_control, ADI_REG_TEMP_25_H);
+/* temperature threshold above which PWM should be 50% */
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point4_temp, axi_fan_control, ADI_REG_TEMP_50_L);
+/* temperature threshold below which PWM should be 50% */
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point5_temp, axi_fan_control, ADI_REG_TEMP_50_H);
+/* temperature threshold above which PWM should be 75% */
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point6_temp, axi_fan_control, ADI_REG_TEMP_75_L);
+/* temperature threshold below which PWM should be 75% */
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point7_temp, axi_fan_control, ADI_REG_TEMP_75_H);
+/* temperature threshold above which PWM should be 100% */
+static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point8_temp, axi_fan_control, ADI_REG_TEMP_100_L);
+
+static struct attribute *axi_fan_control_attrs[] = {
+	&sensor_dev_attr_pwm1_auto_point1_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point2_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point3_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point4_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point5_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point6_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point7_temp.dev_attr.attr,
+	&sensor_dev_attr_pwm1_auto_point8_temp.dev_attr.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(axi_fan_control);
+
 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[] = {
@@ -502,7 +574,7 @@ static int axi_fan_control_probe(struct platform_device *pdev)
 							 name,
 							 ctl,
 							 &axi_chip_info,
-							 NULL);
+							 axi_fan_control_groups);
 
 	return PTR_ERR_OR_ZERO(ctl->hdev);
 }
-- 
2.32.0

Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Rob Herring]

On Thu, Jul 08, 2021 at 02:01:08PM +0200, Nuno Sá wrote:
> Add the bindings for the tacho signal evaluation parameters which depend
> on the FAN being used.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  .../bindings/hwmon/adi,axi-fan-control.yaml          | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
> index 6747b870f297..0481eb34d9f1 100644
> --- a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
> +++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-control.yaml
> @@ -37,6 +37,18 @@ properties:
>      $ref: /schemas/types.yaml#/definitions/uint32
>      enum: [1, 2, 4]
>  
> +  adi,tacho-25-us:
> +    description: Expected tacho signal when the PWM is at 25%.
> +
> +  adi,tacho-50-us:
> +    description: Expected tacho signal when the PWM is at 50%.
> +
> +  adi,tacho-75-us:
> +    description: Expected tacho signal when the PWM is at 75%.
> +
> +  adi,tacho-100-us:
> +    description: Expected tacho signal when the PWM is at 100%.

This looks like it should be common. But having PWM percents in the 
property names doesn't scale. This is also a property of the fan, not 
the fan controller.

There's only so many ways a fan can be controlled and I'm going to keep 
telling folks to make a common fan binding. There's some start to it, 
but it needs some work.

Rob

RE: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Sa, Nuno]

> From: Rob Herring <robh@kernel.org>
> Sent: Monday, July 12, 2021 7:27 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> Guenter Roeck <linux@roeck-us.net>; Jean Delvare
> <jdelvare@suse.com>
> Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho
> properties
> 
> [External]
> 
> On Thu, Jul 08, 2021 at 02:01:08PM +0200, Nuno Sá wrote:
> > Add the bindings for the tacho signal evaluation parameters which
> depend
> > on the FAN being used.
> >
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  .../bindings/hwmon/adi,axi-fan-control.yaml          | 12
> ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> control.yaml b/Documentation/devicetree/bindings/hwmon/adi,axi-
> fan-control.yaml
> > index 6747b870f297..0481eb34d9f1 100644
> > --- a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> control.yaml
> > +++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> control.yaml
> > @@ -37,6 +37,18 @@ properties:
> >      $ref: /schemas/types.yaml#/definitions/uint32
> >      enum: [1, 2, 4]
> >
> > +  adi,tacho-25-us:
> > +    description: Expected tacho signal when the PWM is at 25%.
> > +
> > +  adi,tacho-50-us:
> > +    description: Expected tacho signal when the PWM is at 50%.
> > +
> > +  adi,tacho-75-us:
> > +    description: Expected tacho signal when the PWM is at 75%.
> > +
> > +  adi,tacho-100-us:
> > +    description: Expected tacho signal when the PWM is at 100%.
> 
> This looks like it should be common. But having PWM percents in the
> property names doesn't scale. This is also a property of the fan, not
> the fan controller.

Yes, I see that these parameters are definitely a property of the attached
fan but the evaluation of these timings are very specific to this controller
(I think). The way it works is that the HW can fully operate without any
runtime SW configuration. In this case, it will use the values in these
registers to evaluate the tacho signal coming from the FAN. And the HW
really uses the evaluation points like this (0, 25%, 50% and 100%). It has
some predefined values that work for the FAN that was used to develop
the IP but naturally the evaluation will fail as soon as other FAN is attached
(resulting in fan fault interrupts). And yes, writing these parameters is 
already SW configuration but what I mean with "runtime" is after probe :). 

So, I honestly do not know how we could name this better... Maybe a
'tacho-eval-points-us' array? The question would be the min/max
elements? Do you have any suggestion for a more generic property?

> There's only so many ways a fan can be controlled and I'm going to
> keep
> telling folks to make a common fan binding. There's some start to it,
> but it needs some work.

You mean the pwm-fan.txt? I gave a look to the driver and I don't think
it fully fits this controller. I'm ok in sending an fan.yaml binding if you
prefer it but I'm just not sure what we would need there... Maybe
pulses-per-revolution and the above property
(whatever the decided name) would be a starting point? 

- Nuno Sá

Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Guenter Roeck]

On Thu, Jul 15, 2021 at 10:26:05AM +0000, Sa, Nuno wrote:
> > From: Rob Herring <robh@kernel.org>
> > Sent: Monday, July 12, 2021 7:27 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> > Guenter Roeck <linux@roeck-us.net>; Jean Delvare
> > <jdelvare@suse.com>
> > Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho
> > properties
> > 
> > [External]
> > 
> > On Thu, Jul 08, 2021 at 02:01:08PM +0200, Nuno Sá wrote:
> > > Add the bindings for the tacho signal evaluation parameters which
> > depend
> > > on the FAN being used.
> > >
> > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > ---
> > >  .../bindings/hwmon/adi,axi-fan-control.yaml          | 12
> > ++++++++++++
> > >  1 file changed, 12 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> > control.yaml b/Documentation/devicetree/bindings/hwmon/adi,axi-
> > fan-control.yaml
> > > index 6747b870f297..0481eb34d9f1 100644
> > > --- a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> > control.yaml
> > > +++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> > control.yaml
> > > @@ -37,6 +37,18 @@ properties:
> > >      $ref: /schemas/types.yaml#/definitions/uint32
> > >      enum: [1, 2, 4]
> > >
> > > +  adi,tacho-25-us:
> > > +    description: Expected tacho signal when the PWM is at 25%.
> > > +
> > > +  adi,tacho-50-us:
> > > +    description: Expected tacho signal when the PWM is at 50%.
> > > +
> > > +  adi,tacho-75-us:
> > > +    description: Expected tacho signal when the PWM is at 75%.
> > > +
> > > +  adi,tacho-100-us:
> > > +    description: Expected tacho signal when the PWM is at 100%.
> > 
> > This looks like it should be common. But having PWM percents in the
> > property names doesn't scale. This is also a property of the fan, not
> > the fan controller.
> 
> Yes, I see that these parameters are definitely a property of the attached
> fan but the evaluation of these timings are very specific to this controller
> (I think). The way it works is that the HW can fully operate without any
> runtime SW configuration. In this case, it will use the values in these
> registers to evaluate the tacho signal coming from the FAN. And the HW
> really uses the evaluation points like this (0, 25%, 50% and 100%). It has
> some predefined values that work for the FAN that was used to develop
> the IP but naturally the evaluation will fail as soon as other FAN is attached
> (resulting in fan fault interrupts). And yes, writing these parameters is 
> already SW configuration but what I mean with "runtime" is after probe :). 
> 

Are you sure you can ever get this stable ? Each fan has its own properties
and tolerances. If you replace a fan in a given system, you might get
different RPM numbers. The RPM will differ widely from system to system
and from fan to fan. Anything that assumes a specific RPM in devicetree
data seems to be quite vulnerable to failures. I have experienced that
recently with a different chip which also tries to correlate RPM and PWM
and fails quite miserably.

In my experience, anything other than minimum fan speed is really a recipe
for instability and sporadic false failures. Even setting a minimum fan speed
is tricky because it depends a lot on the fan.

> So, I honestly do not know how we could name this better... Maybe a
> 'tacho-eval-points-us' array? The question would be the min/max
> elements? Do you have any suggestion for a more generic property?
> 
I am guessing that the "us" refers to the time between pulses from the
fan. I think this is a bad value to start with - anything fan speed
related should really be expressed in RPM, not in time between pulses.

Overall I don't think this should be handled as generic set of properties.
Whatever we come up with as standard set of pwm or fan related properties
should not be an expected correlation between pwm and rpm. Assuming such
a property is needed here (after all, the controller is what it is),
maybe a set of tuples makes sense, such as

	adi,pwm-rpm-map = <
		25, 250,
		50, 500,
		75, 750,
		100, 1000
	>;

though I think that each of those should also include the tolerance
instead of just assuming that a 25% tolerance (as implemented in patch
2/6) would work for all fans.

Guenter

> > There's only so many ways a fan can be controlled and I'm going to
> > keep
> > telling folks to make a common fan binding. There's some start to it,
> > but it needs some work.
> 
> You mean the pwm-fan.txt? I gave a look to the driver and I don't think
> it fully fits this controller. I'm ok in sending an fan.yaml binding if you
> prefer it but I'm just not sure what we would need there... Maybe
> pulses-per-revolution and the above property
> (whatever the decided name) would be a starting point? 
> 
> - Nuno Sá

RE: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Sa, Nuno]

> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> Sent: Thursday, July 15, 2021 10:40 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Rob Herring <robh@kernel.org>; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; Jean Delvare <jdelvare@suse.com>
> Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho
> properties
> 
> On Thu, Jul 15, 2021 at 10:26:05AM +0000, Sa, Nuno wrote:
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Monday, July 12, 2021 7:27 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org;
> > > Guenter Roeck <linux@roeck-us.net>; Jean Delvare
> > > <jdelvare@suse.com>
> > > Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add
> tacho
> > > properties
> > >
> > > [External]
> > >
> > > On Thu, Jul 08, 2021 at 02:01:08PM +0200, Nuno Sá wrote:
> > > > Add the bindings for the tacho signal evaluation parameters
> which
> > > depend
> > > > on the FAN being used.
> > > >
> > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > > > ---
> > > >  .../bindings/hwmon/adi,axi-fan-control.yaml          | 12
> > > ++++++++++++
> > > >  1 file changed, 12 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/hwmon/adi,axi-
> fan-
> > > control.yaml
> b/Documentation/devicetree/bindings/hwmon/adi,axi-
> > > fan-control.yaml
> > > > index 6747b870f297..0481eb34d9f1 100644
> > > > --- a/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> > > control.yaml
> > > > +++ b/Documentation/devicetree/bindings/hwmon/adi,axi-fan-
> > > control.yaml
> > > > @@ -37,6 +37,18 @@ properties:
> > > >      $ref: /schemas/types.yaml#/definitions/uint32
> > > >      enum: [1, 2, 4]
> > > >
> > > > +  adi,tacho-25-us:
> > > > +    description: Expected tacho signal when the PWM is at 25%.
> > > > +
> > > > +  adi,tacho-50-us:
> > > > +    description: Expected tacho signal when the PWM is at 50%.
> > > > +
> > > > +  adi,tacho-75-us:
> > > > +    description: Expected tacho signal when the PWM is at 75%.
> > > > +
> > > > +  adi,tacho-100-us:
> > > > +    description: Expected tacho signal when the PWM is at 100%.
> > >
> > > This looks like it should be common. But having PWM percents in
> the
> > > property names doesn't scale. This is also a property of the fan, not
> > > the fan controller.
> >
> > Yes, I see that these parameters are definitely a property of the
> attached
> > fan but the evaluation of these timings are very specific to this
> controller
> > (I think). The way it works is that the HW can fully operate without
> any
> > runtime SW configuration. In this case, it will use the values in these
> > registers to evaluate the tacho signal coming from the FAN. And the
> HW
> > really uses the evaluation points like this (0, 25%, 50% and 100%). It
> has
> > some predefined values that work for the FAN that was used to
> develop
> > the IP but naturally the evaluation will fail as soon as other FAN is
> attached
> > (resulting in fan fault interrupts). And yes, writing these parameters
> is
> > already SW configuration but what I mean with "runtime" is after
> probe :).
> >
> 
> Are you sure you can ever get this stable ? Each fan has its own
> properties
> and tolerances. If you replace a fan in a given system, you might get
> different RPM numbers. The RPM will differ widely from system to
> system
> and from fan to fan. Anything that assumes a specific RPM in
> devicetree
> data seems to be quite vulnerable to failures. I have experienced that
> recently with a different chip which also tries to correlate RPM and
> PWM
> and fails quite miserably.
> 
> In my experience, anything other than minimum fan speed is really a
> recipe
> for instability and sporadic false failures. Even setting a minimum fan
> speed
> is tricky because it depends a lot on the fan.

I see what you mean. So, I had to go through this process when testing
this changes because the fan I'm using is different from the default one
used to develop and stablish the default values in the IP core. The core
provides you with a register which contains the tacho measurements in
clock cycles. You can read that for all the PWM points of interest
(with devmem2 for example) and make your own "calibration". I assume
that people have to go through this process before putting some values
in the devicetree. I'm aware this is not the neatest process but I guess it's
acceptable...

> > So, I honestly do not know how we could name this better... Maybe
> a
> > 'tacho-eval-points-us' array? The question would be the min/max
> > elements? Do you have any suggestion for a more generic property?
> >
> I am guessing that the "us" refers to the time between pulses from the
> fan. I think this is a bad value to start with - anything fan speed
> related should really be expressed in RPM, not in time between
> pulses.
> 
> Overall I don't think this should be handled as generic set of
> properties.
> Whatever we come up with as standard set of pwm or fan related
> properties
> should not be an expected correlation between pwm and rpm.
> Assuming such
> a property is needed here (after all, the controller is what it is),
> maybe a set of tuples makes sense, such as
> 
> 	adi,pwm-rpm-map = <
> 		25, 250,
> 		50, 500,
> 		75, 750,
> 		100, 1000
> 	>;
> 
> though I think that each of those should also include the tolerance
> instead of just assuming that a 25% tolerance (as implemented in patch
> 2/6) would work for all fans.

Yes, this makes sense thanks. As the HW default tolerance is 25% I was 
somehow attached to that. Since all these values are correlated it makes
complete sense to also give the tolerance here as it makes things more
flexible. The map is also a good tip, I just have to see if there is a nice
way to specify that the pwm column is constant...

Thanks!
- Nuno Sá

Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Guenter Roeck]

On 7/16/21 12:44 AM, Sa, Nuno wrote:
[ ... ]
>>
>> Are you sure you can ever get this stable ? Each fan has its own
>> properties
>> and tolerances. If you replace a fan in a given system, you might get
>> different RPM numbers. The RPM will differ widely from system to
>> system
>> and from fan to fan. Anything that assumes a specific RPM in
>> devicetree
>> data seems to be quite vulnerable to failures. I have experienced that
>> recently with a different chip which also tries to correlate RPM and
>> PWM
>> and fails quite miserably.
>>
>> In my experience, anything other than minimum fan speed is really a
>> recipe
>> for instability and sporadic false failures. Even setting a minimum fan
>> speed
>> is tricky because it depends a lot on the fan.
> 
> I see what you mean. So, I had to go through this process when testing
> this changes because the fan I'm using is different from the default one
> used to develop and stablish the default values in the IP core. The core

Exactly my point.

> provides you with a register which contains the tacho measurements in
> clock cycles. You can read that for all the PWM points of interest
> (with devmem2 for example) and make your own "calibration". I assume
> that people have to go through this process before putting some values
> in the devicetree. I'm aware this is not the neatest process but I guess it's
> acceptable...
> 

Do you really expect everyone using a system with this chip to go through
this process and update its devicetree configuration, and then repeat it
whenever a fan is changed ? Given how dynamic this is, I really wonder
if that information should be in devicetree in the first place.

Guenter

Re: [RFC PATCH 6/6] hwmon: axi-fan-control: support temperature vs pwm points

[Guenter Roeck]

On Thu, Jul 08, 2021 at 02:01:11PM +0200, Nuno Sá wrote:
> The HW has some predefined points where it will associate a PWM value.
> However some users might want to better set these points to their
> usecases. This patch exposes these points as pwm auto_points:
> 
>  * pwm1_auto_point1_temp: temperature threshold below which PWM should be
>    0%;
>  * pwm1_auto_point2_temp: temperature threshold above which PWM should be
>    25%;
>  * pwm1_auto_point3_temp: temperature threshold below which PWM should be
>    25%;
>  * pwm1_auto_point4_temp: temperature threshold above which PWM should be
>    50%;
>  * pwm1_auto_point5_temp: temperature threshold below which PWM should be
>    50%;
>  * pwm1_auto_point6_temp: temperature threshold above which PWM should be
>    75%;
>  * pwm1_auto_point7_temp: temperature threshold below which PWM should be
>    75%;
>  * pwm1_auto_point8_temp: temperature threshold above which PWM should be
>    100%;
> 

If I understand those correctly, half of those are really hysteresis points.
I think it would be better to express this with
	pwm1_auto_pointX_temp
	pwm1_auto_pointX_temp_hyst

where the hysteresis point is the temperature where the previous pwm value
is activated. In other words, change attribute names as follows:
    for 25%:
	pwm1_auto_point1_temp -> pwm1_auto_point1_temp_hyst
	pwm1_auto_point2_temp -> pwm1_auto_point1_temp
    for 50%:
	pwm1_auto_point3_temp -> pwm1_auto_point2_temp_hyst
	pwm1_auto_point4_temp -> pwm1_auto_point2_temp
    for 75%:
	pwm1_auto_point5_temp -> pwm1_auto_point3_temp_hyst
	pwm1_auto_point6_temp -> pwm1_auto_point3_temp
    for 100%:
	pwm1_auto_point7_temp -> pwm1_auto_point4_temp_hyst
	pwm1_auto_point8_temp -> pwm1_auto_point4_temp

Thanks,
Guenter

Re: [RFC PATCH 1/6] hwmon: axi-fan-control: make sure the clock is enabled

[Guenter Roeck]

On Thu, Jul 08, 2021 at 02:01:06PM +0200, Nuno Sá wrote:
> The core will only work if it's clock is enabled. This patch is a
> minor enhancement to make sure that's the case.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Can I apply this patch as well as patches 4/6 and 5/6 as-is, or
do they depend on patches 2/6 and 3/6 ?

Thanks,
Guenter

RE: [RFC PATCH 6/6] hwmon: axi-fan-control: support temperature vs pwm points

[Sa, Nuno]

> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> Sent: Saturday, July 17, 2021 7:23 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Jean Delvare <jdelvare@suse.com>
> Subject: Re: [RFC PATCH 6/6] hwmon: axi-fan-control: support
> temperature vs pwm points
> 
> [External]
> 
> On Thu, Jul 08, 2021 at 02:01:11PM +0200, Nuno Sá wrote:
> > The HW has some predefined points where it will associate a PWM
> value.
> > However some users might want to better set these points to their
> > usecases. This patch exposes these points as pwm auto_points:
> >
> >  * pwm1_auto_point1_temp: temperature threshold below which
> PWM should be
> >    0%;
> >  * pwm1_auto_point2_temp: temperature threshold above which
> PWM should be
> >    25%;
> >  * pwm1_auto_point3_temp: temperature threshold below which
> PWM should be
> >    25%;
> >  * pwm1_auto_point4_temp: temperature threshold above which
> PWM should be
> >    50%;
> >  * pwm1_auto_point5_temp: temperature threshold below which
> PWM should be
> >    50%;
> >  * pwm1_auto_point6_temp: temperature threshold above which
> PWM should be
> >    75%;
> >  * pwm1_auto_point7_temp: temperature threshold below which
> PWM should be
> >    75%;
> >  * pwm1_auto_point8_temp: temperature threshold above which
> PWM should be
> >    100%;
> >
> 
> If I understand those correctly, half of those are really hysteresis
> points.
> I think it would be better to express this with
> 	pwm1_auto_pointX_temp
> 	pwm1_auto_pointX_temp_hyst
> 
> where the hysteresis point is the temperature where the previous
> pwm value
> is activated. In other words, change attribute names as follows:
>     for 25%:
> 	pwm1_auto_point1_temp -> pwm1_auto_point1_temp_hyst
> 	pwm1_auto_point2_temp -> pwm1_auto_point1_temp
>     for 50%:
> 	pwm1_auto_point3_temp -> pwm1_auto_point2_temp_hyst
> 	pwm1_auto_point4_temp -> pwm1_auto_point2_temp
>     for 75%:
> 	pwm1_auto_point5_temp -> pwm1_auto_point3_temp_hyst
> 	pwm1_auto_point6_temp -> pwm1_auto_point3_temp
>     for 100%:
> 	pwm1_auto_point7_temp -> pwm1_auto_point4_temp_hyst
> 	pwm1_auto_point8_temp -> pwm1_auto_point4_temp
> 

Agree, it makes sense.

- Nuno Sá

RE: [RFC PATCH 1/6] hwmon: axi-fan-control: make sure the clock is enabled

[Sa, Nuno]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> Sent: Saturday, July 17, 2021 7:25 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; Rob
> Herring <robh+dt@kernel.org>; Jean Delvare <jdelvare@suse.com>
> Subject: Re: [RFC PATCH 1/6] hwmon: axi-fan-control: make sure the
> clock is enabled
> 
> 
> On Thu, Jul 08, 2021 at 02:01:06PM +0200, Nuno Sá wrote:
> > The core will only work if it's clock is enabled. This patch is a
> > minor enhancement to make sure that's the case.
> >
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> 
> Can I apply this patch as well as patches 4/6 and 5/6 as-is, or
> do they depend on patches 2/6 and 3/6 ?
> 
> Thanks,
> Guenter

I think patch 5/6 only makes sense with patch 2/6. If we do not set
the tacho evaluation parameters, clearing the fan fault irq has no effect...

- Nuno Sá

RE: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Sa, Nuno]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> Sent: Friday, July 16, 2021 5:04 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Rob Herring <robh@kernel.org>; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; Jean Delvare <jdelvare@suse.com>
> Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho
> properties
> 
> [External]
> 
> On 7/16/21 12:44 AM, Sa, Nuno wrote:
> [ ... ]
> >>
> >> Are you sure you can ever get this stable ? Each fan has its own
> >> properties
> >> and tolerances. If you replace a fan in a given system, you might get
> >> different RPM numbers. The RPM will differ widely from system to
> >> system
> >> and from fan to fan. Anything that assumes a specific RPM in
> >> devicetree
> >> data seems to be quite vulnerable to failures. I have experienced
> that
> >> recently with a different chip which also tries to correlate RPM and
> >> PWM
> >> and fails quite miserably.
> >>
> >> In my experience, anything other than minimum fan speed is really
> a
> >> recipe
> >> for instability and sporadic false failures. Even setting a minimum
> fan
> >> speed
> >> is tricky because it depends a lot on the fan.
> >
> > I see what you mean. So, I had to go through this process when
> testing
> > this changes because the fan I'm using is different from the default
> one
> > used to develop and stablish the default values in the IP core. The
> core
> 
> Exactly my point.
> 
> > provides you with a register which contains the tacho measurements
> in
> > clock cycles. You can read that for all the PWM points of interest
> > (with devmem2 for example) and make your own "calibration". I
> assume
> > that people have to go through this process before putting some
> values
> > in the devicetree. I'm aware this is not the neatest process but I
> guess it's
> > acceptable...
> >
> 
> Do you really expect everyone using a system with this chip to go
> through
> this process and update its devicetree configuration, and then repeat it
> whenever a fan is changed ? Given how dynamic this is, I really wonder
> if that information should be in devicetree in the first place.
> 

My naive assumption was that we would only do this work at evaluation
time. After that and after we settled with a fan for some system, I expected
that changing to a different fan is not that likely. From your inputs, I guess
this is not really the case which makes this process more cumbersome (as it
also implies recompiling the devicetree for your system).

However, even if we export these as runtime parameters, services/daemons
will also have an hard time doing this "calibration" in a dynamic way. The reason
is because the way the controller works is that it only accepts a new PWM
request if it is an higher value than whatever the HW has at that moment. Thus,
going through the calibration points might be very cumbersome. I can see some
ways of handling this though but not very neat...

Since this is a FPGA core, we might have some flexibility here. Something that
came to my mind would be to have a calibration mode in the HW that would
allow us to freely control the PWM values. In that way we could go freely over
the calibration points. I guess, for safety reasons, this calibration mode would
expire after some reasonable time (that give us enough time for doing the whole
thing). The best place for doing the calibration, I guess it would be directly in the
driver since we do receive the interrupts about new tacho measurements making
things easier to sync and handle. However, given the time that takes for a new
PWM to settle + new tacho measurements, it would not be very acceptable to do this 
during probe which is definitely also not ideal (we could defer this to a worker/timer).

I'm not sure if the above makes much sense to you and it also depends on the HW
guys being on board with this mechanism.

- Nuno Sá

Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Guenter Roeck]

On Mon, Jul 19, 2021 at 07:46:41AM +0000, Sa, Nuno wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> > Roeck
> > Sent: Friday, July 16, 2021 5:04 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: Rob Herring <robh@kernel.org>; linux-hwmon@vger.kernel.org;
> > devicetree@vger.kernel.org; Jean Delvare <jdelvare@suse.com>
> > Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho
> > properties
> > 
> > [External]
> > 
> > On 7/16/21 12:44 AM, Sa, Nuno wrote:
> > [ ... ]
> > >>
> > >> Are you sure you can ever get this stable ? Each fan has its own
> > >> properties
> > >> and tolerances. If you replace a fan in a given system, you might get
> > >> different RPM numbers. The RPM will differ widely from system to
> > >> system
> > >> and from fan to fan. Anything that assumes a specific RPM in
> > >> devicetree
> > >> data seems to be quite vulnerable to failures. I have experienced
> > that
> > >> recently with a different chip which also tries to correlate RPM and
> > >> PWM
> > >> and fails quite miserably.
> > >>
> > >> In my experience, anything other than minimum fan speed is really
> > a
> > >> recipe
> > >> for instability and sporadic false failures. Even setting a minimum
> > fan
> > >> speed
> > >> is tricky because it depends a lot on the fan.
> > >
> > > I see what you mean. So, I had to go through this process when
> > testing
> > > this changes because the fan I'm using is different from the default
> > one
> > > used to develop and stablish the default values in the IP core. The
> > core
> > 
> > Exactly my point.
> > 
> > > provides you with a register which contains the tacho measurements
> > in
> > > clock cycles. You can read that for all the PWM points of interest
> > > (with devmem2 for example) and make your own "calibration". I
> > assume
> > > that people have to go through this process before putting some
> > values
> > > in the devicetree. I'm aware this is not the neatest process but I
> > guess it's
> > > acceptable...
> > >
> > 
> > Do you really expect everyone using a system with this chip to go
> > through
> > this process and update its devicetree configuration, and then repeat it
> > whenever a fan is changed ? Given how dynamic this is, I really wonder
> > if that information should be in devicetree in the first place.
> > 
> 
> My naive assumption was that we would only do this work at evaluation
> time. After that and after we settled with a fan for some system, I expected
> that changing to a different fan is not that likely. From your inputs, I guess
> this is not really the case which makes this process more cumbersome (as it
> also implies recompiling the devicetree for your system).
> 
> However, even if we export these as runtime parameters, services/daemons
> will also have an hard time doing this "calibration" in a dynamic way. The reason
> is because the way the controller works is that it only accepts a new PWM
> request if it is an higher value than whatever the HW has at that moment. Thus,
> going through the calibration points might be very cumbersome. I can see some
> ways of handling this though but not very neat...
> 
> Since this is a FPGA core, we might have some flexibility here. Something that
> came to my mind would be to have a calibration mode in the HW that would
> allow us to freely control the PWM values. In that way we could go freely over
> the calibration points. I guess, for safety reasons, this calibration mode would
> expire after some reasonable time (that give us enough time for doing the whole
> thing). The best place for doing the calibration, I guess it would be directly in the
> driver since we do receive the interrupts about new tacho measurements making
> things easier to sync and handle. However, given the time that takes for a new
> PWM to settle + new tacho measurements, it would not be very acceptable to do this 
> during probe which is definitely also not ideal (we could defer this to a worker/timer).
> 
> I'm not sure if the above makes much sense to you and it also depends on the HW
> guys being on board with this mechanism.
> 

I don't really know what to say or recommend here. Personally I think any
attempt to tie PWM values to RPM are doomed to fail. Here are a couple of
examples:

Take your test system and move the fan to a restricted place (eg close to a
wall). You'll see the fan RPM change, potentially significantly. Put it into
some place with airflow towards or away from the system (eg blow air into
the system from another place, which may happen if the system is installed
in a lab), and again you'll see fan speed changes. Open the chassis, and
the fan speed will change. I have seen fan speeds vary by up to 50% when
changing airflow.

That doesn't even take into account that replacing a fan even with a similar
model (eg after a fan failed) will likely result in potentially significant
rpm changes.

Ultimately, anything that does more than determine if a fan is still running
is potentially unstable.

Having said all that, it is really your call to decide how you want to detect
fan failures. 

Thanks,
Guenter

RE: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Sa, Nuno]

> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> Sent: Wednesday, July 21, 2021 5:00 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: Rob Herring <robh@kernel.org>; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; Jean Delvare <jdelvare@suse.com>
> Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho
> properties
> 
> 
> On Mon, Jul 19, 2021 at 07:46:41AM +0000, Sa, Nuno wrote:
> >
> >
> > > -----Original Message-----
> > > From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> > > Roeck
> > > Sent: Friday, July 16, 2021 5:04 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: Rob Herring <robh@kernel.org>; linux-
> hwmon@vger.kernel.org;
> > > devicetree@vger.kernel.org; Jean Delvare <jdelvare@suse.com>
> > > Subject: Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add
> tacho
> > > properties
> > >
> > > [External]
> > >
> > > On 7/16/21 12:44 AM, Sa, Nuno wrote:
> > > [ ... ]
> > > >>
> > > >> Are you sure you can ever get this stable ? Each fan has its own
> > > >> properties
> > > >> and tolerances. If you replace a fan in a given system, you might
> get
> > > >> different RPM numbers. The RPM will differ widely from system
> to
> > > >> system
> > > >> and from fan to fan. Anything that assumes a specific RPM in
> > > >> devicetree
> > > >> data seems to be quite vulnerable to failures. I have
> experienced
> > > that
> > > >> recently with a different chip which also tries to correlate RPM
> and
> > > >> PWM
> > > >> and fails quite miserably.
> > > >>
> > > >> In my experience, anything other than minimum fan speed is
> really
> > > a
> > > >> recipe
> > > >> for instability and sporadic false failures. Even setting a
> minimum
> > > fan
> > > >> speed
> > > >> is tricky because it depends a lot on the fan.
> > > >
> > > > I see what you mean. So, I had to go through this process when
> > > testing
> > > > this changes because the fan I'm using is different from the
> default
> > > one
> > > > used to develop and stablish the default values in the IP core.
> The
> > > core
> > >
> > > Exactly my point.
> > >
> > > > provides you with a register which contains the tacho
> measurements
> > > in
> > > > clock cycles. You can read that for all the PWM points of interest
> > > > (with devmem2 for example) and make your own "calibration". I
> > > assume
> > > > that people have to go through this process before putting some
> > > values
> > > > in the devicetree. I'm aware this is not the neatest process but I
> > > guess it's
> > > > acceptable...
> > > >
> > >
> > > Do you really expect everyone using a system with this chip to go
> > > through
> > > this process and update its devicetree configuration, and then
> repeat it
> > > whenever a fan is changed ? Given how dynamic this is, I really
> wonder
> > > if that information should be in devicetree in the first place.
> > >
> >
> > My naive assumption was that we would only do this work at
> evaluation
> > time. After that and after we settled with a fan for some system, I
> expected
> > that changing to a different fan is not that likely. From your inputs, I
> guess
> > this is not really the case which makes this process more
> cumbersome (as it
> > also implies recompiling the devicetree for your system).
> >
> > However, even if we export these as runtime parameters,
> services/daemons
> > will also have an hard time doing this "calibration" in a dynamic way.
> The reason
> > is because the way the controller works is that it only accepts a new
> PWM
> > request if it is an higher value than whatever the HW has at that
> moment. Thus,
> > going through the calibration points might be very cumbersome. I
> can see some
> > ways of handling this though but not very neat...
> >
> > Since this is a FPGA core, we might have some flexibility here.
> Something that
> > came to my mind would be to have a calibration mode in the HW that
> would
> > allow us to freely control the PWM values. In that way we could go
> freely over
> > the calibration points. I guess, for safety reasons, this calibration
> mode would
> > expire after some reasonable time (that give us enough time for
> doing the whole
> > thing). The best place for doing the calibration, I guess it would be
> directly in the
> > driver since we do receive the interrupts about new tacho
> measurements making
> > things easier to sync and handle. However, given the time that takes
> for a new
> > PWM to settle + new tacho measurements, it would not be very
> acceptable to do this
> > during probe which is definitely also not ideal (we could defer this to
> a worker/timer).
> >
> > I'm not sure if the above makes much sense to you and it also
> depends on the HW
> > guys being on board with this mechanism.
> >
> 
> I don't really know what to say or recommend here. Personally I think
> any
> attempt to tie PWM values to RPM are doomed to fail. Here are a
> couple of
> examples:
> 
> Take your test system and move the fan to a restricted place (eg close
> to a
> wall). You'll see the fan RPM change, potentially significantly. Put it into
> some place with airflow towards or away from the system (eg blow air
> into
> the system from another place, which may happen if the system is
> installed
> in a lab), and again you'll see fan speed changes. Open the chassis, and
> the fan speed will change. I have seen fan speeds vary by up to 50%
> when
> changing airflow.

Here we can at least control the tolerance for each PWM vs RPM point but
I can image this as a very painful process to get these values right and no
one will think in setting tolerances of 50%...

> That doesn't even take into account that replacing a fan even with a
> similar
> model (eg after a fan failed) will likely result in potentially significant
> rpm changes.
> 
> Ultimately, anything that does more than determine if a fan is still
> running
> is potentially unstable.

Yeah, I understand your points. The HW does the evaluation and of 
course it also looks for the presence of a signal... So, in your opinion,
not even setting a minimum fan speed is likely to be stable?

> Having said all that, it is really your call to decide how you want to
> detect
> fan failures.
> 

Well, my hands are also tied here. The core is supposed to work without
any SW interaction in which case the tacho evaluation is always done. The
only thing I could do is to completely ignore fan faults which is also bad... 

I can try to persuade the HW guy to completely remove the evaluation and
just give fan fauts in case there's no signal but I'm not really sure he will go
for it. In that case, I'm tempted to just leave this as-is (with the extra bindings
for the tolerance and turn these bindings into a map) if you're willing to take it...
The reason is that, as you said, this is likely to be unstable any ways so that the
added complexity in the SW does not really pay off (better keep at least the SW
simple)...

- Nuno Sá

Re: [RFC PATCH 3/6] dt-bindings: axi-fan-control: add tacho properties

[Guenter Roeck]

On 7/22/21 6:00 AM, Sa, Nuno wrote:
[ ... ]

>> I don't really know what to say or recommend here. Personally I think
>> any
>> attempt to tie PWM values to RPM are doomed to fail. Here are a
>> couple of
>> examples:
>>
>> Take your test system and move the fan to a restricted place (eg close
>> to a
>> wall). You'll see the fan RPM change, potentially significantly. Put it into
>> some place with airflow towards or away from the system (eg blow air
>> into
>> the system from another place, which may happen if the system is
>> installed
>> in a lab), and again you'll see fan speed changes. Open the chassis, and
>> the fan speed will change. I have seen fan speeds vary by up to 50%
>> when
>> changing airflow.
> 
> Here we can at least control the tolerance for each PWM vs RPM point but
> I can image this as a very painful process to get these values right and no
> one will think in setting tolerances of 50%...
> 
>> That doesn't even take into account that replacing a fan even with a
>> similar
>> model (eg after a fan failed) will likely result in potentially significant
>> rpm changes.
>>
>> Ultimately, anything that does more than determine if a fan is still
>> running
>> is potentially unstable.
> 
> Yeah, I understand your points. The HW does the evaluation and of
> course it also looks for the presence of a signal... So, in your opinion,
> not even setting a minimum fan speed is likely to be stable?
> 
Using the minimum fan speed as detection mechanism for fan failures is ok
and widely used. My concern is the desire to associate it with pwm values.

>> Having said all that, it is really your call to decide how you want to
>> detect
>> fan failures.
>>
> 
> Well, my hands are also tied here. The core is supposed to work without
> any SW interaction in which case the tacho evaluation is always done. The
> only thing I could do is to completely ignore fan faults which is also bad...
> 
As I said above, it would be perfectly fine to have a parameter that reflects
minimum fan speed (or, translated into chip speak, minimum number of pulses
per minute).

> I can try to persuade the HW guy to completely remove the evaluation and
> just give fan fauts in case there's no signal but I'm not really sure he will go
> for it. In that case, I'm tempted to just leave this as-is (with the extra bindings
> for the tolerance and turn these bindings into a map) if you're willing to take it...
> The reason is that, as you said, this is likely to be unstable any ways so that the
> added complexity in the SW does not really pay off (better keep at least the SW
> simple)...
> 
Sure, I'll take it, as long as you find a binding that is acceptable for Rob.
It is your funeral, after all :-).

Thanks,
Guenter

RE: [RFC PATCH 0/6] AXI FAN new features and improvements

[Sa, Nuno]

Hi Guenter,

> From: Nuno Sá <nuno.sa@analog.com>
> Sent: Thursday, July 8, 2021 2:01 PM
> To: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org
> Cc: Guenter Roeck <linux@roeck-us.net>; Rob Herring
> <robh+dt@kernel.org>; Jean Delvare <jdelvare@suse.com>
> Subject: [RFC PATCH 0/6] AXI FAN new features and improvements
> 
> This series adds some new features to the axi-fan-control driver. On
> top
> of that, the HW had some changes (basically it now starts automatically
> out of reset) so that the driver needed some minor refactoring. The
> reason I'm sending this as RFC, is mainly because of the last patch
> ("hwmon: axi-fan-control: support temperature vs pwm points"). The
> core
> has some predefined values which define a temperature vs pwm
> curve [1].
> It also exposes registers so that users can change it according to their
> needs. As I could not find standard attributes in the subsystem, I'm
> proposing some "raw" sysfs files. Looking at [2], the pwm_auto_point
> stuff looked to be what I want. Obviously I might be wrong :). If this
> is accepted, I will add a proper sysfs DOC file describing the new files
> (being lazy in the RFC).
> 
> For patch 5 ("hwmon: axi-fan-control: clear the fan fault irq at
> startup"),
> it's also arguable if we really need it. The main reason I have it is
> because of some userland apps that might take some drastic measures
> by
> just reading 1 fan_fault alarm. Obviously, we can argue that the
> problem
> is in the app and not in the driver. Though it's such a minimal change
> that I decided to include it (I'm more than fine in dropping the patch).
> 
> [1]: https://wiki.analog.com/resources/fpga/docs/axi_fan_control
> [2]:
> https://urldefense.com/v3/__https://www.kernel.org/doc/Documen
> tation/hwmon/sysfs-
> interface__;!!A3Ni8CS0y2Y!uwjpaOT8QEBVfKTCWELJNbjJJ69iR7S3tKS
> WV4B0K742CtcARkTtAqMxknnpPw$
> 
> Nuno Sá (6):
>   hwmon: axi-fan-control: make sure the clock is enabled
>   hwmon: axi-fan-control: add tacho devicetree properties
>   dt-bindings: axi-fan-control: add tacho properties
>   hwmon: axi-fan-control: handle irqs in natural order
>   hwmon: axi-fan-control: clear the fan fault irq at startup
>   hwmon: axi-fan-control: support temperature vs pwm points
> 

The HW guy is willing to change how the core works. This means,
that all that unstable pwm - rpm points will go away and we will
have a register where we can set the minimum fan speed for
evaluating the FAN. He also said that the default value for the
this setting will be pretty low so that we should only have _real_ 
faults at startup which means patch 5 should not be needed
anymore...

Anyways, I will send a new pull with patches 1,3 and 5 and
as soon as I have some HW ready to test, I will send the other
patches. With the new mechanism, we can also simplify the IRQ
handling [1]...

For the new devicetree property, I think now it really is a fan
property which makes me wonder if the property will be accepted
in the controller bindings or if I need to send a fan.yaml...

[1]: https://elixir.bootlin.com/linux/v5.14-rc3/source/drivers/hwmon/axi-fan-control.c#L280

- Nuno Sá

Re: [RFC PATCH 0/6] AXI FAN new features and improvements

[Guenter Roeck]

Hi,

On 7/27/21 1:42 AM, Sa, Nuno wrote:
> 
> Hi Guenter,
> 
>> From: Nuno Sá <nuno.sa@analog.com>
>> Sent: Thursday, July 8, 2021 2:01 PM
>> To: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org
>> Cc: Guenter Roeck <linux@roeck-us.net>; Rob Herring
>> <robh+dt@kernel.org>; Jean Delvare <jdelvare@suse.com>
>> Subject: [RFC PATCH 0/6] AXI FAN new features and improvements
>>
>> This series adds some new features to the axi-fan-control driver. On
>> top
>> of that, the HW had some changes (basically it now starts automatically
>> out of reset) so that the driver needed some minor refactoring. The
>> reason I'm sending this as RFC, is mainly because of the last patch
>> ("hwmon: axi-fan-control: support temperature vs pwm points"). The
>> core
>> has some predefined values which define a temperature vs pwm
>> curve [1].
>> It also exposes registers so that users can change it according to their
>> needs. As I could not find standard attributes in the subsystem, I'm
>> proposing some "raw" sysfs files. Looking at [2], the pwm_auto_point
>> stuff looked to be what I want. Obviously I might be wrong :). If this
>> is accepted, I will add a proper sysfs DOC file describing the new files
>> (being lazy in the RFC).
>>
>> For patch 5 ("hwmon: axi-fan-control: clear the fan fault irq at
>> startup"),
>> it's also arguable if we really need it. The main reason I have it is
>> because of some userland apps that might take some drastic measures
>> by
>> just reading 1 fan_fault alarm. Obviously, we can argue that the
>> problem
>> is in the app and not in the driver. Though it's such a minimal change
>> that I decided to include it (I'm more than fine in dropping the patch).
>>
>> [1]: https://wiki.analog.com/resources/fpga/docs/axi_fan_control
>> [2]:
>> https://urldefense.com/v3/__https://www.kernel.org/doc/Documen
>> tation/hwmon/sysfs-
>> interface__;!!A3Ni8CS0y2Y!uwjpaOT8QEBVfKTCWELJNbjJJ69iR7S3tKS
>> WV4B0K742CtcARkTtAqMxknnpPw$
>>
>> Nuno Sá (6):
>>    hwmon: axi-fan-control: make sure the clock is enabled
>>    hwmon: axi-fan-control: add tacho devicetree properties
>>    dt-bindings: axi-fan-control: add tacho properties
>>    hwmon: axi-fan-control: handle irqs in natural order
>>    hwmon: axi-fan-control: clear the fan fault irq at startup
>>    hwmon: axi-fan-control: support temperature vs pwm points
>>
> 
> The HW guy is willing to change how the core works. This means,
> that all that unstable pwm - rpm points will go away and we will
> have a register where we can set the minimum fan speed for
> evaluating the FAN. He also said that the default value for the
> this setting will be pretty low so that we should only have _real_
> faults at startup which means patch 5 should not be needed
> anymore...
> 
> Anyways, I will send a new pull with patches 1,3 and 5 and

That kind of contradicts what you say above, that patch 5 won't be
needed anymore. Am I missing something ?

> as soon as I have some HW ready to test, I will send the other
> patches. With the new mechanism, we can also simplify the IRQ
> handling [1]...
> 
> For the new devicetree property, I think now it really is a fan
> property which makes me wonder if the property will be accepted
> in the controller bindings or if I need to send a fan.yaml...
> 
Not really sure myself. At he very least we'll have sysfs properties,
so the minimum speed could also be updated from userspace. Ultimately
we'll need some set of devicetree properties, not only for fans but
for pretty much everything supported by hwmon, but I have no idea what
is acceptable and what isn't - if I did I might have proposed something
by now.

Guenter

RE: [RFC PATCH 0/6] AXI FAN new features and improvements

[Sa, Nuno]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter
> Roeck
> Sent: Wednesday, July 28, 2021 8:38 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org
> Cc: Rob Herring <robh+dt@kernel.org>; Jean Delvare
> <jdelvare@suse.com>
> Subject: Re: [RFC PATCH 0/6] AXI FAN new features and
> improvements
> 
> [External]
> 
> Hi,
> 
> On 7/27/21 1:42 AM, Sa, Nuno wrote:
> >
> > Hi Guenter,
> >
> >> From: Nuno Sá <nuno.sa@analog.com>
> >> Sent: Thursday, July 8, 2021 2:01 PM
> >> To: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org
> >> Cc: Guenter Roeck <linux@roeck-us.net>; Rob Herring
> >> <robh+dt@kernel.org>; Jean Delvare <jdelvare@suse.com>
> >> Subject: [RFC PATCH 0/6] AXI FAN new features and improvements
> >>
> >> This series adds some new features to the axi-fan-control driver.
> On
> >> top
> >> of that, the HW had some changes (basically it now starts
> automatically
> >> out of reset) so that the driver needed some minor refactoring. The
> >> reason I'm sending this as RFC, is mainly because of the last patch
> >> ("hwmon: axi-fan-control: support temperature vs pwm points").
> The
> >> core
> >> has some predefined values which define a temperature vs pwm
> >> curve [1].
> >> It also exposes registers so that users can change it according to
> their
> >> needs. As I could not find standard attributes in the subsystem, I'm
> >> proposing some "raw" sysfs files. Looking at [2], the
> pwm_auto_point
> >> stuff looked to be what I want. Obviously I might be wrong :). If this
> >> is accepted, I will add a proper sysfs DOC file describing the new
> files
> >> (being lazy in the RFC).
> >>
> >> For patch 5 ("hwmon: axi-fan-control: clear the fan fault irq at
> >> startup"),
> >> it's also arguable if we really need it. The main reason I have it is
> >> because of some userland apps that might take some drastic
> measures
> >> by
> >> just reading 1 fan_fault alarm. Obviously, we can argue that the
> >> problem
> >> is in the app and not in the driver. Though it's such a minimal change
> >> that I decided to include it (I'm more than fine in dropping the
> patch).
> >>
> >> [1]: https://wiki.analog.com/resources/fpga/docs/axi_fan_control
> >> [2]:
> >>
> https://urldefense.com/v3/__https://www.kernel.org/doc/Documen
> >> tation/hwmon/sysfs-
> >>
> interface__;!!A3Ni8CS0y2Y!uwjpaOT8QEBVfKTCWELJNbjJJ69iR7S3tKS
> >> WV4B0K742CtcARkTtAqMxknnpPw$
> >>
> >> Nuno Sá (6):
> >>    hwmon: axi-fan-control: make sure the clock is enabled
> >>    hwmon: axi-fan-control: add tacho devicetree properties
> >>    dt-bindings: axi-fan-control: add tacho properties
> >>    hwmon: axi-fan-control: handle irqs in natural order
> >>    hwmon: axi-fan-control: clear the fan fault irq at startup
> >>    hwmon: axi-fan-control: support temperature vs pwm points
> >>
> >
> > The HW guy is willing to change how the core works. This means,
> > that all that unstable pwm - rpm points will go away and we will
> > have a register where we can set the minimum fan speed for
> > evaluating the FAN. He also said that the default value for the
> > this setting will be pretty low so that we should only have _real_
> > faults at startup which means patch 5 should not be needed
> > anymore...
> >
> > Anyways, I will send a new pull with patches 1,3 and 5 and
> 
> That kind of contradicts what you say above, that patch 5 won't be
> needed anymore. Am I missing something ?

My bad... I meant patch 6 and not 5.

- Nuno Sá