On 03/31/2017 04:23 PM, Jon Mason wrote:
On Fri, Mar 31, 2017 at 3:03 AM, Rafał Miłecki <zajec5@xxxxxxxxx> wrote:
On 03/24/2017 03:35 PM, Jon Mason wrote:
On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafał Miłecki wrote:
diff --git a/drivers/thermal/broadcom/ns-thermal.c
b/drivers/thermal/broadcom/ns-thermal.c
new file mode 100644
index 000000000000..acf3a4de62a4
--- /dev/null
+++ b/drivers/thermal/broadcom/ns-thermal.c
@@ -0,0 +1,98 @@
+/*
+ * Copyright (C) 2017 Rafał Miłecki <rafal@xxxxxxxxxx>
+ *
+ * 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.
+ */
+
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/thermal.h>
+
+#define PVTMON_CONTROL0 0x00
+#define PVTMON_CONTROL0_SEL_MASK 0x0000000e
+#define PVTMON_CONTROL0_SEL_TEMP_MONITOR 0x00000000
+#define PVTMON_CONTROL0_SEL_TEST_MODE 0x0000000e
+#define PVTMON_STATUS 0x08
+
+struct ns_thermal {
+ void __iomem *pvtmon;
+};
+
+static int ns_thermal_get_temp(void *data, int *temp)
+{
+ struct ns_thermal *ns_thermal = data;
+ u32 val;
+
+ val = readl(ns_thermal->pvtmon + PVTMON_CONTROL0);
+ if ((val & PVTMON_CONTROL0_SEL_MASK) !=
PVTMON_CONTROL0_SEL_TEMP_MONITOR) {
+ val &= ~PVTMON_CONTROL0_SEL_MASK;
+ val |= PVTMON_CONTROL0_SEL_TEMP_MONITOR;
I think this is slightly confusing here. If this was off, ORing in 0
will not enable it. I think we need to flip the #define to make it
PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE (or a less crappy name).
then use this line here to toggle it. Something like
val &= ~PVTMON_CONTROL0_SEL_TEMP_MONITOR_DISABLE;
I don't understand this, can you help me further?
OR-ing with 0 is only a cosmetic thing obviously - just to make it clear
which
value we expect to be set in bits 1:3. The important part is:
val &= ~PVTMON_CONTROL0_SEL_MASK;
You are using a side effect of the masking to clear/enable the block.
I'm simply saying that we should be explicit about enabling it. My
concern is that using the side effect hides what is being done and
could result in a bug somewhere down the line. I think this is
improbable based on the code, but wanted to err on the side of
caution.
Well, I'm clearing current mode selection and "selecting" the mode I want. By
OR-ing PVTMON_CONTROL0_SEL_TEMP_MONITOR I'm clearly indicating I want temp
monitor more.
How else I could make it more obvious? Should I add some comment maybe?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
