diff mbox

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

Message ID 20160625034511.7966-3-megous@megous.com (mailing list archive)
State Changes Requested
Delegated to: Zhang Rui
Headers show

Commit Message

Ondřej Jirman June 25, 2016, 3:44 a.m. UTC
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

Comments

Maxime Ripard June 25, 2016, 7:10 a.m. UTC | #1
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
Ondřej Jirman June 25, 2016, 3:12 p.m. UTC | #2
On 25.6.2016 09:10, Maxime Ripard wrote:
> 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 ?!

Because it's overkill for reading a single word from memeory, the sunxi
nvmem driver doesn't support H3, the sid is not documented in the
datasheet, aside from some registger name/offset dump on the mailing
list some time ago.

Aside from that, I've yet to see H3 soc that has anything else than 0 in
the calibration data memory location. So this is basically nop.

Proposed solution seems simpler with no drawbacks that I can see,
without resorting to dropping the thing entirely from this driver. Which
I would be fine with too. Calibration is optional feature in the BSP
kernel, so I assume dropping it may not do too much harm either.

If anyone wants to implement sid in the future, it will be easy enough
to do with backwards compatibility. The second reg will become optional,
and the driver can check for nvmem.

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

Suspend/resume handling? I would have no way of testing it, other than
blindly impelementing what BSP kernel does. Other than that, I can add
it quite easily. It should be rather simple.

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

Good catch. I'll just move the above sun8i_ths_h3_init call below the
tzd initialization. That should fix it.

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

What driver?

Thank you very much for the review.

regards,
  Ondrej

> Thanks,
> Maxime
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime Ripard June 28, 2016, 11:39 a.m. UTC | #3
On Sat, Jun 25, 2016 at 05:12:41PM +0200, Ondřej Jirman wrote:
> >> +	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 ?!
> 
> Because it's overkill for reading a single word from memeory, the sunxi
> nvmem driver doesn't support H3, the sid is not documented in the
> datasheet, aside from some registger name/offset dump on the mailing
> list some time ago.
> 
> Aside from that, I've yet to see H3 soc that has anything else than 0 in
> the calibration data memory location. So this is basically nop.
> 
> Proposed solution seems simpler with no drawbacks that I can see,
> without resorting to dropping the thing entirely from this driver. Which
> I would be fine with too. Calibration is optional feature in the BSP
> kernel, so I assume dropping it may not do too much harm either.
> 
> If anyone wants to implement sid in the future, it will be easy enough
> to do with backwards compatibility. The second reg will become optional,
> and the driver can check for nvmem.

A lot of things in drivers boil down to "reading a single word from
memory". However, abstractions are here for a reason, and there's none
to not use it.

If that's not something we can use today, remove it entirely. And if
that becomes necessary, we will add an optional nvmem property.

> >> +
> >> +	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.
> 
> Suspend/resume handling? I would have no way of testing it, other than
> blindly impelementing what BSP kernel does. Other than that, I can add
> it quite easily. It should be rather simple.

No, I mean runtime_pm, with runtime_suspend and runtime_resume, to
allow only powering up the device when it's used, and shut it down
when not used.

> >> +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?
> 
> What driver?

drivers/input/touchscreen/sun4i-ts.c

Maxime
diff mbox

Patch

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;
+	}
+
+	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;
+	}
+
+	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;
+	}
+
+	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");