Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer

From: Guenter Roeck
Date: Thu Mar 08 2018 - 09:33:18 EST

On 03/07/2018 11:57 PM, Mikko Perttunen wrote:


On 07.03.2018 16:20, Guenter Roeck wrote:
On 03/07/2018 01:47 AM, Rajkumar Rampelli wrote:


On Wednesday 28 February 2018 07:59 PM, Guenter Roeck wrote:
On 02/27/2018 11:03 PM, Mikko Perttunen wrote:
On 02/28/2018 08:12 AM, Rajkumar Rampelli wrote:

On Wednesday 28 February 2018 11:28 AM, Guenter Roeck wrote:
On 02/27/2018 09:38 PM, Rajkumar Rampelli wrote:

On Wednesday 21 February 2018 08:20 PM, Guenter Roeck wrote:
On 02/20/2018 10:58 PM, Rajkumar Rampelli wrote:
Add generic PWM based tachometer driver via HWMON interface
to report the RPM of motor. This drivers get the period/duty
cycle from PWM IP which captures the motor PWM output.

This driver implements a simple interface for monitoring the
speed of
a fan and exposes it in roatations per minute (RPM) to the user
space
by using the hwmon's sysfs interface

Signed-off-by: Rajkumar Rampelli <rrajk@xxxxxxxxxx>
---
 Documentation/hwmon/generic-pwm-tachometer | 17 +++++
 drivers/hwmon/Kconfig | 10 +++
 drivers/hwmon/Makefile | 1 +
 drivers/hwmon/generic-pwm-tachometer.c | 112
+++++++++++++++++++++++++++++
 4 files changed, 140 insertions(+)
 create mode 100644 Documentation/hwmon/generic-pwm-tachometer
 create mode 100644 drivers/hwmon/generic-pwm-tachometer.c

diff --git a/Documentation/hwmon/generic-pwm-tachometer
b/Documentation/hwmon/generic-pwm-tachometer
new file mode 100644
index 0000000..e0713ee
--- /dev/null
+++ b/Documentation/hwmon/generic-pwm-tachometer
@@ -0,0 +1,17 @@
+Kernel driver generic-pwm-tachometer
+====================================
+
+This driver enables the use of a PWM module to monitor a fan.
It uses the
+generic PWM interface and can be used on SoCs as along as the
SoC supports
+Tachometer controller that moniors the Fan speed in periods.
+
+Author: Rajkumar Rampelli <rrajk@xxxxxxxxxx>
+
+Description
+-----------
+
+The driver implements a simple interface for monitoring the
Fan speed using
+PWM module and Tachometer controller. It requests period value
through PWM
+capture interface to Tachometer and measures the Rotations per
minute using
+received period value. It exposes the Fan speed in RPM to the
user space by
+using the hwmon's sysfs interface.
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553..8912dcb 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1878,6 +1878,16 @@ config SENSORS_XGENE
ÂÂÂÂÂÂÂ If you say yes here you get support for the temperature
ÂÂÂÂÂÂÂ and power sensors for APM X-Gene SoC.
 +config GENERIC_PWM_TACHOMETER
+ÂÂÂ tristate "Generic PWM based tachometer driver"
+ÂÂÂ depends on PWM
+ÂÂÂ help
+ÂÂÂÂÂ Enables a driver to use PWM signal from motor to use
+ÂÂÂÂÂ for measuring the motor speed. The RPM is captured by
+ÂÂÂÂÂ PWM modules which has PWM capture capability and this
+ÂÂÂÂÂ drivers reads the captured data from PWM IP to convert
+ÂÂÂÂÂ it to speed in RPM.
+
 if ACPI
ÂÂÂ comment "ACPI drivers"
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index f814b4a..9dcc374 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -175,6 +175,7 @@ obj-$(CONFIG_SENSORS_WM8350)ÂÂÂ +=
wm8350-hwmon.o
 obj-$(CONFIG_SENSORS_XGENE) += xgene-hwmon.o
ÂÂÂ obj-$(CONFIG_PMBUS)ÂÂÂÂÂÂÂ += pmbus/
+obj-$(CONFIG_GENERIC_PWM_TACHOMETER) += generic-pwm-tachometer.o
ÂÂÂ ccflags-$(CONFIG_HWMON_DEBUG_CHIP) := -DDEBUG
 diff --git a/drivers/hwmon/generic-pwm-tachometer.c
b/drivers/hwmon/generic-pwm-tachometer.c
new file mode 100644
index 0000000..9354d43
--- /dev/null
+++ b/drivers/hwmon/generic-pwm-tachometer.c
@@ -0,0 +1,112 @@
+/*
+ * Copyright (c) 2017-2018, NVIDIA CORPORATION. All rights
reserved.
+ *
+ * This program is free software; you can redistribute it
and/or modify it
+ * under the terms and conditions of the GNU General Public
License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful,
but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General
Public License for
+ * more details.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/err.h>
+#include <linux/pwm.h>
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+
+struct pwm_hwmon_tach {
+ÂÂÂ struct deviceÂÂÂÂÂÂÂ *dev;
+ÂÂÂ struct pwm_deviceÂÂÂ *pwm;
+ÂÂÂ struct deviceÂÂÂÂÂÂÂ *hwmon;
+};
+
+static ssize_t show_rpm(struct device *dev, struct
device_attribute *attr,
+ÂÂÂÂÂÂÂÂÂÂÂ char *buf)
+{
+ÂÂÂ struct pwm_hwmon_tach *ptt = dev_get_drvdata(dev);
+ÂÂÂ struct pwm_device *pwm = ptt->pwm;
+ÂÂÂ struct pwm_capture result;
+ÂÂÂ int err;
+ÂÂÂ unsigned int rpm = 0;
+
+ÂÂÂ err = pwm_capture(pwm, &result, 0);
+ÂÂÂ if (err < 0) {
+ÂÂÂÂÂÂÂ dev_err(ptt->dev, "Failed to capture PWM: %d\n", err);
+ÂÂÂÂÂÂÂ return err;
+ÂÂÂ }
+
+ÂÂÂ if (result.period)
+ÂÂÂÂÂÂÂ rpm = DIV_ROUND_CLOSEST_ULL(60ULL * NSEC_PER_SEC,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ result.period);
+
+ÂÂÂ return sprintf(buf, "%u\n", rpm);
+}
+
+static SENSOR_DEVICE_ATTR(rpm, 0444, show_rpm, NULL, 0);
+
+static struct attribute *pwm_tach_attrs[] = {
+ÂÂÂ &sensor_dev_attr_rpm.dev_attr.attr,
+ÂÂÂ NULL,
+};

"rpm" is not a standard hwmon sysfs attribute. If you don't provide
a single standard hwmon sysfs attribute, having a hwmon driver
is pointless.
Guenter Roeck,
I will define a new hwmon sysfs attribute node called
"hwmon_tachometer_attributes" in hwmon.h like below and update
the same in tachometer hwmon driver. Is it fine ?
enum hwmon_tachometer_attributes {

Are you kidding me ?

Guenter
Sorry, I just wanted to confirm whether my understanding is correct
or not before implementing it actually.
Or, shall I add this attribute as a part of fan attributes with
"hwmon_fan_rpm" ? or any other way to do it ? I need your inputs in
fixing this.

I think he wants the attribute to be named according to the
properties in this document:
https://www.kernel.org/doc/Documentation/hwmon/sysfs-interface. I
guess the attribute would then map to fan1_input, though I'm not
sure if that's 100% correct either since this could technically be
attached to something other than a fan. But I would think in
practice that's not a big concern.

Guenter,
Please correct me as well if I'm wrong.

You are absolutely correct.

While I am not opposed to ABI changes, the merits of those would need
to be
discussed on the mailing list. But replacing "fan1_input" with "rpm" is
not an acceptable ABI change, even if it may measure something that
turns
but isn't a fan.

If this _is_ in fact supposed to be used for something else but fans, we
would have to discuss what that might be, and if hwmon is the
appropriate
subsystem to measure and report it. This does to some degree lead
back to
my concern of having the "fan" part of this patch series in the pwm
core.
I am still not sure if that makes sense.

Thanks,
Guenter
I am planning to add tachometer support in pwm-fan.c driver
(drivers/hwmon/) instead of adding new generic-pwm-tachometer.c
driver. Measuring RPM value will be done in pwm-fan driver itself
using pwm capture feature and will add new sysfs attributes under this
driver to report rpm value of fan.

There is an existing attribute to report the RPM of fans. It is called
fan[1..n]_input.

"replacing "fan1_input" with "rpm" is not an acceptable ABI change"

Preemptive NACK.

Guenter


I think the idea here (based on personal discussion with Rajkumar) was to add a "new" sysfs attribute the driver exports, but that attribute would be the existing fan[1..n]_input.


Ah, sorry, in this case misunderstanding on my side.

Guenter