Re: [PATCH v14 1/2] thermal: loongson-2: add thermal management support

[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

> > > +    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