Hi Daniel,
在 2023/6/14 下午6:59, Daniel Lezcano 写道:
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
okay, I got it.
+ 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.
You can think it's actually a sensor, but the thermal sensor include a
set of status register and four sets of control register in loongson-2k
series (2k1000 /2k2000), but the control register only select a group
register by set "SEL".
2. When reading a temperature, we have to select the sensor via the
'SEL' register.
The 'SEL' was to select thermal control register set, that reading a
tempearure was to thermal status register set. thermal status register
set only one and no no other choice. The function of 'SEL' is actually
to select which temperature sensor collects the temperature as input.
3. The 2k1000 has one sensor with an id = 0.
Yes, the future 2k series product may still be a sensor, but the ID may
not be 0, it may be 1 or 2 or 3.
4. In the future, more Loongson platform can be submitted with more than
one sensor
From the current situation of the 2k series (2k1000/2k2000), it seems
that there will always be only one sensor, but 'SEL' can be used to
select which sensor. (0th, 1th, 2th, 3th). If there are really special
circumstances in the future, add appropriate modifications at that time.
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
Yes, but currently this code does not support the requirement for
multiple sensors.
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 ;)
In fact, my code is more inclined to support one sensor, but it can
determine which sensor to use based on the ID. (id = 0, 1, 2, 3)
Let me know if these deductions are correct
-- Daniel
ps : is there an English translation for the 2k1000 datasheet ?
Sorry, there seems to be no English version available at the moment.
Thanks,
Yinbo.
