diff mbox

[1/2] thermal: Add support for thermal sensor for Kirkwood SoC

Message ID 1354922151-3250-1-git-send-email-iwamatsu@nigauri.org (mailing list archive)
State New, archived
Headers show

Commit Message

Nobuhiro Iwamatsu Dec. 7, 2012, 11:15 p.m. UTC
Some kirkwood SoC has thermal sensor.
This patch adds support for 88F6282 and 88F6283.

Signed-off-by: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
---
 drivers/thermal/Kconfig            |    7 ++
 drivers/thermal/Makefile           |    1 +
 drivers/thermal/kirkwood_thermal.c |  131 ++++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+)
 create mode 100644 drivers/thermal/kirkwood_thermal.c

Comments

Jason Gunthorpe Dec. 7, 2012, 11:24 p.m. UTC | #1
On Sat, Dec 08, 2012 at 08:15:50AM +0900, Nobuhiro Iwamatsu wrote:
> Some kirkwood SoC has thermal sensor.
> This patch adds support for 88F6282 and 88F6283.

Thanks! I was just about to write this.. Looks good here.

Tested-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>

Jason
Jason Gunthorpe Dec. 7, 2012, 11:59 p.m. UTC | #2
On Fri, Dec 07, 2012 at 04:24:59PM -0700, Jason Gunthorpe wrote:
> On Sat, Dec 08, 2012 at 08:15:50AM +0900, Nobuhiro Iwamatsu wrote:
> > Some kirkwood SoC has thermal sensor.
> > This patch adds support for 88F6282 and 88F6283.
> 
> Thanks! I was just about to write this.. Looks good here.

Ah, looking closer:

$ cat /sys/class/thermal/thermal_zone0/temp 
37

That should be 37000, the value out of the driver should be in
milli-Celsius.

I'd use this equation instead:

	*temp = ((322 - reg) * 10000 * 1000) / 13625;

Regards,
Jason
Thomas Petazzoni Dec. 8, 2012, 12:07 a.m. UTC | #3
Hello,

On Sat,  8 Dec 2012 08:15:50 +0900, Nobuhiro Iwamatsu wrote:

> +config KIRKWOOD_THERMAL
> +	tristate "Temperature sensor on Marvel Kirkwood"

Marvell

> +	/* Valid check */
> +	if (!(reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK) {
> +		dev_info(&thermal->device, "Reading temperature is not valid\n");

"Temperature read is not valid" maybe? An native english speaker could
say.

> +	thermal_dev
> +		= devm_kzalloc(&pdev->dev, sizeof(*thermal_dev), GFP_KERNEL);

I think the usual coding style is to have the = on the first line.

> +static int kirkwood_thermal_exit(struct platform_device *pdev)
> +{
> +	struct thermal_zone_device *kirkwood_thermal
> +			= platform_get_drvdata(pdev);

Ditto.

> +static const struct of_device_id kirkwood_thermal_id_table[] = {
> +	{ .compatible = "marvel,thermal-kirkwood" },

marvel -> marvell

Also, I think it should be marvell,kirkwood-thermal, since most other
DT compatible strings that we have for Marvell SoCs are
marvell,<soc>-<function>.

Also, the Device Tree binding documentation is missing (even though it
is admittedly going to be a very short documentation).

Thanks!

Thomas
Russell King - ARM Linux Dec. 8, 2012, 12:08 a.m. UTC | #4
On Fri, Dec 07, 2012 at 04:59:04PM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 07, 2012 at 04:24:59PM -0700, Jason Gunthorpe wrote:
> > On Sat, Dec 08, 2012 at 08:15:50AM +0900, Nobuhiro Iwamatsu wrote:
> > > Some kirkwood SoC has thermal sensor.
> > > This patch adds support for 88F6282 and 88F6283.
> > 
> > Thanks! I was just about to write this.. Looks good here.
> 
> Ah, looking closer:
> 
> $ cat /sys/class/thermal/thermal_zone0/temp 
> 37
> 
> That should be 37000, the value out of the driver should be in
> milli-Celsius.
> 
> I'd use this equation instead:
> 
> 	*temp = ((322 - reg) * 10000 * 1000) / 13625;

Be careful of math overflows... make sure you do this calculation using
unsigned arithmetic as temperatures above 157 degress will cause this
to look like a negative number using signed math... However, that
probably won't ever be noticed because at 157 degrees, you'll definitely
be outside the operating limits of the device.
Russell King - ARM Linux Dec. 8, 2012, 12:11 a.m. UTC | #5
On Sat, Dec 08, 2012 at 01:07:08AM +0100, Thomas Petazzoni wrote:
> Hello,
> 
> On Sat,  8 Dec 2012 08:15:50 +0900, Nobuhiro Iwamatsu wrote:
> > +static const struct of_device_id kirkwood_thermal_id_table[] = {
> > +	{ .compatible = "marvel,thermal-kirkwood" },
> 
> marvel -> marvell
> 
> Also, I think it should be marvell,kirkwood-thermal, since most other
> DT compatible strings that we have for Marvell SoCs are
> marvell,<soc>-<function>.
> 
> Also, the Device Tree binding documentation is missing (even though it
> is admittedly going to be a very short documentation).

Is this in any way compatible with the thermal monitoring found on
Dove (510) stuff?  If so, should it have the SoC prefix in there,
or should it be "armada-thermal" for the SoC family?
Sebastian Hesselbarth Dec. 9, 2012, 1:54 p.m. UTC | #6
On 12/08/2012 01:11 AM, Russell King - ARM Linux wrote:
> On Sat, Dec 08, 2012 at 01:07:08AM +0100, Thomas Petazzoni wrote:
>> Hello,
>>
>> On Sat,  8 Dec 2012 08:15:50 +0900, Nobuhiro Iwamatsu wrote:
>>> +static const struct of_device_id kirkwood_thermal_id_table[] = {
>>> +	{ .compatible = "marvel,thermal-kirkwood" },
>>
>> marvel ->  marvell
>>
>> Also, I think it should be marvell,kirkwood-thermal, since most other
>> DT compatible strings that we have for Marvell SoCs are
>> marvell,<soc>-<function>.
>>
>> Also, the Device Tree binding documentation is missing (even though it
>> is admittedly going to be a very short documentation).
>
> Is this in any way compatible with the thermal monitoring found on
> Dove (510) stuff?  If so, should it have the SoC prefix in there,
> or should it be "armada-thermal" for the SoC family?

I haven't checked the driver in detail but at least register offsets
and the register-to-temperature function are different for Dove.
This is no big deal and can be handled with compatible strings.

But more important, "kirkwood" includes 88f618x, 88f619x, and 88f6281
that have no thermal diode - at least it is not mentioned in the public
datasheet. So, finally for Nobuhiro's patch I suggest to have two
compatible strings, one for marvell,88f6282-thermal and one for
marvell,88f6282-thermal. Numbering scheme of Marvell SoCs is a mess..

For the driver, the name should be either orion_thermal.c (as we will
reuse it for Dove), or mvebu_thermal.c if there is also a thermal diode
on Armada 370/XP. Using "armada" alone is not a good idea, as it also
includes some pxa-based SoCs - naming scheme of Marvell SoCs is even
more broken than numbering scheme ;)

Sebastian
Nobuhiro Iwamatsu Dec. 14, 2012, 9:22 p.m. UTC | #7
On Sat, Dec 8, 2012 at 8:59 AM, Jason Gunthorpe
<jgunthorpe@obsidianresearch.com> wrote:
> On Fri, Dec 07, 2012 at 04:24:59PM -0700, Jason Gunthorpe wrote:
>> On Sat, Dec 08, 2012 at 08:15:50AM +0900, Nobuhiro Iwamatsu wrote:
>> > Some kirkwood SoC has thermal sensor.
>> > This patch adds support for 88F6282 and 88F6283.
>>
>> Thanks! I was just about to write this.. Looks good here.
>
> Ah, looking closer:
>
> $ cat /sys/class/thermal/thermal_zone0/temp
> 37
>
> That should be 37000, the value out of the driver should be in
> milli-Celsius.
>
> I'd use this equation instead:
>
>         *temp = ((322 - reg) * 10000 * 1000) / 13625;
>

OK, I will fix this.
Thanks for your review.

Best regards,
  Nobuhiro
Nobuhiro Iwamatsu Dec. 14, 2012, 9:24 p.m. UTC | #8
Hi,

On Sat, Dec 8, 2012 at 9:07 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Sat,  8 Dec 2012 08:15:50 +0900, Nobuhiro Iwamatsu wrote:
>
>> +config KIRKWOOD_THERMAL
>> +     tristate "Temperature sensor on Marvel Kirkwood"
>
> Marvell

Thanks, I will fix.
>
>> +     /* Valid check */
>> +     if (!(reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK) {
>> +             dev_info(&thermal->device, "Reading temperature is not valid\n");
>
> "Temperature read is not valid" maybe? An native english speaker could
> say.

I will fix.

>
>> +     thermal_dev
>> +             = devm_kzalloc(&pdev->dev, sizeof(*thermal_dev), GFP_KERNEL);
>
> I think the usual coding style is to have the = on the first line.
>
>> +static int kirkwood_thermal_exit(struct platform_device *pdev)
>> +{
>> +     struct thermal_zone_device *kirkwood_thermal
>> +                     = platform_get_drvdata(pdev);
>
> Ditto.
>

I will fix.

>> +static const struct of_device_id kirkwood_thermal_id_table[] = {
>> +     { .compatible = "marvel,thermal-kirkwood" },
>
> marvel -> marvell

Thanks, I will fix.

>
> Also, I think it should be marvell,kirkwood-thermal, since most other
> DT compatible strings that we have for Marvell SoCs are
> marvell,<soc>-<function>.
>
> Also, the Device Tree binding documentation is missing (even though it
> is admittedly going to be a very short documentation).
>

OK, I forgot this.
I will write documentation.

> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, Free Electrons
> Kernel, drivers, real-time and embedded Linux
> development, consulting, training and support.
> http://free-electrons.com

Best regards,
  Nobuhiro
Nobuhiro Iwamatsu Dec. 14, 2012, 9:25 p.m. UTC | #9
Hi,

On Sat, Dec 8, 2012 at 9:08 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Fri, Dec 07, 2012 at 04:59:04PM -0700, Jason Gunthorpe wrote:
>> On Fri, Dec 07, 2012 at 04:24:59PM -0700, Jason Gunthorpe wrote:
>> > On Sat, Dec 08, 2012 at 08:15:50AM +0900, Nobuhiro Iwamatsu wrote:
>> > > Some kirkwood SoC has thermal sensor.
>> > > This patch adds support for 88F6282 and 88F6283.
>> >
>> > Thanks! I was just about to write this.. Looks good here.
>>
>> Ah, looking closer:
>>
>> $ cat /sys/class/thermal/thermal_zone0/temp
>> 37
>>
>> That should be 37000, the value out of the driver should be in
>> milli-Celsius.
>>
>> I'd use this equation instead:
>>
>>       *temp = ((322 - reg) * 10000 * 1000) / 13625;
>
> Be careful of math overflows... make sure you do this calculation using
> unsigned arithmetic as temperatures above 157 degress will cause this
> to look like a negative number using signed math... However, that
> probably won't ever be noticed because at 157 degrees, you'll definitely
> be outside the operating limits of the device.

Oh, OK. I would like to thank you pointed out.
I will fix.

Best regards,
  Nobuhiro
Nobuhiro Iwamatsu Dec. 14, 2012, 9:31 p.m. UTC | #10
Hi,

On Sat, Dec 8, 2012 at 9:11 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Sat, Dec 08, 2012 at 01:07:08AM +0100, Thomas Petazzoni wrote:
>> Hello,
>>
>> On Sat,  8 Dec 2012 08:15:50 +0900, Nobuhiro Iwamatsu wrote:
>> > +static const struct of_device_id kirkwood_thermal_id_table[] = {
>> > +   { .compatible = "marvel,thermal-kirkwood" },
>>
>> marvel -> marvell
>>
>> Also, I think it should be marvell,kirkwood-thermal, since most other
>> DT compatible strings that we have for Marvell SoCs are
>> marvell,<soc>-<function>.
>>
>> Also, the Device Tree binding documentation is missing (even though it
>> is admittedly going to be a very short documentation).
>
> Is this in any way compatible with the thermal monitoring found on
> Dove (510) stuff?  If so, should it have the SoC prefix in there,
> or should it be "armada-thermal" for the SoC family?

I have document of armada and kirkwood only.
I dont know about Dove. if Dove has same device, we had better change
it is a good idea.

Best regards,
  Nobuhiro
diff mbox

Patch

diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index e1cb6bd..8710ac2 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -55,3 +55,10 @@  config EXYNOS_THERMAL
 	help
 	  If you say yes here you get support for TMU (Thermal Managment
 	  Unit) on SAMSUNG EXYNOS series of SoC.
+
+config KIRKWOOD_THERMAL
+	tristate "Temperature sensor on Marvel Kirkwood"
+	depends on ARCH_KIRKWOOD && THERMAL
+	help
+	  Support for the Kirkwood thermal sensor driver into the Linux thermal
+	  framework. This supports only 88F6282 and 88F6283.
diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile
index 885550d..4dbe5e1 100644
--- a/drivers/thermal/Makefile
+++ b/drivers/thermal/Makefile
@@ -6,4 +6,5 @@  obj-$(CONFIG_THERMAL)		+= thermal_sys.o
 obj-$(CONFIG_CPU_THERMAL)		+= cpu_cooling.o
 obj-$(CONFIG_SPEAR_THERMAL)		+= spear_thermal.o
 obj-$(CONFIG_RCAR_THERMAL)	+= rcar_thermal.o
+obj-$(CONFIG_KIRKWOOD_THERMAL)         += kirkwood_thermal.o
 obj-$(CONFIG_EXYNOS_THERMAL)		+= exynos_thermal.o
diff --git a/drivers/thermal/kirkwood_thermal.c b/drivers/thermal/kirkwood_thermal.c
new file mode 100644
index 0000000..bddcf49
--- /dev/null
+++ b/drivers/thermal/kirkwood_thermal.c
@@ -0,0 +1,131 @@ 
+/*
+ * kirkwood thermal sensor driver
+ *
+ * Copyright (C) 2012 Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
+ *
+ * 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/device.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define THERMAL_VALID_OFFSET	9
+#define THERMAL_VALID_MASK	0x1
+#define THERMAL_TEMP_OFFSET	10
+#define THERMAL_TEMP_MASK	0x1FF
+
+/* Kirkwood Thermal Sensor Dev Structure */
+struct kirkwood_thermal_dev {
+	void __iomem *base_addr;
+};
+
+static inline int kirkwood_get_temp(struct thermal_zone_device *thermal,
+			unsigned long *temp)
+{
+	unsigned long reg;
+	struct kirkwood_thermal_dev *thermal_dev = thermal->devdata;
+
+	reg = readl_relaxed(thermal_dev->base_addr);
+
+	/* Valid check */
+	if (!(reg >> THERMAL_VALID_OFFSET) & THERMAL_VALID_MASK) {
+		dev_info(&thermal->device, "Reading temperature is not valid\n");
+		return -EIO;
+	}
+
+	reg = (reg >> THERMAL_TEMP_OFFSET) & THERMAL_TEMP_MASK;
+	/* Calculate temperature. See Table 814 in hardware manual. */
+	*temp = ((322 - reg) * 10000) / 13625;
+
+	return 0;
+}
+
+static struct thermal_zone_device_ops ops = {
+	.get_temp = kirkwood_get_temp,
+};
+
+static int kirkwood_thermal_probe(struct platform_device *pdev)
+{
+	struct thermal_zone_device *thermal = NULL;
+	struct kirkwood_thermal_dev *thermal_dev;
+	struct resource *res;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "Failed to get platform resource\n");
+		return -ENODEV;
+	}
+
+	thermal_dev
+		= devm_kzalloc(&pdev->dev, sizeof(*thermal_dev), GFP_KERNEL);
+	if (!thermal_dev) {
+		dev_err(&pdev->dev, "kzalloc fail\n");
+		return -ENOMEM;
+	}
+
+	thermal_dev->base_addr = devm_ioremap(&pdev->dev, res->start,
+				resource_size(res));
+	if (!thermal_dev->base_addr) {
+		dev_err(&pdev->dev, "Failed to ioremap memory\n");
+		return -ENOMEM;
+	}
+
+	thermal = thermal_zone_device_register("kirkwood_thermal", 0, 0,
+				   thermal_dev, &ops, 0, 0);
+	if (IS_ERR(thermal)) {
+		dev_err(&pdev->dev, "Failed to register thermal zone device\n");
+		return  PTR_ERR(thermal);
+	}
+
+	platform_set_drvdata(pdev, thermal);
+
+	dev_info(&thermal->device, KBUILD_MODNAME ": Thermal sensor registered\n");
+
+	return 0;
+}
+
+static int kirkwood_thermal_exit(struct platform_device *pdev)
+{
+	struct thermal_zone_device *kirkwood_thermal
+			= platform_get_drvdata(pdev);
+
+	thermal_zone_device_unregister(kirkwood_thermal);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id kirkwood_thermal_id_table[] = {
+	{ .compatible = "marvel,thermal-kirkwood" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, kirkwood_thermal_id_table);
+
+static struct platform_driver kirkwood_thermal_driver = {
+	.probe = kirkwood_thermal_probe,
+	.remove = kirkwood_thermal_exit,
+	.driver = {
+		.name = "kirkwood_thermal",
+		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(kirkwood_thermal_id_table),
+	},
+};
+
+module_platform_driver(kirkwood_thermal_driver);
+
+MODULE_AUTHOR("Nobuhiro Iwamatsu <iwamatsu@nigauri.org>");
+MODULE_DESCRIPTION("kirkwood thermal driver");
+MODULE_LICENSE("GPL");