Hi Icenowy,

On 16/09/2017 12:14, icenowy-h8G6r0blFSE@public.gmane.org wrote:

在 2017-09-16 17:45，Quentin Schulz 写道：

quoted

Hi Icenowy,

On 14/09/2017 16:52, Icenowy Zheng wrote:

quoted

This adds support for the Allwinner H3 thermal sensor.

Allwinner H3 has a thermal sensor like the one in A33, but have its
registers nearly all re-arranged, sample clock moved to CCU and a pair
of bus clock and reset added. It's also the base of newer SoCs' thermal
sensors.

The thermal sensors on A64 and H5 is like the one on H3, but with of
course different formula factors.

Signed-off-by: Icenowy Zheng <redacted>
---
Changes in v4:
- Splitted out some code refactors.
- Code sequence changed back. (The gpadc_data went back to the start of
  the source file)

 drivers/iio/adc/sun4i-gpadc-iio.c | 48
+++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/sun4i-gpadc.h   | 27 ++++++++++++++++++++++
 2 files changed, 75 insertions(+)

[...]

quoted

quoted

 #define SUN4I_GPADC_CTRL3                0x0c
+/*
+ * This register is named "Average filter Control Register" in H3
Datasheet,
+ * but the register's definition is the same as the old CTRL3 register.
+ */
+#define SUN8I_H3_GPADC_CTRL3                0x70

I would name it as it is in the documentation:
SUN8I_H3_THS_FILTER

The definition of this register is the same as the CTRL3.

Maybe this name is better, but the similarity between them still needs
to be documented, as the SUN4I_GPADC_CTRL3_XXX macros will be used to
populate this register.

quoted

No need for comments then.

quoted

 #define SUN4I_GPADC_CTRL3_FILTER_EN            BIT(2)
 #define SUN4I_GPADC_CTRL3_FILTER_TYPE(x)        (GENMASK(1, 0) & (x))

They have _FILTER_ in their name, isn't it enough?

Just a matter of taste for me.

quoted

quoted

@@ -71,6 +84,13 @@

 #define SUN4I_GPADC_INT_FIFOC_TP_UP_IRQ_EN        BIT(1)
 #define SUN4I_GPADC_INT_FIFOC_TP_DOWN_IRQ_EN        BIT(0)

+#define SUN8I_H3_GPADC_INTC                0x44
+
+#define SUN8I_H3_GPADC_INTC_TEMP_PERIOD(x)        ((GENMASK(19, 0) &
(x)) << 12)
+#define SUN8I_H3_GPADC_INTC_TEMP_DATA            BIT(8)
+#define SUN8I_H3_GPADC_INTC_TEMP_SHUT            BIT(4)
+#define SUN8I_H3_GPADC_INTC_TEMP_ALARM            BIT(0)
+

Since it isn't an ADC anymore but rather just a THS, why don't you use
SUN8I_H3_THS instead of SUN8I_H3_GPADC? That way, it also matches the
datasheet.

quoted

 #define SUN4I_GPADC_INT_FIFOS                0x14

 #define SUN4I_GPADC_INT_FIFOS_TEMP_DATA_PENDING        BIT(18)

@@ -80,9 +100,16 @@

 #define SUN4I_GPADC_INT_FIFOS_TP_UP_PENDING        BIT(1)
 #define SUN4I_GPADC_INT_FIFOS_TP_DOWN_PENDING        BIT(0)

+#define SUN8I_H3_GPADC_INTS                0x44

0x48

[...]

1) You're not using irqs, why would you define registers that will never
be used?

I will then rework it to use IRQs, but not now.

Maybe I should add it when I use them?

Why not make it work right away the way we want :)?

quoted

2) Why aren't you using irqs? I remember we discussed on IRC that you
had some problems with the H3 when resuming or when probing the driver.
The register would have a zero in it until you have a first sample that
arrived (i.e. after the sample rate you set with T_ACQ) that would make
the thermal framework panic since the thermal sensor would return
something way too hot and shutdown your board?

Nope, it's another problem -- the runtime resume function is even not
called before the first sample, and the first sample will happen when
the THS is still suspended.

As discussed on IRC (a long time ago :) ), it's a combination of two
problems:
1) get_temp (used by thermal framework) uses pm_runtime function that
isn't ready yet <= I will send a patch for registering thermal framework
after pm_runtime to you, hopefully in the upcoming hour or tomorrow,

2) The A33 (and H3 in your implementation) does not wait for an
interrupt to read the TEMP_DATA register which resets to 0 when the
sensor is disabled (or until a first sample arrives) i.e. when probing
or when resuming. Using IRQs would get rid of 2). It isn't critical for
A33 as the formula for temp returns something really cold so thermal
does not care. But for the H3, it's critically hot and shuts down your
board. To make it work on the H3, we would have to use a delay in the
pm_resume function equal to the sensor sampling rate and I don't really
like that. If we could use IRQs, it'd be better IMHO (but they aren't
working on A33).

Thanks,
Quentin

quoted

The H3 apparently supports IRQs, why do you not support them for the
temperature? They might be broken as it is on A33 but then it might be a
good idea to write it down in a comment in the driver (and not adding
the unused registers in the header file) or at least in the commit log.

3) Now that you have support for clocks, wouldn't it be a good idea to
disable them during suspend?

Interesting... It's meaningful to disable the mod clock during suspend.

quoted

Thanks,
Quentin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Quentin Schulz, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.