Re: [PATCH V2 3/9] dt-bindings: Tegra186 tachometer device tree bindings

[Mikko Perttunen]

On 04/09/2018 04:21 PM, Rob Herring wrote:

On Mon, Apr 9, 2018 at 12:38 AM, Mikko Perttunen <cyn...@kapsi.fi> wrote:

Rob,


Please don't top post to lists.


this binding is for a specific IP block (for measuring/aggregating input
pulses) on the Tegra186 SoC, so I don't think it fits into any generic
binding.


What is it hooked up to to measure? You only mention "fan" five times
in the doc.


In practice, fans.



You have #pwm-cells too, so this block has PWM output as well? If not,
then where's the PWM for the fan control because there is no point in
having fan tach without some control mechanism.


It doesn't provide a PWM output. The (Linux) PWM framework provides 
functionality in both directions - control and capture. But if the 
device tree #pwm-cells/pwms properties are only for control, we may need 
to introduce a new #capture-pwm-cells/capture-pwms or similar.


The idea is that the generic fan node can then specify two pwms, one for 
control and one for capture, to enable e.g. closed-loop control (I'm not 
personally familiar with the usecase for this but I could imagine 
something like that). The control PWM can be something completely 
different, maybe not a PWM in the first place (e.g. some fixed voltage).




There's only so many ways to control fans and types of fans, so yes,
the interface of control and feedback lines between a fan and its
controller should absolutely be generic.


I'm not quite getting what you mean by this. Clearly we need a custom 
compatibility string for the tachometer as it's a different hardware 
block with different programming than others. Or are you complaining 
about the nvidia,pulse-per-rev/capture-window-len properties?


Thanks,
Mikko



Rob



Thanks,
Mikko


On 03/27/2018 05:52 PM, Rob Herring wrote:


On Wed, Mar 21, 2018 at 10:10:38AM +0530, Rajkumar Rampelli wrote:


Supply Device tree binding documentation for the NVIDIA
Tegra186 SoC's Tachometer Controller

Signed-off-by: Rajkumar Rampelli <rr...@nvidia.com>
---

V2: Renamed compatible string to "nvidia,tegra186-pwm-tachometer"
  Renamed dt property values of clock-names and reset-names to
"tachometer"
  from "tach"



Read my prior comments on v1.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2 3/9] dt-bindings: Tegra186 tachometer device tree bindings

[Mikko Perttunen]

Rob,

this binding is for a specific IP block (for measuring/aggregating input 
pulses) on the Tegra186 SoC, so I don't think it fits into any generic 
binding.


Thanks,
Mikko

On 03/27/2018 05:52 PM, Rob Herring wrote:

On Wed, Mar 21, 2018 at 10:10:38AM +0530, Rajkumar Rampelli wrote:

Supply Device tree binding documentation for the NVIDIA
Tegra186 SoC's Tachometer Controller

Signed-off-by: Rajkumar Rampelli 
---

V2: Renamed compatible string to "nvidia,tegra186-pwm-tachometer"
 Renamed dt property values of clock-names and reset-names to "tachometer"
 from "tach"


Read my prior comments on v1.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mikko Perttunen]

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 <rr...@nvidia.com>
---
  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 000..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 <rr...@nvidia.com>
+
+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 000..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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, , 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);

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

[Mikko Perttunen]

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 
---
  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 000..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 
+
+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 000..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+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, , 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[] = {
+    _dev_attr_rpm.dev_attr.attr,
+    NULL,
+};


"rpm" is not a standard hwmon sysfs attribute. If you don't provide
a single 

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

[Mikko Perttunen]

On 21.02.2018 16:46, Guenter Roeck wrote:

On 02/20/2018 11:15 PM, Mikko Perttunen wrote:

AIUI, the PWM framework already exposes a sysfs node with period
information. We should just use that instead of adding a new driver
for this.



I am kind of lost. Please explain.

Are you saying that we should drop the pwm-fan driver as well (which goes
the opposite way), as well as any other drivers doing anything with pwm
signals,
because after all those signals are already exposed to userspace a sysfs
attributes,
and a kernel driver to abstract those values is thus not needed ?


The only thing this driver does is do a constant division in 
kernelspace. I'm not really seeing why that couldn't be done in 
userspace. But if you think it's appropriate to do the RPM conversion in 
kernelspace then I'm not greatly opposed to that.





In any case, we cannot add something like this to device tree since
it's not a hardware device.



So you are saying there is no means to express in devicetree that
a pwm input is connected to a fan ? How is that not hardware ?

If so, how do you express in devicetree that a pwm signal is connected
to anything ?


If we want to describe that the tachometer is connected to a fan, then 
we should have a fan node in the board's device tree. We don't have a 
chip that has a thing called "generic-pwm-tachometer" attached to it. 
(We have chips that have a "nvidia,tegra186-tachometer", so it's proper 
to have that.)


Thanks,
Mikko



Guenter


Mikko

On 21.02.2018 08:58, 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 <rr...@nvidia.com>
---
 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 000..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 <rr...@nvidia.com>
+
+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 000..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