From: Maxime Ripard <maxime.ripard@free-electrons.com>
To: megous@megous.com
Cc: dev@linux-sunxi.org, linux-arm-kernel@lists.infradead.org,
Zhang Rui <rui.zhang@intel.com>,
Eduardo Valentin <edubezval@gmail.com>,
Chen-Yu Tsai <wens@csie.org>,
open list <linux-kernel@vger.kernel.org>,
"open list:THERMAL" <linux-pm@vger.kernel.org>
Subject: Re: [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3
Date: Sat, 25 Jun 2016 09:10:44 +0200 [thread overview]
Message-ID: <20160625071044.GB4000@lukather> (raw)
In-Reply-To: <20160625034511.7966-3-megous@megous.com>
[-- Attachment #1: Type: text/plain, Size: 11038 bytes --]
On Sat, Jun 25, 2016 at 05:44:59AM +0200, megous@megous.com wrote:
> From: Ondrej Jirman <megous@megous.com>
>
> This patch adds support for the sun8i thermal sensor on
> Allwinner H3 SoC.
>
> Signed-off-by: Ondřej Jirman <megous@megous.com>
> ---
> v2:
> - removed incorrect use of SID driver in sun8i_ths
> - read calibration data directly from iomem
> - better explanation for the thermal sensor driver
> - dt documentation fixes
> - dropped unncecessary macros and init code reorganization
> - moved resource aquisition from init to probe function
> - deassert reset after clock rate is set, not before
> - enable irq after all other registers are configured
> ---
> drivers/thermal/Kconfig | 7 ++
> drivers/thermal/Makefile | 1 +
> drivers/thermal/sun8i_ths.c | 260 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 268 insertions(+)
> create mode 100644 drivers/thermal/sun8i_ths.c
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 2d702ca..d3209d9 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -351,6 +351,13 @@ config MTK_THERMAL
> Enable this option if you want to have support for thermal management
> controller present in Mediatek SoCs
>
> +config SUN8I_THS
> + tristate "Thermal sensor driver for Allwinner H3"
> + depends on MACH_SUN8I
> + depends on OF
> + help
> + Enable this to support thermal reporting on some newer Allwinner SoCs.
> +
> menu "Texas Instruments thermal drivers"
> depends on ARCH_HAS_BANDGAP || COMPILE_TEST
> depends on HAS_IOMEM
> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
> index 10b07c1..7261ee8 100644
> --- a/drivers/thermal/Makefile
> +++ b/drivers/thermal/Makefile
> @@ -51,3 +51,4 @@ obj-$(CONFIG_TEGRA_SOCTHERM) += tegra/
> obj-$(CONFIG_HISI_THERMAL) += hisi_thermal.o
> obj-$(CONFIG_MTK_THERMAL) += mtk_thermal.o
> obj-$(CONFIG_GENERIC_ADC_THERMAL) += thermal-generic-adc.o
> +obj-$(CONFIG_SUN8I_THS) += sun8i_ths.o
> diff --git a/drivers/thermal/sun8i_ths.c b/drivers/thermal/sun8i_ths.c
> new file mode 100644
> index 0000000..9ba0f96
> --- /dev/null
> +++ b/drivers/thermal/sun8i_ths.c
> @@ -0,0 +1,260 @@
> +/*
> + * Thermal sensor driver for Allwinner H3 SoC
> + *
> + * Copyright (C) 2016 Ondřej Jirman
> + * Based on the work of Josef Gajdusek <atx@atx.name>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that 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/clk.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/reset.h>
> +#include <linux/slab.h>
> +#include <linux/thermal.h>
> +#include <linux/printk.h>
> +
> +#define THS_H3_CTRL0 0x00
> +#define THS_H3_CTRL2 0x40
> +#define THS_H3_INT_CTRL 0x44
> +#define THS_H3_STAT 0x48
> +#define THS_H3_FILTER 0x70
> +#define THS_H3_CDATA 0x74
> +#define THS_H3_DATA 0x80
> +
> +#define THS_H3_CTRL0_SENSOR_ACQ0(x) (x)
> +#define THS_H3_CTRL2_SENSE_EN BIT(0)
> +#define THS_H3_CTRL2_SENSOR_ACQ1(x) ((x) << 16)
> +#define THS_H3_INT_CTRL_DATA_IRQ_EN BIT(8)
> +#define THS_H3_INT_CTRL_THERMAL_PER(x) ((x) << 12)
> +#define THS_H3_STAT_DATA_IRQ_STS BIT(8)
> +#define THS_H3_FILTER_TYPE(x) ((x) << 0)
> +#define THS_H3_FILTER_EN BIT(2)
> +
> +#define THS_H3_CLK_IN 40000000 /* Hz */
> +#define THS_H3_DATA_PERIOD 330 /* ms */
> +
> +#define THS_H3_FILTER_TYPE_VALUE 2 /* average over 2^(n+1) samples */
> +#define THS_H3_FILTER_DIV (1 << (THS_H3_FILTER_TYPE_VALUE + 1))
> +#define THS_H3_INT_CTRL_THERMAL_PER_VALUE \
> + (THS_H3_DATA_PERIOD * (THS_H3_CLK_IN / 1000) / THS_H3_FILTER_DIV / 4096 - 1)
> +#define THS_H3_CTRL0_SENSOR_ACQ0_VALUE 0x3f /* 16us */
> +#define THS_H3_CTRL2_SENSOR_ACQ1_VALUE 0x3f
> +
> +struct sun8i_ths_data {
> + struct reset_control *reset;
> + struct clk *clk;
> + struct clk *busclk;
> + void __iomem *regs;
> + void __iomem *calreg;
> + struct thermal_zone_device *tzd;
> + u32 temp;
> +};
> +
> +static int sun8i_ths_get_temp(void *_data, int *out)
> +{
> + struct sun8i_ths_data *data = _data;
> +
> + if (data->temp == 0)
> + return -EINVAL;
> +
> + /* Formula and parameters from the Allwinner 3.4 kernel */
> + *out = 217000 - (int)((data->temp * 1000000) / 8253);
> + return 0;
> +}
> +
> +static irqreturn_t sun8i_ths_irq_thread(int irq, void *_data)
> +{
> + struct sun8i_ths_data *data = _data;
> +
> + writel(THS_H3_STAT_DATA_IRQ_STS, data->regs + THS_H3_STAT);
> +
> + data->temp = readl(data->regs + THS_H3_DATA);
> + if (data->temp)
> + thermal_zone_device_update(data->tzd);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void sun8i_ths_h3_init(struct sun8i_ths_data *data)
> +{
> + u32 caldata;
> +
> + caldata = readl(data->calreg) & 0xfff;
> + if (caldata != 0)
> + writel(caldata, data->regs + THS_H3_CDATA);
> +
> + writel(THS_H3_CTRL0_SENSOR_ACQ0(THS_H3_CTRL0_SENSOR_ACQ0_VALUE),
> + data->regs + THS_H3_CTRL0);
> + writel(THS_H3_FILTER_EN | THS_H3_FILTER_TYPE(THS_H3_FILTER_TYPE_VALUE),
> + data->regs + THS_H3_FILTER);
> + writel(THS_H3_CTRL2_SENSOR_ACQ1(THS_H3_CTRL2_SENSOR_ACQ1_VALUE) |
> + THS_H3_CTRL2_SENSE_EN,
> + data->regs + THS_H3_CTRL2);
> + writel(THS_H3_INT_CTRL_THERMAL_PER(THS_H3_INT_CTRL_THERMAL_PER_VALUE) |
> + THS_H3_INT_CTRL_DATA_IRQ_EN,
> + data->regs + THS_H3_INT_CTRL);
> +}
> +
> +static const struct thermal_zone_of_device_ops sun8i_ths_thermal_ops = {
> + .get_temp = sun8i_ths_get_temp,
> +};
> +
> +static int sun8i_ths_probe(struct platform_device *pdev)
> +{
> + struct sun8i_ths_data *data;
> + struct resource *res;
> + int ret;
> + int irq;
> +
> + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (!res) {
> + dev_err(&pdev->dev, "no memory resources defined\n");
> + return -EINVAL;
> + }
> +
> + data->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->regs)) {
> + ret = PTR_ERR(data->regs);
> + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret);
> + return ret;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + if (!res) {
> + dev_err(&pdev->dev, "no calibration data memory resource defined\n");
> + return -EINVAL;
> + }
> +
> + data->calreg = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(data->calreg)) {
> + ret = PTR_ERR(data->calreg);
> + dev_err(&pdev->dev, "failed to ioremap THS registers: %d\n", ret);
> + return ret;
> + }
Why did you remove the SID use through the nvmem framework ?!
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "failed to get IRQ: %d\n", irq);
> + return irq;
> + }
> +
> + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> + sun8i_ths_irq_thread, IRQF_ONESHOT,
> + dev_name(&pdev->dev), data);
> + if (ret)
> + return ret;
> +
> + data->busclk = devm_clk_get(&pdev->dev, "ahb");
> + if (IS_ERR(data->busclk)) {
> + ret = PTR_ERR(data->busclk);
> + dev_err(&pdev->dev, "failed to get ahb clk: %d\n", ret);
> + return ret;
> + }
> +
> + data->clk = devm_clk_get(&pdev->dev, "ths");
> + if (IS_ERR(data->clk)) {
> + ret = PTR_ERR(data->clk);
> + dev_err(&pdev->dev, "failed to get ths clk: %d\n", ret);
> + return ret;
> + }
> +
> + data->reset = devm_reset_control_get(&pdev->dev, "ahb");
> + if (IS_ERR(data->reset)) {
> + ret = PTR_ERR(data->reset);
> + dev_err(&pdev->dev, "failed to get reset: %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(data->busclk);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to enable bus clk: %d\n", ret);
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(data->clk);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to enable ths clk: %d\n", ret);
> + goto err_disable_bus;
> + }
> +
> + ret = clk_set_rate(data->clk, THS_H3_CLK_IN);
> + if (ret)
> + goto err_disable_ths;
> +
> + ret = reset_control_deassert(data->reset);
> + if (ret) {
> + dev_err(&pdev->dev, "reset deassert failed: %d\n", ret);
> + goto err_disable_ths;
> + }
Having runtime_pm support would be great.
> + sun8i_ths_h3_init(data);
> +
> + data->tzd = thermal_zone_of_sensor_register(&pdev->dev, 0, data,
> + &sun8i_ths_thermal_ops);
> + if (IS_ERR(data->tzd)) {
> + ret = PTR_ERR(data->tzd);
> + dev_err(&pdev->dev, "failed to register thermal zone: %d\n",
> + ret);
> + goto err_assert_reset;
> + }
You reference data->tzd in your interrupt handler, and the interrupts
have been activated before initializing that field. That is likely to
cause a kernel crash when you receive an interrupt between your
request_irq and that call.
> +
> + platform_set_drvdata(pdev, data);
> + return 0;
> +
> +err_assert_reset:
> + reset_control_assert(data->reset);
> +err_disable_ths:
> + clk_disable_unprepare(data->clk);
> +err_disable_bus:
> + clk_disable_unprepare(data->busclk);
> + return ret;
> +}
> +
> +static int sun8i_ths_remove(struct platform_device *pdev)
> +{
> + struct sun8i_ths_data *data = platform_get_drvdata(pdev);
> +
> + thermal_zone_of_sensor_unregister(&pdev->dev, data->tzd);
> + reset_control_assert(data->reset);
> + clk_disable_unprepare(data->clk);
> + clk_disable_unprepare(data->busclk);
> + return 0;
> +}
> +
> +static const struct of_device_id sun8i_ths_id_table[] = {
> + { .compatible = "allwinner,sun8i-h3-ths", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, sun8i_ths_id_table);
> +
> +static struct platform_driver sun8i_ths_driver = {
> + .probe = sun8i_ths_probe,
> + .remove = sun8i_ths_remove,
> + .driver = {
> + .name = "sun8i_ths",
> + .of_match_table = sun8i_ths_id_table,
> + },
> +};
> +
> +module_platform_driver(sun8i_ths_driver);
> +
> +MODULE_AUTHOR("Ondřej Jirman <megous@megous.com>");
> +MODULE_DESCRIPTION("Thermal sensor driver for Allwinner H3 SoC");
> +MODULE_LICENSE("GPL v2");
Looks quite good otherwise. It looks very similar to the older
touchscreen driver (without the touchscreen part).
Have you tried to merge the two?
Thanks,
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
next prev parent reply other threads:[~2016-06-25 7:10 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20160625034511.7966-1-megous@megous.com>
2016-06-25 3:44 ` [PATCH v2 01/14] ARM: clk: sunxi: Add driver for the H3 THS clock megous
2016-06-25 7:13 ` Maxime Ripard
2016-06-25 15:23 ` Ondřej Jirman
2016-06-28 11:52 ` Maxime Ripard
2016-06-25 3:44 ` [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3 megous
2016-06-25 7:10 ` Maxime Ripard [this message]
2016-06-25 15:12 ` Ondřej Jirman
2016-06-28 11:39 ` Maxime Ripard
2016-06-25 3:45 ` [PATCH v2 03/14] dt-bindings: document sun8i_ths - H3 thermal sensor driver megous
2016-06-28 20:56 ` Rob Herring
2016-06-25 3:45 ` [PATCH v2 04/14] regulator: SY8106A regulator driver megous
2016-06-26 11:26 ` Mark Brown
2016-06-26 15:07 ` Ondřej Jirman
2016-06-27 14:54 ` Mark Brown
2016-06-28 16:27 ` Ondřej Jirman
2016-06-27 15:10 ` Mark Brown
2016-06-25 3:45 ` [PATCH v2 05/14] dt-bindings: document " megous
2016-06-26 11:27 ` Mark Brown
2016-06-26 15:10 ` Ondřej Jirman
2016-06-26 18:52 ` Mark Brown
2016-06-25 3:45 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method megous
2016-06-30 20:40 ` Maxime Ripard
2016-07-01 0:50 ` Ondřej Jirman
2016-07-01 5:37 ` Jean-Francois Moine
2016-07-01 6:34 ` Ondřej Jirman
2016-07-01 7:47 ` Jean-Francois Moine
2016-07-15 8:53 ` Maxime Ripard
2016-07-15 10:38 ` Ondřej Jirman
2016-07-15 13:27 ` Jean-Francois Moine
2016-07-15 13:48 ` Ondřej Jirman
2016-07-15 14:22 ` [linux-sunxi] " Michal Suchanek
2016-07-15 16:33 ` Ondřej Jirman
2016-07-21 9:51 ` Maxime Ripard
2016-07-21 9:48 ` Maxime Ripard
2016-07-21 9:52 ` Ondřej Jirman
2016-07-26 6:32 ` Maxime Ripard
2016-07-28 11:27 ` Changed: sunxi-ng clock code - NKMP clock implementation is wrong Ondřej Jirman
2016-07-28 11:38 ` [linux-sunxi] " Chen-Yu Tsai
2016-07-28 21:00 ` Maxime Ripard
2016-07-28 22:01 ` Ondřej Jirman
2016-07-31 10:31 ` Maxime Ripard
2016-07-31 22:01 ` Ondřej Jirman
2016-08-31 20:25 ` Maxime Ripard
2016-07-01 0:53 ` [PATCH v2 06/14] ARM: sun8i: clk: Add clk-factor rate application method Ondřej Jirman
2016-07-15 8:19 ` Maxime Ripard
2016-06-25 3:45 ` [PATCH v2 07/14] ARM: dts: sun8i: Use sun8i-h3-pll1-clk for pll1 in H3 megous
2016-06-25 3:45 ` [PATCH v2 08/14] ARM: dts: sun8i: Add thermal sensor node to the sun8i-h3.dtsi megous
2016-06-25 3:45 ` [PATCH v2 09/14] ARM: dts: sun8i: Add cpu0 label to sun8i-h3.dtsi megous
2016-06-25 3:45 ` [PATCH v2 10/14] ARM: dts: sun8i: Add r_twi I2C controller megous
2016-06-25 7:16 ` Maxime Ripard
2016-06-25 3:45 ` [PATCH v2 11/14] ARM: dts: sun8i: Add sy8106a regulator to Orange Pi PC megous
2016-06-25 3:45 ` [PATCH v2 12/14] ARM: dts: sun8i: Setup CPU operating points for Onrage PI PC megous
2016-06-25 3:45 ` [PATCH v2 13/14] ARM: dts: sun8i: Add gpio-regulator used on Orange Pi One megous
2016-06-25 7:18 ` Maxime Ripard
2016-06-25 3:45 ` [PATCH v2 14/14] ARM: dts: sun8i: Enable DVFS " megous
2016-06-30 11:13 ` [linux-sunxi] " Michal Suchanek
2016-06-30 14:19 ` Ondřej Jirman
2016-06-30 15:16 ` Michal Suchanek
2016-06-30 15:32 ` Ondřej Jirman
2016-06-30 15:50 ` Michal Suchanek
2016-06-30 15:53 ` Ondřej Jirman
2016-07-01 10:54 ` Michal Suchanek
2016-06-30 14:23 ` Siarhei Siamashka
2016-07-01 1:17 ` Ondřej Jirman
2016-07-22 0:41 ` Ondřej Jirman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160625071044.GB4000@lukather \
--to=maxime.ripard@free-electrons.com \
--cc=dev@linux-sunxi.org \
--cc=edubezval@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=megous@megous.com \
--cc=rui.zhang@intel.com \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).