Hi Yinbo,
On 14/06/2023 10:03, zhuyinbo wrote:
Hi Daniel,
Thank you very much for your feedback and suggestions. Below, I have
some comments, please review.
[ ... ]
+
+ low += 100;
+ high += 100;
Literals -> macros
+ reg_ctrl = low;
+ reg_ctrl |= enable ? 0x100 : 0;
+ writew(reg_ctrl, data->regs + LOONGSON2_TSENSOR_CTRL_LO + reg_off);
+
+ reg_ctrl = high;
+ reg_ctrl |= enable ? 0x100 : 0;
+ writew(reg_ctrl, data->regs + LOONGSON2_TSENSOR_CTRL_HI + reg_off);
Is the 'enable' boolean really useful?
Yes, this 'enable' was to enable thermal irq.
Wouldn't be the sensor trip points disabled by default at reset time?
Only here will thermal irq be enabled throughout the entire driver, and
actual testing has shown that interrupts are valid, so this is
meaningful.
Ok.
If it is the case then we can get ride of this variable and make the
routine simpler
+ return 0;
+}
+
+static int loongson2_thermal_get_temp(struct thermal_zone_device
*tz, int *temp)
+{
+ u32 reg_val;
+ struct loongson2_thermal_data *data = tz->devdata;
+
+ reg_val = readl(data->regs + LOONGSON2_TSENSOR_OUT);
Seems like there is no offset for the sensor id here ?
There is no need for a sensor ID here.
There are some things that I didn't describe clearly, which made you
misunderstand. Actually, the temperature sensor of 2K1000 is like this:
There are 4 sets of temperature interrupt controllers, only one set of
temperature sampling registers. a sets of temperature interrupt
controllers was considered a sensor, which sensor include 3 register as
follows, where "SEL" represents which sensor is referenced, In 2k1000
datasheet, which "SEL" must be 0.
I'm not sure to understand. Let me rephrase it and know what is wrong.
1. The thermal controller has 4 sensors. The interrupt can be set for
these 4 sensors.
2. When reading a temperature, we have to select the sensor via the
'SEL' register.
3. The 2k1000 has one sensor with an id = 0.
4. In the future, more Loongson platform can be submitted with more than
one sensor
If this is correct, then my comments are about the inconsistency of the
proposed changes. Guessing in the future Loongson board there will be
more than one sensor, the existing code mixes support for one and
multiple sensors as well as assuming id is 0.
So if you add in the of_loongson2_thermal_match table a new platform
with several sensors, the current code will be broken because:
- the initialization loop does exit when the first thermal zone
registration succeed
- the interrupt handler does not figure out which sensor crossed the
low/high limit
- the get_temp is not selecting the right sensor
That is my point:
- write the code to support one sensor with id=0 only
*or*
- write the code to support multiple sensors
If I'm not wrong the code is closer to support multiple sensors ;)
Let me know if these deductions are correct
-- Daniel
ps : is there an English translation for the 2k1000 datasheet ?
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
