Re: [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal sensor on Allwinner H3

[Maxime Ripard]

On Sat, Jun 25, 2016 at 05:44:59AM +0200, megous@xxxxxxxxxx wrote:
> From: Ondrej Jirman <megous@xxxxxxxxxx>
> 
> This patch adds support for the sun8i thermal sensor on
> Allwinner H3 SoC.
> 
> Signed-off-by: OndÅej Jirman <megous@xxxxxxxxxx>
> ---
> 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@xxxxxxxx>
> + *
> + * 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@xxxxxxxxxx>");
> +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: signature.asc
Description: PGP signature