Skip to contentSkip to conversation

thermal: Add Raspberry Pi BCM2835 thermal driver

PATCH New Lubomir Rintel 3 participants 3 messages Oct 11, 2015 – Oct 27, 2015

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 thermal: Add Raspberry Pi BCM2835 thermal driver

BCM2835 thermal sensor accessible via Raspberry Pi VideoCore 4 firmware interface.

Based on work by Craig McGeachie, ported to the Raspberry Pi firmware interface (from the original mailbox client version).

Cc Stephen Warren <[email protected]>
Cc Lee Jones <[email protected]>
Cc Eric Anholt <[email protected]>
Cc linux-rpi-kernel at lists.infradead.org
Cc linux-arm-kernel at lists.infradead.org

---
Needs the RPi firmware patchset from branch 'rpi-firmware' of
https://github.com/anholt/linux

 drivers/thermal/Kconfig           |   8 ++
 drivers/thermal/Makefile          |   1 +
 drivers/thermal/bcm2835-thermal.c | 161 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 170 insertions(+)
 create mode 100644 drivers/thermal/bcm2835-thermal.c
drivers/thermal/Kconfig +8 -0
index 5aabc4b..5305a95 100644
@@ -222,6 +222,14 @@ config DOVE_THERMAL
 	  Support for the Dove thermal sensor driver in the Linux thermal
 	  framework.
 
+config BCM2835_THERMAL
+	tristate "BCM2835 Temperature sensor on Raspberry Pi"
+	depends on RASPBERRYPI_FIRMWARE
+	help
+	  Support for the Broadcom BCM2835 thermal sensor driver on Raspberry Pi
+	  devices in the Linux thermal framework. The BCM2835 has one sensor on
+	  chip, with one trip point and no cooling devices.
+
 config DB8500_THERMAL
 	bool "DB8500 thermal management"
 	depends on ARCH_U8500
drivers/thermal/Makefile +1 -0
index 26f1608..e71182b 100644
@@ -45,3 +45,4 @@ obj-$(CONFIG_INTEL_PCH_THERMAL)	+= intel_pch_thermal.o
 obj-$(CONFIG_ST_THERMAL)	+= st/
 obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
 obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
+obj-$(CONFIG_BCM2835_THERMAL)	+= bcm2835-thermal.o
drivers/thermal/bcm2835-thermal.c new file +161 -0

161 lines not on this page · raw message

Signature · 1 line
2.4.3
#1 lore raw

PATCH 1/1 thermal: Add Raspberry Pi BCM2835 thermal driver

Quoted 7 lines · Lubomir Rintel

BCM2835 thermal sensor accessible via Raspberry Pi VideoCore 4 firmware interface.

Based on work by Craig McGeachie, ported to the Raspberry Pi firmware interface (from the original mailbox client version).

drivers/thermal/Kconfig config DOVE_THERMAL · line 231

Quoted 8 lines
drivers/thermal/Kconfig +7 -0
 
+config BCM2835_THERMAL
+	tristate "BCM2835 Temperature sensor on Raspberry Pi"
+	depends on RASPBERRYPI_FIRMWARE
+	help
+	  Support for the Broadcom BCM2835 thermal sensor driver on Raspberry Pi
+	  devices in the Linux thermal framework. The BCM2835 has one sensor on
+	  chip, with one trip point and no cooling devices.

That's about the minimum explanation that I'd expect to see in the DT binding too.

If someone were to implement the binding for another OS, they should be able to understand what to implement purely by reading the binding, without reference to another OS's driver.

drivers/thermal/bcm2835-thermal.c line 80

Quoted 9 lines
drivers/thermal/bcm2835-thermal.c +7 -0
 
+static int bcm2835_get_trip_type(struct thermal_zone_device *thermal_dev,
+			int trip_num, enum thermal_trip_type *trip_type)
+{
+	*trip_type = THERMAL_TRIP_HOT;
+
+	return 0;
+}

Shouldn't that validate trip_num, and return an error when it's not 0?

drivers/thermal/bcm2835-thermal.c line 149

Quoted 4 lines
drivers/thermal/bcm2835-thermal.c +4 -0
+static struct platform_driver bcm2835_thermal_driver = {
+	.driver = {
+		.name = "bcm2835_thermal",
+		.owner = THIS_MODULE,

I don't think .owner is required any more.

drivers/thermal/bcm2835-thermal.c line 156

Quoted 3 lines
drivers/thermal/bcm2835-thermal.c +3 -0
+};
+
+module_platform_driver(bcm2835_thermal_driver);

Nit: Typically/often you'd put the module_platform_driver() line right after the closing brace without a blank line between.

#2 lore raw

PATCH 1/1 thermal: Add Raspberry Pi BCM2835 thermal driver

In reply to Lubomir Rintel
Quoted 17 lines · Lubomir Rintel

BCM2835 thermal sensor accessible via Raspberry Pi VideoCore 4 firmware interface.

Based on work by Craig McGeachie, ported to the Raspberry Pi firmware interface (from the original mailbox client version).

Cc Stephen Warren <[email protected]>
Cc Lee Jones <[email protected]>
Cc Eric Anholt <[email protected]>
Cc linux-rpi-kernel at lists.infradead.org
Cc linux-arm-kernel at lists.infradead.org

---
Needs the RPi firmware patchset from branch 'rpi-firmware' of
https://github.com/anholt/linux

Quoted 5 lines
 drivers/thermal/Kconfig           |   8 ++
 drivers/thermal/Makefile          |   1 +
 drivers/thermal/bcm2835-thermal.c | 161 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 170 insertions(+)
 create mode 100644 drivers/thermal/bcm2835-thermal.c

Driver looks pretty simple, so not too much to go wrong.

Some small nits below though.

Quoted 47 lines
drivers/thermal/Kconfig +8 -0
index 5aabc4b..5305a95 100644
@@ -222,6 +222,14 @@ config DOVE_THERMAL
 	  Support for the Dove thermal sensor driver in the Linux thermal
 	  framework.
 
+config BCM2835_THERMAL
+	tristate "BCM2835 Temperature sensor on Raspberry Pi"
+	depends on RASPBERRYPI_FIRMWARE
+	help
+	  Support for the Broadcom BCM2835 thermal sensor driver on Raspberry Pi
+	  devices in the Linux thermal framework. The BCM2835 has one sensor on
+	  chip, with one trip point and no cooling devices.
+
 config DB8500_THERMAL
 	bool "DB8500 thermal management"
 	depends on ARCH_U8500
drivers/thermal/Makefile +1 -0
index 26f1608..e71182b 100644
@@ -45,3 +45,4 @@ obj-$(CONFIG_INTEL_PCH_THERMAL)	+= intel_pch_thermal.o
 obj-$(CONFIG_ST_THERMAL)	+= st/
 obj-$(CONFIG_TEGRA_SOCTHERM)	+= tegra_soctherm.o
 obj-$(CONFIG_HISI_THERMAL)     += hisi_thermal.o
+obj-$(CONFIG_BCM2835_THERMAL)	+= bcm2835-thermal.o
drivers/thermal/bcm2835-thermal.c new file +13 -0
new file mode 100644
index 0000000..7989c60
@@ -0,0 +1,161 @@
+/*
+ *  Copyright (C) 2013 Craig McGeachie
+ *  Copyright (C) 2014,2015 Lubomir Rintel
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This device presents the BCM2835 SoC temperature sensor as a thermal
+ * device.
+ */
+
+#include <linux/kernel.h>

drivers/thermal/bcm2835-thermal.c line 23

Quoted 10 lines
drivers/thermal/bcm2835-thermal.c new file +10 -0
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/thermal.h>
+#include <linux/dma-mapping.h>
+#include <linux/mailbox_client.h>
+#include <linux/mutex.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>

Nit: Alphabetical

drivers/thermal/bcm2835-thermal.c line 27

Quoted 3 lines
drivers/thermal/bcm2835-thermal.c new file +3 -0
+#define VC_TAG_GET_TEMP 0x00030006
+#define VC_TAG_GET_MAX_TEMP 0x0003000A
+#define VC_SUCCESS 0x80000000

Tabs, not spaces.

drivers/thermal/bcm2835-thermal.c line 38

Quoted 10 lines
drivers/thermal/bcm2835-thermal.c new file +10 -0
+struct prop {
+	u32 id;
+	u32 val;
+} __packed;
+
+struct bcm2835_therm {
+	struct device *dev;
+	struct thermal_zone_device *thermal_dev;
+	struct rpi_firmware *fw;
+};

Consider adding a Kernel doc?

Tabs.

drivers/thermal/bcm2835-thermal.c line 41

Quoted 2 lines
drivers/thermal/bcm2835-thermal.c new file +2 -0
+static int bcm2835_get_temp_common(struct thermal_zone_device *thermal_dev,
+						int *temp, u32 temp_type)

What kind of tabbing is this?

Lining up to the '(' is my personal preference.

drivers/thermal/bcm2835-thermal.c line 54

Quoted 13 lines
drivers/thermal/bcm2835-thermal.c new file +13 -0
+{
+	struct bcm2835_therm *therm = thermal_dev->devdata;
+	struct device *dev = therm->dev;
+	struct prop msg = {
+		.id = 0,
+		.val = 0
+	};
+	int ret;
+
+	ret = rpi_firmware_property(therm->fw, temp_type, &msg, sizeof(msg));
+	if (ret) {
+		dev_err(dev, "VC temperature request failed\n");
+		goto exit;

Don't do this. Just return.

Quoted 5 lines
drivers/thermal/bcm2835-thermal.c new file +5 -0
+	}
+
+	*temp = msg.val;
+
+exit:

drivers/thermal/bcm2835-thermal.c line 69

Quoted 10 lines
drivers/thermal/bcm2835-thermal.c new file +10 -0
+	return ret;
+}
+
+static int bcm2835_get_temp(struct thermal_zone_device *thermal_dev, int *temp)
+{
+	return bcm2835_get_temp_common(thermal_dev, temp, VC_TAG_GET_TEMP);
+}
+
+static int bcm2835_get_max_temp(struct thermal_zone_device *thermal_dev,
+						int trip_num, int *temp)

Tabbing.

drivers/thermal/bcm2835-thermal.c line 75

Quoted 6 lines
drivers/thermal/bcm2835-thermal.c new file +6 -0
+{
+	return bcm2835_get_temp_common(thermal_dev, temp, VC_TAG_GET_MAX_TEMP);
+}
+
+static int bcm2835_get_trip_type(struct thermal_zone_device *thermal_dev,
+			int trip_num, enum thermal_trip_type *trip_type)

Tabbing, etc.

drivers/thermal/bcm2835-thermal.c line 88

Quoted 13 lines
drivers/thermal/bcm2835-thermal.c new file +13 -0
+{
+	*trip_type = THERMAL_TRIP_HOT;
+
+	return 0;
+}
+
+static int bcm2835_get_mode(struct thermal_zone_device *thermal_dev,
+		enum thermal_device_mode *dev_mode)
+{
+	*dev_mode = THERMAL_DEVICE_ENABLED;
+
+	return 0;
+}

Not your fault, but these call-backs seem pretty pointless.

Quoted 15 lines
drivers/thermal/bcm2835-thermal.c new file +15 -0
+static struct thermal_zone_device_ops ops  = {
+	.get_temp = bcm2835_get_temp,
+	.get_trip_temp = bcm2835_get_max_temp,
+	.get_trip_type = bcm2835_get_trip_type,
+	.get_mode = bcm2835_get_mode,
+};
+
+static int bcm2835_thermal_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bcm2835_therm *therm;
+	struct device_node *fw;
+
+	therm = devm_kzalloc(dev, sizeof(*therm), GFP_KERNEL);
+	if (!therm)

drivers/thermal/bcm2835-thermal.c line 114

Quoted 10 lines
drivers/thermal/bcm2835-thermal.c new file +10 -0
+		return -ENOMEM;
+
+	therm->dev = dev;
+	dev_set_drvdata(dev, therm);
+
+	fw = of_parse_phandle(pdev->dev.of_node, "firmware", 0);
+	if (!fw) {
+		dev_err(dev, "no firmware node");
+		return -ENODEV;
+	}

'\n' here.

drivers/thermal/bcm2835-thermal.c line 126

Quoted 12 lines
drivers/thermal/bcm2835-thermal.c new file +12 -0
+	therm->fw = rpi_firmware_get(fw);
+	if (!therm->fw)
+		return -EPROBE_DEFER;
+
+	therm->thermal_dev = thermal_zone_device_register("bcm2835_thermal",
+					1, 0, therm, &ops, NULL, 0, 0);
+	if (IS_ERR(therm->thermal_dev)) {
+		dev_err(dev, "Unable to register the thermal device");
+		return PTR_ERR(therm->thermal_dev);
+	}
+
+	dev_info(dev, "Broadcom BCM2835 thermal sensor\n");

It's best practice not to print static information.

Quoted 12 lines
drivers/thermal/bcm2835-thermal.c new file +12 -0
+	return 0;
+}
+
+static int bcm2835_thermal_remove(struct platform_device *pdev)
+{
+	struct bcm2835_therm *therm = dev_get_drvdata(&pdev->dev);
+
+	thermal_zone_device_unregister(therm->thermal_dev);
+
+	return 0;
+}
+

drivers/thermal/bcm2835-thermal.c line 149

Quoted 10 lines
drivers/thermal/bcm2835-thermal.c new file +10 -0
+static const struct of_device_id bcm2835_thermal_of_match[] = {
+	{ .compatible = "raspberrypi,bcm2835-thermal", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, bcm2835_thermal_of_match);
+
+static struct platform_driver bcm2835_thermal_driver = {
+	.driver = {
+		.name = "bcm2835_thermal",
+		.owner = THIS_MODULE,

Remove this. It's handled else where.

drivers/thermal/bcm2835-thermal.c line 159

Quoted 10 lines
drivers/thermal/bcm2835-thermal.c new file +10 -0
+		.of_match_table = bcm2835_thermal_of_match,
+	},
+	.probe = bcm2835_thermal_probe,
+	.remove = bcm2835_thermal_remove,
+};
+
+module_platform_driver(bcm2835_thermal_driver);
+
+MODULE_AUTHOR("Craig McGeachie");
+MODULE_AUTHOR("Lubomir Rintel");

It's common practice to also place your email address in here.

Quoted 2 lines
drivers/thermal/bcm2835-thermal.c new file +2 -0
+MODULE_DESCRIPTION("Raspberry Pi BCM2835 thermal driver");
+MODULE_LICENSE("GPL v2");