On 03/31/2017 04:23 PM, Jon Mason wrote:

On Fri, Mar 31, 2017 at 3:03 AM, Rafa? Mi?ecki [off-list ref] wrote:

quoted

On 03/24/2017 03:35 PM, Jon Mason wrote:

quoted

On Thu, Mar 23, 2017 at 11:30:45PM +0100, Rafa? Mi?ecki wrote:

quoted

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@milecki.pl>
+ *
+ * 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?