Re: [PATCH v2 7/8] [PATCH 7/8] drivers/hwmon: Add a generic PECI hwmon client driver

[Miguel Ojeda]

On Thu, Feb 22, 2018 at 2:29 AM, Jae Hyun Yoo
 wrote:
> On 2/21/2018 4:37 PM, Andrew Lunn wrote:
>>>
>>> But even with this change, it still needs to use delayed creation
>>> because BMC side kernel doesn't know how many DIMMs are populated on
>>> a remote server before the remote server completes its memory
>>> training and testing in BIOS, but it needs to check the remote
>>> server's CPU temperature as immediate as possible to make
>>> appropriate thermal control based on the remote CPU's temperature to
>>> avoid any critical thermal issue. What would be a better solution in
>>> this case?
>>
>>
>> You could change this driver so that it supports one DIMM.  Move the
>> 'hotplug' part into another driver which creates and destroys
>> instances of the hwmon DIMM device as the DIMMS come and go.
>>
>> Also, do you need to handle CPU hotplug? You could split the CPU
>> temperature part into a separate hwmon driver? And again create and
>> destroy devices as CPUs come and go?
>>
>> Andrew
>>
>
> That seems like a possible option. I'll rewrite the hwmon driver again like
> that.
>
> Thanks for the good idea. :)

By the way, in the rewrite, please try to avoid the create*workqueue()
functions (they are deprecated :).

Cheers,
Miguel

>
> Jae
--
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

[PATCH v6 17/20] hwmon: (core) Add hwmon_max to hwmon_sensor_types enumeration

[Sudeep Holla]

It's useful to know the maximum types of sensor supported by hwmon
framework. It can be used to allocate some data structures when sorting
the monitors based on their type.

This will be used by scmi hwmon support.

Cc: linux-hwmon@vger.kernel.org
Acked-by: Guenter Roeck 
Signed-off-by: Sudeep Holla 
---
 include/linux/hwmon.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/hwmon.h b/include/linux/hwmon.h
index ceb751987c40..e5fd2707b6df 100644
--- a/include/linux/hwmon.h
+++ b/include/linux/hwmon.h
@@ -29,6 +29,7 @@ enum hwmon_sensor_types {
hwmon_humidity,
hwmon_fan,
hwmon_pwm,
+   hwmon_max,
 };
 
 enum hwmon_chip_attributes {
-- 
2.7.4

--
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

[PATCH v6 18/20] hwmon: add support for sensors exported via ARM SCMI

[Sudeep Holla]

Create a driver to add support for SoC sensors exported by the System
Control Processor (SCP) via the System Control and Management Interface
(SCMI). The supported sensor types is one of voltage, temperature,
current, and power.

The sensor labels and values provided by the SCP are exported via the
hwmon sysfs interface.

Cc: linux-hwmon@vger.kernel.org
Acked-by: Guenter Roeck 
Signed-off-by: Sudeep Holla 
---
 drivers/hwmon/Kconfig  |  12 +++
 drivers/hwmon/Makefile |   1 +
 drivers/hwmon/scmi-hwmon.c | 233 +
 3 files changed, 246 insertions(+)
 create mode 100644 drivers/hwmon/scmi-hwmon.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ef23553ff5cb..033e57366d56 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -317,6 +317,18 @@ config SENSORS_APPLESMC
  Say Y here if you have an applicable laptop and want to experience
  the awesome power of applesmc.
 
+config SENSORS_ARM_SCMI
+   tristate "ARM SCMI Sensors"
+   depends on ARM_SCMI_PROTOCOL
+   depends on THERMAL || !THERMAL_OF
+   help
+ This driver provides support for temperature, voltage, current
+ and power sensors available on SCMI based platforms. The actual
+ number and type of sensors exported depend on the platform.
+
+ This driver can also be built as a module.  If so, the module
+ will be called scmi-hwmon.
+
 config SENSORS_ARM_SCPI
tristate "ARM SCPI Sensors"
depends on ARM_SCPI_PROTOCOL
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index f814b4ace138..e7d52a36e6c4 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7462) += adt7462.o
 obj-$(CONFIG_SENSORS_ADT7470)  += adt7470.o
 obj-$(CONFIG_SENSORS_ADT7475)  += adt7475.o
 obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
+obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o
 obj-$(CONFIG_SENSORS_ARM_SCPI) += scpi-hwmon.o
 obj-$(CONFIG_SENSORS_ASC7621)  += asc7621.o
 obj-$(CONFIG_SENSORS_ASPEED)   += aspeed-pwm-tacho.o
diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
new file mode 100644
index ..7a0ab768d4b5
--- /dev/null
+++ b/drivers/hwmon/scmi-hwmon.c
@@ -0,0 +1,233 @@
+/*
+ * System Control and Management Interface(SCMI) based hwmon sensor driver
+ *
+ * Copyright (C) 2017 ARM Ltd.
+ * Sudeep Holla 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; 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 scmi_sensors {
+   const struct scmi_handle *handle;
+   const struct scmi_sensor_info **info[hwmon_max];
+};
+
+static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
+  u32 attr, int channel, long *val)
+{
+   int ret;
+   u64 value;
+   const struct scmi_sensor_info *sensor;
+   struct scmi_sensors *scmi_sensors = dev_get_drvdata(dev);
+   const struct scmi_handle *h = scmi_sensors->handle;
+
+   sensor = *(scmi_sensors->info[type] + channel);
+   ret = h->sensor_ops->reading_get(h, sensor->id, false, );
+   if (!ret)
+   *val = value;
+
+   return ret;
+}
+
+static int
+scmi_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type,
+  u32 attr, int channel, const char **str)
+{
+   const struct scmi_sensor_info *sensor;
+   struct scmi_sensors *scmi_sensors = dev_get_drvdata(dev);
+
+   sensor = *(scmi_sensors->info[type] + channel);
+   *str = sensor->name;
+
+   return 0;
+}
+
+static umode_t
+scmi_hwmon_is_visible(const void *drvdata, enum hwmon_sensor_types type,
+ u32 attr, int channel)
+{
+   const struct scmi_sensor_info *sensor;
+   const struct scmi_sensors *scmi_sensors = drvdata;
+
+   sensor = *(scmi_sensors->info[type] + channel);
+   if (sensor && sensor->name)
+   return S_IRUGO;
+
+   return 0;
+}
+
+static const struct hwmon_ops scmi_hwmon_ops = {
+   .is_visible = scmi_hwmon_is_visible,
+   .read = scmi_hwmon_read,
+   .read_string = scmi_hwmon_read_string,
+};
+
+static struct hwmon_chip_info scmi_chip_info = {
+   .ops = _hwmon_ops,
+   .info = NULL,
+};
+
+static int scmi_hwmon_add_chan_info(struct hwmon_channel_info *scmi_hwmon_chan,
+   struct device *dev, int num,
+   enum hwmon_sensor_types type, u32 config)
+{
+   int i;

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

[RAJKUMAR]

On Wednesday 21 February 2018 09:38 PM, Guenter Roeck wrote:

On Wed, Feb 21, 2018 at 05:20:29PM +0200, Mikko Perttunen wrote:

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.


Isn't that true for any conversion from and to pwm values ? Voltages,
for example ? Should those be handled in userspace as well ?

Note that I am not entirely convinced that the approach suggested in this
patch series makes sense. Patch 4 seems to move the notion of a tachometer
into the pwm subsystem. I am not really convinced that this makes sense
(maybe all that code should be in the hwmon driver instead, or in a thermal
driver if the author prefers). But that is a different issue. The question
you raised is if there should be any abstraction to or from raw pwm values
in the kernel.
Since tachometer driver implements PWM capture callback feature, I have 
added it under pwm driver as pwm-tegra-tachometer.c driver.
If the best approach is to have this tachometer driver under hwmon 
driver then I will move it to hwmon driver.



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.)


So you are concerned about the property name ? I don't really care how
it is called.

Guenter


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 
---
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