Received: by 10.223.185.116 with SMTP id b49csp1036673wrg; Wed, 21 Feb 2018 10:59:21 -0800 (PST) X-Google-Smtp-Source: AH8x226igAcOvhgT/oJt6jV3yE2/yZP9+fUcd+2dY0BJkC8iPzVz92u6ARK/wDGd5TJXUXbuEuEy X-Received: by 2002:a17:902:4601:: with SMTP id o1-v6mr4017195pld.210.1519239561703; Wed, 21 Feb 2018 10:59:21 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1519239561; cv=none; d=google.com; s=arc-20160816; b=M4uOgP376pmdX1E4o8XPaVcVAH+pZwcllvSC5M3d/o3QX75f7xhD6mZdtoluPBclza WQQMxj39iqlDNfsRT5WMNItTpe70oNoDXUtlkYO+NrpRBAakouphJJxMlJg5VaRU86Al c+WskAzAMgZqs7zfmG+nPJQbtqPtmkF8QXqxJU3tdfpEqbZN9XrVSHm+eTVSJsKntKg/ 9cSjEGgyEDlnctmz9eROVbPxjZqYyGXFDIqJG3TKgowPKwPfCmOiObMrXrahLrqF7Pzy O/BCxzpEBSs7wB3Hl6YkevqzBr71IglRdhR6EpR9BRFWhGR8EBfvVOeAXp2NK3CuboIL HX8w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature:arc-authentication-results; bh=lNqMDpmngM4y37dR6QGmVlhjZeuRm4dH3IHR/dlaLEQ=; b=k0ImlVSs1tLMQZIf/PyxiDFlRaoare+OQPkqqL+ycE2sOXiBKOZiCjpcm5KP1sNdhW SFDQ256lmkrZCgWV2PNJEeNJjf0mxltIJ+RWhTD5zrUGOnv3JCv73VqB1X27AfLO5Lnh 9yAaHkL4DR/7gwXHb0xGR/h44UX0KJYd2x2Zc51hYrjOS2sXNd0w5KyCBB97n7EcfPRe Bvz9WQ5ec1xyWb/G3pOcmPJ1F05BwSNRYYHzpy0JNGFp3iw07mg/cQXsVpKthgr5siVY n2EXm6cfjDQLXgDy37b7sxmRgNCHQdb4wL6On85Df/mmNe+396+Sp4YmZw9x4MkMt2cv CliQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=Hm7P5OCX; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 1-v6si279004ple.726.2018.02.21.10.59.07; Wed, 21 Feb 2018 10:59:21 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=fail header.i=@gmail.com header.s=20161025 header.b=Hm7P5OCX; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938462AbeBUQIk (ORCPT + 99 others); Wed, 21 Feb 2018 11:08:40 -0500 Received: from mail-pf0-f195.google.com ([209.85.192.195]:44745 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753249AbeBUQIg (ORCPT ); Wed, 21 Feb 2018 11:08:36 -0500 Received: by mail-pf0-f195.google.com with SMTP id 17so820763pfw.11; Wed, 21 Feb 2018 08:08:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=lNqMDpmngM4y37dR6QGmVlhjZeuRm4dH3IHR/dlaLEQ=; b=Hm7P5OCXaWxnNDtJJ5Yde4ZxiM5bfiowwh7Zi/jIuEDx2eJziewR0s8FDNsGU6zgqR 9ZwvBMMmBMWacRbBeg4N+FXHK3s9GPnAGPTi3QkOVLuCABflp4mSxZWLmv0chN8/jh+c NexVweZ4zVVlPc02lt9hcaIK+IdSqUzcVC9SngiHFXMw3tM4fo2WQKw8+ZT8NSAnh7Sj jI4IGcHtTGBChxBYUPSYn3NEw6aeSZrbgwW74vTXkqXYeGGofOnayffA2o/+A4i8qsFu yhgMFUv8shC3RwyQBA2M6AE3Q5UvbKtOs8mlymtsJu0DLuG8J2MQ9DjJXQLxlZNeVSiK GHOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to:user-agent; bh=lNqMDpmngM4y37dR6QGmVlhjZeuRm4dH3IHR/dlaLEQ=; b=WcK9fJ4PQGWMitNFIgrXixw6KxgY0rw1EeryTzTN9aLWnR+wHiYAaw3dUsXpOmg0wd xNhQjTxeR8nteRqzxoROdPyoAmE4rOBPVc3o9AOmmYCC4LlJ7YsbtUyYcM3tZ4cezfF8 8YshK3exGAMTgljD+Qh66MAjgy9kExIVY0wfIfjlto1XIotXtxVUnztAB/2NblXV0jtY ujmqeJeWkYeVlB8r18XEyRqZ65dRkzBtXg1lrW3KNQnywUNYl+GTopDYM8ILtz6YSJsg Mcjx6JW/KB5+SqQ6fsQzf8+WxjBs3iacnmC/F3SVfZmUM71iuj8vxkDaoJRdbnXMjz7x 8Ckw== X-Gm-Message-State: APf1xPBvSvZVn6dyqJeLedxPyQ+RspInaP134Tqow1h4KhBvS4XXC3+7 aqjqq9O3a5DZWhTLd2e1kGw= X-Received: by 10.99.180.3 with SMTP id s3mr3102390pgf.258.1519229316054; Wed, 21 Feb 2018 08:08:36 -0800 (PST) Received: from localhost (108-223-40-66.lightspeed.sntcca.sbcglobal.net. [108.223.40.66]) by smtp.gmail.com with ESMTPSA id f74sm1887740pff.8.2018.02.21.08.08.34 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 21 Feb 2018 08:08:34 -0800 (PST) Date: Wed, 21 Feb 2018 08:08:33 -0800 From: Guenter Roeck To: Mikko Perttunen Cc: Rajkumar Rampelli , robh+dt@kernel.org, mark.rutland@arm.com, thierry.reding@gmail.com, jonathanh@nvidia.com, jdelvare@suse.com, corbet@lwn.net, catalin.marinas@arm.com, will.deacon@arm.com, kstewart@linuxfoundation.org, gregkh@linuxfoundation.org, pombredanne@nexb.com, mmaddireddy@nvidia.com, mperttunen@nvidia.com, arnd@arndb.de, gregory.clement@free-electrons.com, timur@codeaurora.org, andy.gross@linaro.org, xuwei5@hisilicon.com, elder@linaro.org, heiko@sntech.de, krzk@kernel.org, ard.biesheuvel@linaro.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-pwm@vger.kernel.org, linux-tegra@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, ldewangan@nvidia.com Subject: Re: [PATCH 05/10] hwmon: generic-pwm-tachometer: Add generic PWM based tachometer Message-ID: <20180221160833.GA8063@roeck-us.net> References: <1519196339-9377-1-git-send-email-rrajk@nvidia.com> <1519196339-9377-6-git-send-email-rrajk@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > > > >>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 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 > >>>+ > >>>+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 > >>>+#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, &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, > >>>+}; > >>>+ > >>>+ATTRIBUTE_GROUPS(pwm_tach); > >>>+ > >>>+static int pwm_tach_probe(struct platform_device *pdev) > >>>+{ > >>>+ struct pwm_hwmon_tach *ptt; > >>>+ int err; > >>>+ > >>>+ ptt = devm_kzalloc(&pdev->dev, sizeof(*ptt), GFP_KERNEL); > >>>+ if (!ptt) > >>>+ return -ENOMEM; > >>>+ > >>>+ ptt->dev = &pdev->dev; > >>>+ > >>>+ platform_set_drvdata(pdev, ptt); > >>>+ dev_set_drvdata(&pdev->dev, ptt); > >>>+ > >>>+ ptt->pwm = devm_of_pwm_get(&pdev->dev, pdev->dev.of_node, NULL); > >>>+ if (IS_ERR(ptt->pwm)) { > >>>+ err = PTR_ERR(ptt->pwm); > >>>+ dev_err(&pdev->dev, "Failed to get pwm handle, err: %d\n", > >>>+ err); > >>>+ return err; > >>>+ } > >>>+ > >>>+ ptt->hwmon = devm_hwmon_device_register_with_groups(&pdev->dev, > >>>+ "pwm_tach", ptt, pwm_tach_groups); > >>>+ if (IS_ERR(ptt->hwmon)) { > >>>+ err = PTR_ERR_OR_ZERO(ptt->hwmon); > >>>+ dev_err(&pdev->dev, "Failed to register hwmon device: %d\n", > >>>+ err); > >>>+ return err; > >>>+ } > >>>+ > >>>+ return 0; > >>>+} > >>>+ > >>>+static const struct of_device_id pwm_tach_of_match[] = { > >>>+ { .compatible = "generic-pwm-tachometer" }, > >>>+ {} > >>>+}; > >>>+MODULE_DEVICE_TABLE(of, pwm_tach_of_match); > >>>+ > >>>+static struct platform_driver pwm_tach_driver = { > >>>+ .driver = { > >>>+ .name = "generic-pwm-tachometer", > >>>+ .of_match_table = pwm_tach_of_match, > >>>+ }, > >>>+ .probe = pwm_tach_probe, > >>>+}; > >>>+ > >>>+module_platform_driver(pwm_tach_driver); > >>>+ > >>>+MODULE_DESCRIPTION("PWM based Generic Tachometer driver"); > >>>+MODULE_AUTHOR("Laxman Dewangan "); > >>>+MODULE_AUTHOR("Rajkumar Rampelli "); > >>>+MODULE_LICENSE("GPL v2"); > >>> > >> > >