Message-ID: <4a43f993-6b58-8303-466b-a9115ed26fd6@megous.com>
Date:	Sat, 25 Jun 2016 17:12:41 +0200
From:	Ondřej Jirman <megous@...ous.com>
To:	Maxime Ripard <maxime.ripard@...e-electrons.com>
Cc:	dev@...ux-sunxi.org, linux-arm-kernel@...ts.infradead.org,
	Zhang Rui <rui.zhang@...el.com>,
	Eduardo Valentin <edubezval@...il.com>,
	Chen-Yu Tsai <wens@...e.org>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:THERMAL" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v2 02/14] thermal: sun8i_ths: Add support for the thermal
 sensor on Allwinner H3



On 25.6.2016 09:10, Maxime Ripard wrote:
> On Sat, Jun 25, 2016 at 05:44:59AM +0200, megous@...ous.com wrote:
>> From: Ondrej Jirman <megous@...ous.com>
>>
>> This patch adds support for the sun8i thermal sensor on
>> Allwinner H3 SoC.
>>
>> Signed-off-by: Ondřej Jirman <megous@...ous.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@....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@...ous.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
> 

Powered by blists - more mailing lists