[PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs

[PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs

[Paul Barker]

These changes were made to support a custom board where one PWM output
is routed to two fans, each of which has a tachometer signal routed to a
GPIO input on the SoC.

As the custom board doesn't currently support the latest mainline kernel
I've tested these changes on a SanCloud BeagleBone Enhanced using an
oscilloscope to check the PWM output and a signal generator to simulate
the fan tachometer signals. I've tested variants of the device tree with
0, 1 and 2 fan tachometer inputs configured to ensure the logic in the
probe function is correct.

The device tree bindings changes have been submitted in a separate
series:
https://lore.kernel.org/linux-devicetree/20200920180943.352526-1-pbarker@konsulko.com/

These changes can also be pulled from:

  https://gitlab.com/pbarker.dev/staging/linux.git
  tag: for-hwmon/pwm-fan-tachometers-v2_2020-11-13

Changes since v1:

  * Split RPM calculation fix into a separate patch which has now been
    accepted.

  * Break the changes down into smaller patches so they're easier to
    review.

  * Rebased changes on hwmon-next.

Paul Barker (4):
  hwmon: pwm-fan: Refactor pwm_fan_probe
  hwmon: pwm-fan: Dynamically setup attribute groups
  hwmon: pwm-fan: Store tach data separately
  hwmon: pwm-fan: Support multiple fan tachometers

 drivers/hwmon/pwm-fan.c | 155 ++++++++++++++++++++++++----------------
 1 file changed, 95 insertions(+), 60 deletions(-)


base-commit: 414920a4a5d5613e4aa77c89944f9c1dc86b06c4
-- 
2.29.2

[PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe

[Paul Barker]

Use platform_irq_count to determine the number of fan tachometer inputs
configured in the device tree. At this stage we support either 0 or 1
inputs.

Once we have this information we only need to read the
pulses-per-revolution value if a fan tachometer is actually configured
via an IRQ value.

Also add a debug print of the IRQ number and the pulses-per-revolution
value to aid in investigating issues.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/hwmon/pwm-fan.c | 50 +++++++++++++++++++++++++++--------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 1f63807c0399..efe2764f42d3 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -286,7 +286,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	struct device *hwmon;
 	int ret;
 	struct pwm_state state = { };
-	u32 ppr = 2;
+	int tach_count;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
@@ -300,10 +300,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, ctx);
 
-	ctx->irq = platform_get_irq_optional(pdev, 0);
-	if (ctx->irq == -EPROBE_DEFER)
-		return ctx->irq;
-
 	ctx->reg_en = devm_regulator_get_optional(dev, "fan");
 	if (IS_ERR(ctx->reg_en)) {
 		if (PTR_ERR(ctx->reg_en) != -ENODEV)
@@ -339,20 +335,40 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	of_property_read_u32(dev->of_node, "pulses-per-revolution", &ppr);
-	ctx->pulses_per_revolution = ppr;
-	if (!ctx->pulses_per_revolution) {
-		dev_err(dev, "pulses-per-revolution can't be zero.\n");
-		return -EINVAL;
-	}
+	tach_count = platform_irq_count(pdev);
+	if (tach_count < 0)
+		return dev_err_probe(dev, tach_count,
+				     "Could not get number of fan tachometer inputs\n");
+
+	if (tach_count > 0) {
+		u32 ppr = 2;
+
+		ctx->irq = platform_get_irq(pdev, 0);
+		if (ctx->irq == -EPROBE_DEFER)
+			return ctx->irq;
+		if (ctx->irq > 0) {
+			ret = devm_request_irq(dev, ctx->irq, pulse_handler, 0,
+					       pdev->name, ctx);
+			if (ret) {
+				dev_err(dev,
+					"Failed to request interrupt: %d\n",
+					ret);
+				return ret;
+			}
+		}
 
-	if (ctx->irq > 0) {
-		ret = devm_request_irq(dev, ctx->irq, pulse_handler, 0,
-				       pdev->name, ctx);
-		if (ret) {
-			dev_err(dev, "Failed to request interrupt: %d\n", ret);
-			return ret;
+		of_property_read_u32(dev->of_node,
+				     "pulses-per-revolution",
+				     &ppr);
+		ctx->pulses_per_revolution = ppr;
+		if (!ctx->pulses_per_revolution) {
+			dev_err(dev, "pulses-per-revolution can't be zero.\n");
+			return -EINVAL;
 		}
+
+		dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
+			ctx->irq, ctx->pulses_per_revolution);
+
 		ctx->sample_start = ktime_get();
 		mod_timer(&ctx->rpm_timer, jiffies + HZ);
 	}
-- 
2.29.2

[PATCH v2 2/4] hwmon: pwm-fan: Dynamically setup attribute groups

[Paul Barker]

Instead of implementing an is_visible function we can dynamically
populate the attribute group for the device based on whether a fan
tachometer input is configured or not.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/hwmon/pwm-fan.c | 55 +++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 32 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index efe2764f42d3..c4e0059ccaec 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -31,6 +31,8 @@ struct pwm_fan_ctx {
 	atomic_t pulses;
 	unsigned int rpm;
 	u8 pulses_per_revolution;
+	struct sensor_device_attribute sensor_attr;
+
 	ktime_t sample_start;
 	struct timer_list rpm_timer;
 
@@ -39,6 +41,9 @@ struct pwm_fan_ctx {
 	unsigned int pwm_fan_max_state;
 	unsigned int *pwm_fan_cooling_levels;
 	struct thermal_cooling_device *cdev;
+
+	struct attribute_group fan_group;
+	struct attribute_group *fan_groups[2];
 };
 
 /* This handler assumes self resetting edge triggered interrupt. */
@@ -138,36 +143,6 @@ static ssize_t rpm_show(struct device *dev,
 }
 
 static SENSOR_DEVICE_ATTR_RW(pwm1, pwm, 0);
-static SENSOR_DEVICE_ATTR_RO(fan1_input, rpm, 0);
-
-static struct attribute *pwm_fan_attrs[] = {
-	&sensor_dev_attr_pwm1.dev_attr.attr,
-	&sensor_dev_attr_fan1_input.dev_attr.attr,
-	NULL,
-};
-
-static umode_t pwm_fan_attrs_visible(struct kobject *kobj, struct attribute *a,
-				     int n)
-{
-	struct device *dev = container_of(kobj, struct device, kobj);
-	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
-
-	/* Hide fan_input in case no interrupt is available  */
-	if (n == 1 && ctx->irq <= 0)
-		return 0;
-
-	return a->mode;
-}
-
-static const struct attribute_group pwm_fan_group = {
-	.attrs = pwm_fan_attrs,
-	.is_visible = pwm_fan_attrs_visible,
-};
-
-static const struct attribute_group *pwm_fan_groups[] = {
-	&pwm_fan_group,
-	NULL,
-};
 
 /* thermal cooling device callbacks */
 static int pwm_fan_get_max_state(struct thermal_cooling_device *cdev,
@@ -287,6 +262,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	int ret;
 	struct pwm_state state = { };
 	int tach_count;
+	size_t sz;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
@@ -340,6 +316,13 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, tach_count,
 				     "Could not get number of fan tachometer inputs\n");
 
+	sz = (2 + tach_count) * sizeof(struct attribute *);
+	ctx->fan_group.attrs = devm_kzalloc(dev, sz, GFP_KERNEL);
+	if (!ctx->fan_group.attrs)
+		return -ENOMEM;
+
+	ctx->fan_group.attrs[0] = &sensor_dev_attr_pwm1.dev_attr.attr;
+
 	if (tach_count > 0) {
 		u32 ppr = 2;
 
@@ -366,6 +349,13 @@ static int pwm_fan_probe(struct platform_device *pdev)
 			return -EINVAL;
 		}
 
+		sysfs_attr_init(&ctx->sensor_attr.dev_attr.attr);
+
+		ctx->sensor_attr.dev_attr.attr.name = "fan1_input";
+		ctx->sensor_attr.dev_attr.attr.mode = 0444;
+		ctx->sensor_attr.dev_attr.show = rpm_show;
+		ctx->fan_group.attrs[1] = &ctx->sensor_attr.dev_attr.attr;
+
 		dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
 			ctx->irq, ctx->pulses_per_revolution);
 
@@ -373,8 +363,9 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		mod_timer(&ctx->rpm_timer, jiffies + HZ);
 	}
 
-	hwmon = devm_hwmon_device_register_with_groups(dev, "pwmfan",
-						       ctx, pwm_fan_groups);
+	ctx->fan_groups[0] = &ctx->fan_group;
+	hwmon = devm_hwmon_device_register_with_groups(dev, "pwmfan", ctx,
+			(const struct attribute_group **)ctx->fan_groups);
 	if (IS_ERR(hwmon)) {
 		dev_err(dev, "Failed to register hwmon device\n");
 		return PTR_ERR(hwmon);
-- 
2.29.2

[PATCH v2 3/4] hwmon: pwm-fan: Store tach data separately

[Paul Barker]

The data for the (optional) fan tachometer input is moved to a separate
structure which is only allocated if an input is actually configured.

After this change the pulse IRQ handler takes a pointer to the
tachometer data structure instead of the whole device context.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/hwmon/pwm-fan.c | 66 +++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 26 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index c4e0059ccaec..7c75ce78b36a 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -22,17 +22,21 @@
 
 #define MAX_PWM 255
 
-struct pwm_fan_ctx {
-	struct mutex lock;
-	struct pwm_device *pwm;
-	struct regulator *reg_en;
-
+struct pwm_fan_tach {
 	int irq;
 	atomic_t pulses;
 	unsigned int rpm;
 	u8 pulses_per_revolution;
+
 	struct sensor_device_attribute sensor_attr;
+};
 
+struct pwm_fan_ctx {
+	struct mutex lock;
+	struct pwm_device *pwm;
+	struct regulator *reg_en;
+
+	struct pwm_fan_tach *tach;
 	ktime_t sample_start;
 	struct timer_list rpm_timer;
 
@@ -49,9 +53,9 @@ struct pwm_fan_ctx {
 /* This handler assumes self resetting edge triggered interrupt. */
 static irqreturn_t pulse_handler(int irq, void *dev_id)
 {
-	struct pwm_fan_ctx *ctx = dev_id;
+	struct pwm_fan_tach *tach = dev_id;
 
-	atomic_inc(&ctx->pulses);
+	atomic_inc(&tach->pulses);
 
 	return IRQ_HANDLED;
 }
@@ -59,14 +63,15 @@ static irqreturn_t pulse_handler(int irq, void *dev_id)
 static void sample_timer(struct timer_list *t)
 {
 	struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer);
+	struct pwm_fan_tach *tach = ctx->tach;
 	unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start);
 	int pulses;
 
 	if (delta) {
-		pulses = atomic_read(&ctx->pulses);
-		atomic_sub(pulses, &ctx->pulses);
-		ctx->rpm = (unsigned int)(pulses * 1000 * 60) /
-			(ctx->pulses_per_revolution * delta);
+		pulses = atomic_read(&tach->pulses);
+		atomic_sub(pulses, &tach->pulses);
+		tach->rpm = (unsigned int)(pulses * 1000 * 60) /
+			(tach->pulses_per_revolution * delta);
 
 		ctx->sample_start = ktime_get();
 	}
@@ -138,8 +143,9 @@ static ssize_t rpm_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
 	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
+	struct pwm_fan_tach *tach = ctx->tach;
 
-	return sprintf(buf, "%u\n", ctx->rpm);
+	return sprintf(buf, "%u\n", tach->rpm);
 }
 
 static SENSOR_DEVICE_ATTR_RW(pwm1, pwm, 0);
@@ -324,14 +330,22 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	ctx->fan_group.attrs[0] = &sensor_dev_attr_pwm1.dev_attr.attr;
 
 	if (tach_count > 0) {
+		struct pwm_fan_tach *tach;
 		u32 ppr = 2;
 
-		ctx->irq = platform_get_irq(pdev, 0);
-		if (ctx->irq == -EPROBE_DEFER)
-			return ctx->irq;
-		if (ctx->irq > 0) {
-			ret = devm_request_irq(dev, ctx->irq, pulse_handler, 0,
-					       pdev->name, ctx);
+		ctx->tach = devm_kzalloc(dev, sizeof(struct pwm_fan_tach),
+					 GFP_KERNEL);
+		if (!ctx->tach)
+			return -ENOMEM;
+
+		tach = ctx->tach;
+
+		tach->irq = platform_get_irq(pdev, 0);
+		if (tach->irq == -EPROBE_DEFER)
+			return tach->irq;
+		if (tach->irq > 0) {
+			ret = devm_request_irq(dev, tach->irq, pulse_handler, 0,
+					       pdev->name, tach);
 			if (ret) {
 				dev_err(dev,
 					"Failed to request interrupt: %d\n",
@@ -343,21 +357,21 @@ static int pwm_fan_probe(struct platform_device *pdev)
 		of_property_read_u32(dev->of_node,
 				     "pulses-per-revolution",
 				     &ppr);
-		ctx->pulses_per_revolution = ppr;
-		if (!ctx->pulses_per_revolution) {
+		tach->pulses_per_revolution = ppr;
+		if (!tach->pulses_per_revolution) {
 			dev_err(dev, "pulses-per-revolution can't be zero.\n");
 			return -EINVAL;
 		}
 
-		sysfs_attr_init(&ctx->sensor_attr.dev_attr.attr);
+		sysfs_attr_init(&tach->sensor_attr.dev_attr.attr);
 
-		ctx->sensor_attr.dev_attr.attr.name = "fan1_input";
-		ctx->sensor_attr.dev_attr.attr.mode = 0444;
-		ctx->sensor_attr.dev_attr.show = rpm_show;
-		ctx->fan_group.attrs[1] = &ctx->sensor_attr.dev_attr.attr;
+		tach->sensor_attr.dev_attr.attr.name = "fan1_input";
+		tach->sensor_attr.dev_attr.attr.mode = 0444;
+		tach->sensor_attr.dev_attr.show = rpm_show;
+		ctx->fan_group.attrs[1] = &tach->sensor_attr.dev_attr.attr;
 
 		dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
-			ctx->irq, ctx->pulses_per_revolution);
+			tach->irq, tach->pulses_per_revolution);
 
 		ctx->sample_start = ktime_get();
 		mod_timer(&ctx->rpm_timer, jiffies + HZ);
-- 
2.29.2

[PATCH v2 4/4] hwmon: pwm-fan: Support multiple fan tachometers

[Paul Barker]

The pwm-fan driver is extended to support multiple fan tachometer
signals connected to GPIO inputs. This is intended to support the case
where a single PWM output signal is routed to multiple fans, each of
which have a tachometer output connected back to a GPIO pin.

The number of fan tachometer inputs is determined by the number of
interrupt sources configured for the pwm-fan device. The number of
pulses-per-revolution entries should match the number of interrupt
sources so that each input has a value assigned.

The fan tachometer measurements are exposed as sysfs files fan1_input,
fan2_input, etc up to the number of configured inputs.

Signed-off-by: Paul Barker <pbarker@konsulko.com>
---
 drivers/hwmon/pwm-fan.c | 72 ++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 29 deletions(-)

diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 7c75ce78b36a..03a3b57e2d99 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -36,7 +36,8 @@ struct pwm_fan_ctx {
 	struct pwm_device *pwm;
 	struct regulator *reg_en;
 
-	struct pwm_fan_tach *tach;
+	int tach_count;
+	struct pwm_fan_tach *tachs;
 	ktime_t sample_start;
 	struct timer_list rpm_timer;
 
@@ -63,15 +64,19 @@ static irqreturn_t pulse_handler(int irq, void *dev_id)
 static void sample_timer(struct timer_list *t)
 {
 	struct pwm_fan_ctx *ctx = from_timer(ctx, t, rpm_timer);
-	struct pwm_fan_tach *tach = ctx->tach;
 	unsigned int delta = ktime_ms_delta(ktime_get(), ctx->sample_start);
-	int pulses;
+	int i;
 
 	if (delta) {
-		pulses = atomic_read(&tach->pulses);
-		atomic_sub(pulses, &tach->pulses);
-		tach->rpm = (unsigned int)(pulses * 1000 * 60) /
-			(tach->pulses_per_revolution * delta);
+		for (i = 0; i < ctx->tach_count; i++) {
+			struct pwm_fan_tach *tach = &ctx->tachs[i];
+			int pulses;
+
+			pulses = atomic_read(&tach->pulses);
+			atomic_sub(pulses, &tach->pulses);
+			tach->rpm = (unsigned int)(pulses * 1000 * 60) /
+				(tach->pulses_per_revolution * delta);
+		}
 
 		ctx->sample_start = ktime_get();
 	}
@@ -143,7 +148,8 @@ static ssize_t rpm_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
 	struct pwm_fan_ctx *ctx = dev_get_drvdata(dev);
-	struct pwm_fan_tach *tach = ctx->tach;
+	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
+	struct pwm_fan_tach *tach = &ctx->tachs[sensor_attr->index];
 
 	return sprintf(buf, "%u\n", tach->rpm);
 }
@@ -267,8 +273,8 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	struct device *hwmon;
 	int ret;
 	struct pwm_state state = { };
-	int tach_count;
 	size_t sz;
+	int i;
 
 	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
@@ -317,30 +323,32 @@ static int pwm_fan_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	tach_count = platform_irq_count(pdev);
-	if (tach_count < 0)
-		return dev_err_probe(dev, tach_count,
+	ctx->tach_count = platform_irq_count(pdev);
+	if (ctx->tach_count < 0)
+		return dev_err_probe(dev, ctx->tach_count,
 				     "Could not get number of fan tachometer inputs\n");
+	dev_dbg(dev, "%d fan tachometer inputs\n", ctx->tach_count);
 
-	sz = (2 + tach_count) * sizeof(struct attribute *);
+	sz = (2 + ctx->tach_count) * sizeof(struct attribute *);
 	ctx->fan_group.attrs = devm_kzalloc(dev, sz, GFP_KERNEL);
 	if (!ctx->fan_group.attrs)
 		return -ENOMEM;
 
 	ctx->fan_group.attrs[0] = &sensor_dev_attr_pwm1.dev_attr.attr;
 
-	if (tach_count > 0) {
-		struct pwm_fan_tach *tach;
-		u32 ppr = 2;
-
-		ctx->tach = devm_kzalloc(dev, sizeof(struct pwm_fan_tach),
-					 GFP_KERNEL);
-		if (!ctx->tach)
+	if (ctx->tach_count > 0) {
+		sz = ctx->tach_count * sizeof(struct pwm_fan_tach);
+		ctx->tachs = devm_kzalloc(dev, sz, GFP_KERNEL);
+		if (!ctx->tachs)
 			return -ENOMEM;
+	}
 
-		tach = ctx->tach;
+	for (i = 0; i < ctx->tach_count; i++) {
+		struct pwm_fan_tach *tach = &ctx->tachs[i];
+		u32 ppr = 2;
+		char *name;
 
-		tach->irq = platform_get_irq(pdev, 0);
+		tach->irq = platform_get_irq(pdev, i);
 		if (tach->irq == -EPROBE_DEFER)
 			return tach->irq;
 		if (tach->irq > 0) {
@@ -354,9 +362,10 @@ static int pwm_fan_probe(struct platform_device *pdev)
 			}
 		}
 
-		of_property_read_u32(dev->of_node,
-				     "pulses-per-revolution",
-				     &ppr);
+		of_property_read_u32_index(dev->of_node,
+					   "pulses-per-revolution",
+					   i,
+					   &ppr);
 		tach->pulses_per_revolution = ppr;
 		if (!tach->pulses_per_revolution) {
 			dev_err(dev, "pulses-per-revolution can't be zero.\n");
@@ -365,14 +374,19 @@ static int pwm_fan_probe(struct platform_device *pdev)
 
 		sysfs_attr_init(&tach->sensor_attr.dev_attr.attr);
 
-		tach->sensor_attr.dev_attr.attr.name = "fan1_input";
+		name = devm_kzalloc(dev, 16, GFP_KERNEL);
+		snprintf(name, 16, "fan%d_input", i + 1);
+		tach->sensor_attr.dev_attr.attr.name = name;
 		tach->sensor_attr.dev_attr.attr.mode = 0444;
 		tach->sensor_attr.dev_attr.show = rpm_show;
-		ctx->fan_group.attrs[1] = &tach->sensor_attr.dev_attr.attr;
+		tach->sensor_attr.index = i;
+		ctx->fan_group.attrs[i + 1] = &tach->sensor_attr.dev_attr.attr;
 
-		dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
-			tach->irq, tach->pulses_per_revolution);
+		dev_dbg(dev, "%s: irq=%d, pulses_per_revolution=%d\n",
+			name, tach->irq, tach->pulses_per_revolution);
+	}
 
+	if (ctx->tach_count > 0) {
 		ctx->sample_start = ktime_get();
 		mod_timer(&ctx->rpm_timer, jiffies + HZ);
 	}
-- 
2.29.2

Re: [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs

[Paul Barker]

On Fri, 13 Nov 2020 at 15:09, Paul Barker <pbarker@konsulko.com> wrote:
>
> These changes were made to support a custom board where one PWM output
> is routed to two fans, each of which has a tachometer signal routed to a
> GPIO input on the SoC.
>
> As the custom board doesn't currently support the latest mainline kernel
> I've tested these changes on a SanCloud BeagleBone Enhanced using an
> oscilloscope to check the PWM output and a signal generator to simulate
> the fan tachometer signals. I've tested variants of the device tree with
> 0, 1 and 2 fan tachometer inputs configured to ensure the logic in the
> probe function is correct.
>
> The device tree bindings changes have been submitted in a separate
> series:
> https://lore.kernel.org/linux-devicetree/20200920180943.352526-1-pbarker@konsulko.com/
>
> These changes can also be pulled from:
>
>   https://gitlab.com/pbarker.dev/staging/linux.git
>   tag: for-hwmon/pwm-fan-tachometers-v2_2020-11-13
>
> Changes since v1:
>
>   * Split RPM calculation fix into a separate patch which has now been
>     accepted.
>
>   * Break the changes down into smaller patches so they're easier to
>     review.
>
>   * Rebased changes on hwmon-next.
>
> Paul Barker (4):
>   hwmon: pwm-fan: Refactor pwm_fan_probe
>   hwmon: pwm-fan: Dynamically setup attribute groups
>   hwmon: pwm-fan: Store tach data separately
>   hwmon: pwm-fan: Support multiple fan tachometers
>
>  drivers/hwmon/pwm-fan.c | 155 ++++++++++++++++++++++++----------------
>  1 file changed, 95 insertions(+), 60 deletions(-)
>
>
> base-commit: 414920a4a5d5613e4aa77c89944f9c1dc86b06c4
> --
> 2.29.2
>

Has anyone had a chance to look at this series? I see that the
corresponding dt-bindings change has been accepted into the hwmon-next
tree but I've not heard anything back on these patches.

I also see that a patch just got sent to this list which will conflict
with this series
(https://lore.kernel.org/linux-hwmon/20201125163242.GA1264232@paju/T/#u).
It'd be good to get feedback so that either myself, Dongjin Kim or
both of us can re-work our patches to be compatible.

Thanks,

-- 
Paul Barker
Konsulko Group

Re: [PATCH v2 0/4] pwm-fan: Support multiple tachometer inputs

[Guenter Roeck]

On Wed, Nov 25, 2020 at 04:45:27PM +0000, Paul Barker wrote:
> On Fri, 13 Nov 2020 at 15:09, Paul Barker <pbarker@konsulko.com> wrote:
> >
> > These changes were made to support a custom board where one PWM output
> > is routed to two fans, each of which has a tachometer signal routed to a
> > GPIO input on the SoC.
> >
> > As the custom board doesn't currently support the latest mainline kernel
> > I've tested these changes on a SanCloud BeagleBone Enhanced using an
> > oscilloscope to check the PWM output and a signal generator to simulate
> > the fan tachometer signals. I've tested variants of the device tree with
> > 0, 1 and 2 fan tachometer inputs configured to ensure the logic in the
> > probe function is correct.
> >
> > The device tree bindings changes have been submitted in a separate
> > series:
> > https://lore.kernel.org/linux-devicetree/20200920180943.352526-1-pbarker@konsulko.com/
> >
> > These changes can also be pulled from:
> >
> >   https://gitlab.com/pbarker.dev/staging/linux.git
> >   tag: for-hwmon/pwm-fan-tachometers-v2_2020-11-13
> >
> > Changes since v1:
> >
> >   * Split RPM calculation fix into a separate patch which has now been
> >     accepted.
> >
> >   * Break the changes down into smaller patches so they're easier to
> >     review.
> >
> >   * Rebased changes on hwmon-next.
> >
> > Paul Barker (4):
> >   hwmon: pwm-fan: Refactor pwm_fan_probe
> >   hwmon: pwm-fan: Dynamically setup attribute groups
> >   hwmon: pwm-fan: Store tach data separately
> >   hwmon: pwm-fan: Support multiple fan tachometers
> >
> >  drivers/hwmon/pwm-fan.c | 155 ++++++++++++++++++++++++----------------
> >  1 file changed, 95 insertions(+), 60 deletions(-)
> >
> >
> > base-commit: 414920a4a5d5613e4aa77c89944f9c1dc86b06c4
> > --
> > 2.29.2
> >
> 
> Has anyone had a chance to look at this series? I see that the
> corresponding dt-bindings change has been accepted into the hwmon-next
> tree but I've not heard anything back on these patches.
> 
Still on my list. Sorry, I have been buried in work, and being stuck in
the middle of nowhere (aka Wyoming) last week with a broken charge port
on a Tesla didn't help.

> I also see that a patch just got sent to this list which will conflict
> with this series
> (https://lore.kernel.org/linux-hwmon/20201125163242.GA1264232@paju/T/#u).
> It'd be good to get feedback so that either myself, Dongjin Kim or
> both of us can re-work our patches to be compatible.
> 
I have seen it. I hope I'll get to your series over the weekend.
Sorry for the delay.

Guenter

Re: [PATCH v2 1/4] hwmon: pwm-fan: Refactor pwm_fan_probe

[Guenter Roeck]

On Fri, Nov 13, 2020 at 03:08:50PM +0000, Paul Barker wrote:
> Use platform_irq_count to determine the number of fan tachometer inputs
> configured in the device tree. At this stage we support either 0 or 1
> inputs.
> 
> Once we have this information we only need to read the
> pulses-per-revolution value if a fan tachometer is actually configured
> via an IRQ value.
> 
> Also add a debug print of the IRQ number and the pulses-per-revolution
> value to aid in investigating issues.
> 
> Signed-off-by: Paul Barker <pbarker@konsulko.com>

For my reference:

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/hwmon/pwm-fan.c | 50 +++++++++++++++++++++++++++--------------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 1f63807c0399..efe2764f42d3 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -286,7 +286,7 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	struct device *hwmon;
>  	int ret;
>  	struct pwm_state state = { };
> -	u32 ppr = 2;
> +	int tach_count;
>  
>  	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>  	if (!ctx)
> @@ -300,10 +300,6 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, ctx);
>  
> -	ctx->irq = platform_get_irq_optional(pdev, 0);
> -	if (ctx->irq == -EPROBE_DEFER)
> -		return ctx->irq;
> -
>  	ctx->reg_en = devm_regulator_get_optional(dev, "fan");
>  	if (IS_ERR(ctx->reg_en)) {
>  		if (PTR_ERR(ctx->reg_en) != -ENODEV)
> @@ -339,20 +335,40 @@ static int pwm_fan_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	of_property_read_u32(dev->of_node, "pulses-per-revolution", &ppr);
> -	ctx->pulses_per_revolution = ppr;
> -	if (!ctx->pulses_per_revolution) {
> -		dev_err(dev, "pulses-per-revolution can't be zero.\n");
> -		return -EINVAL;
> -	}
> +	tach_count = platform_irq_count(pdev);
> +	if (tach_count < 0)
> +		return dev_err_probe(dev, tach_count,
> +				     "Could not get number of fan tachometer inputs\n");
> +
> +	if (tach_count > 0) {
> +		u32 ppr = 2;
> +
> +		ctx->irq = platform_get_irq(pdev, 0);
> +		if (ctx->irq == -EPROBE_DEFER)
> +			return ctx->irq;
> +		if (ctx->irq > 0) {
> +			ret = devm_request_irq(dev, ctx->irq, pulse_handler, 0,
> +					       pdev->name, ctx);
> +			if (ret) {
> +				dev_err(dev,
> +					"Failed to request interrupt: %d\n",
> +					ret);
> +				return ret;
> +			}
> +		}
>  
> -	if (ctx->irq > 0) {
> -		ret = devm_request_irq(dev, ctx->irq, pulse_handler, 0,
> -				       pdev->name, ctx);
> -		if (ret) {
> -			dev_err(dev, "Failed to request interrupt: %d\n", ret);
> -			return ret;
> +		of_property_read_u32(dev->of_node,
> +				     "pulses-per-revolution",
> +				     &ppr);
> +		ctx->pulses_per_revolution = ppr;
> +		if (!ctx->pulses_per_revolution) {
> +			dev_err(dev, "pulses-per-revolution can't be zero.\n");
> +			return -EINVAL;
>  		}
> +
> +		dev_dbg(dev, "tach: irq=%d, pulses_per_revolution=%d\n",
> +			ctx->irq, ctx->pulses_per_revolution);
> +
>  		ctx->sample_start = ktime_get();
>  		mod_timer(&ctx->rpm_timer, jiffies + HZ);
>  	}
> -- 
> 2.29.2
> 

Re: [PATCH v2 2/4] hwmon: pwm-fan: Dynamically setup attribute groups

[Paul Barker]

On Thu, 26 Nov 2020 at 01:45, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Fri, Nov 13, 2020 at 03:08:51PM +0000, Paul Barker wrote:
> > Instead of implementing an is_visible function we can dynamically
> > populate the attribute group for the device based on whether a fan
> > tachometer input is configured or not.
> >
> > Signed-off-by: Paul Barker <pbarker@konsulko.com>
>
> This will make it more difficult to ever convert the driver to use the
> devm_hwmon_device_register_with_info() API. I'd rather see a conversion
> to that API now instead of making it more difficult to implement in the
> future.

Ah I hadn't realised that was the preferred API. I'll take a look and
see if I can convert the driver, perhaps as a standalone patch so we
can get this right before I re-work my implementation of multiple
tachometer input support.

Thanks,

-- 
Paul Barker
Konsulko Group