| Message ID | 1479818088-6007-4-git-send-email-wxt@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 22, 2016 at 08:34:46PM +0800, Caesar Wang wrote: > The temp_to_code function will return 0 when we set the trip points value > or valid temperature. I'm not quite sure what you mean by "when we set the trip points value or valid temperature." Do you mean "when we set the trip point's value to an invalid temperature"? Assuming that's what you meant... > This patch will prevent this case happening. This is good to change, but IMO, it's better to actually pick a close value, instead of the max. e.g., if you support temperatures at degree intervals of 80, 85, 90, ..., 125, but someone lists 82 in the device tree, we should pick either 80 or 85, not 125. Brian > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > --- > > drivers/thermal/rockchip_thermal.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c > index 766486f..535f1fa 100644 > --- a/drivers/thermal/rockchip_thermal.c > +++ b/drivers/thermal/rockchip_thermal.c > @@ -401,17 +401,15 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table, > int temp) > { > int high, low, mid; > - u32 error = 0; > + u32 error = table->data_mask; > > low = 0; > high = table->length - 1; > mid = (high + low) / 2; > > /* Return mask code data when the temp is over table range */ > - if (temp < table->id[low].temp || temp > table->id[high].temp) { > - error = table->data_mask; > + if (temp < table->id[low].temp || temp > table->id[high].temp) > goto exit; > - } > > while (low <= high) { > if (temp == table->id[mid].temp) > -- > 2.7.4 >
On Tue, Nov 22, 2016 at 12:57:37PM -0800, Brian Norris wrote: > On Tue, Nov 22, 2016 at 08:34:46PM +0800, Caesar Wang wrote: > > The temp_to_code function will return 0 when we set the trip points value > > or valid temperature. > > I'm not quite sure what you mean by "when we set the trip points value > or valid temperature." Do you mean "when we set the trip point's value > to an invalid temperature"? > > Assuming that's what you meant... > > > This patch will prevent this case happening. > > This is good to change, but IMO, it's better to actually pick a close > value, instead of the max. e.g., if you support temperatures at degree > intervals of 80, 85, 90, ..., 125, but someone lists 82 in the device > tree, we should pick either 80 or 85, not 125. I see that's what you're doing in a patch later in this series. Good. > > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > > --- > > > > drivers/thermal/rockchip_thermal.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c > > index 766486f..535f1fa 100644 > > --- a/drivers/thermal/rockchip_thermal.c > > +++ b/drivers/thermal/rockchip_thermal.c > > @@ -401,17 +401,15 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table, > > int temp) > > { > > int high, low, mid; > > - u32 error = 0; > > + u32 error = table->data_mask; > > > > low = 0; > > high = table->length - 1; > > mid = (high + low) / 2; > > > > /* Return mask code data when the temp is over table range */ > > - if (temp < table->id[low].temp || temp > table->id[high].temp) { > > - error = table->data_mask; > > + if (temp < table->id[low].temp || temp > table->id[high].temp) > > goto exit; I was revisiting the logic here though, and I don't understand your error case. You're treating "too low" and "too high" the same, and in either case, you're choosing a value of ->data_mask. That doesn't make sense to me, especially for ADC_DECREMENT cases like rk3288. In that case, you're programming the trip to the lowest possible temperature. It seems like either you should make this conditional, so that "too low" and "too high" make sane alternative choices (like MAX or MIN temp), or else restructure this to pass error codes back to the upper layers. Brian > > - } > > > > while (low <= high) { > > if (temp == table->id[mid].temp) > > -- > > 2.7.4 > >
在 2016年11月23日 05:52, Brian Norris 写道: > On Tue, Nov 22, 2016 at 12:57:37PM -0800, Brian Norris wrote: >> On Tue, Nov 22, 2016 at 08:34:46PM +0800, Caesar Wang wrote: >>> The temp_to_code function will return 0 when we set the trip points value >>> or valid temperature. >> I'm not quite sure what you mean by "when we set the trip points value >> or valid temperature." Do you mean "when we set the trip point's value >> to an invalid temperature"? >> >> Assuming that's what you meant... Almost, I will improve the commit. >> >>> This patch will prevent this case happening. >> This is good to change, but IMO, it's better to actually pick a close >> value, instead of the max. e.g., if you support temperatures at degree >> intervals of 80, 85, 90, ..., 125, but someone lists 82 in the device >> tree, we should pick either 80 or 85, not 125. > I see that's what you're doing in a patch later in this series. Good. Yeah, find another issue during writing this patch, as the patch[4/5]. > >>> Signed-off-by: Caesar Wang <wxt@rock-chips.com> >>> --- >>> >>> drivers/thermal/rockchip_thermal.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c >>> index 766486f..535f1fa 100644 >>> --- a/drivers/thermal/rockchip_thermal.c >>> +++ b/drivers/thermal/rockchip_thermal.c >>> @@ -401,17 +401,15 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table, >>> int temp) >>> { >>> int high, low, mid; >>> - u32 error = 0; >>> + u32 error = table->data_mask; >>> >>> low = 0; >>> high = table->length - 1; >>> mid = (high + low) / 2; >>> >>> /* Return mask code data when the temp is over table range */ >>> - if (temp < table->id[low].temp || temp > table->id[high].temp) { >>> - error = table->data_mask; >>> + if (temp < table->id[low].temp || temp > table->id[high].temp) >>> goto exit; > I was revisiting the logic here though, and I don't understand your > error case. You're treating "too low" and "too high" the same, and in > either case, you're choosing a value of ->data_mask. That doesn't make > sense to me, especially for ADC_DECREMENT cases like rk3288. In that > case, you're programming the trip to the lowest possible temperature. I admit that's not perfect, but that should conform to reality. Whichever is the adc value, 12it or 10bit. #define TSADCV2_DATA_MASK 0xfff #define TSADCV3_DATA_MASK 0x3ff The "too low" and "too high" are same, that should indicate that temperature is invalid or over table range. The currect code will return the max analog value to warn it. --- The temperature {-40C, 125C} is for rockchip SoCs, that should be similar with real world's temperature {-INT_MAX, INT_MAX}. -Caesar > > It seems like either you should make this conditional, so that "too low" > and "too high" make sane alternative choices (like MAX or MIN temp), or > else restructure this to pass error codes back to the upper layers. > > Brian > >>> - } >>> >>> while (low <= high) { >>> if (temp == table->id[mid].temp) >>> -- >>> 2.7.4 >>> > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Wed, Nov 23, 2016 at 10:06:15AM +0800, Caesar Wang wrote: > 在 2016年11月23日 05:52, Brian Norris 写道: > >On Tue, Nov 22, 2016 at 12:57:37PM -0800, Brian Norris wrote: > >>>+ if (temp < table->id[low].temp || temp > table->id[high].temp) > >>> goto exit; > >I was revisiting the logic here though, and I don't understand your > >error case. You're treating "too low" and "too high" the same, and in > >either case, you're choosing a value of ->data_mask. That doesn't make > >sense to me, especially for ADC_DECREMENT cases like rk3288. In that > >case, you're programming the trip to the lowest possible temperature. > > I admit that's not perfect, but that should conform to reality. > > Whichever is the adc value, 12it or 10bit. > #define TSADCV2_DATA_MASK 0xfff > #define TSADCV3_DATA_MASK 0x3ff > > The "too low" and "too high" are same, that should indicate that temperature is > invalid or over table range. > > The currect code will return the max analog value to warn it. > --- > > The temperature {-40C, 125C} is for rockchip SoCs, that should be > similar with real world's temperature {-INT_MAX, INT_MAX}. IIUC, "too high" should not be interpreted as TSADCV2_DATA_MASK on rk3288, should it? That corresponds to -40C, which means you'll be triggering the alarm temperature at a very *low* temperature, not a very high one, no? Brian
在 2016年11月23日 10:33, Brian Norris 写道: > On Wed, Nov 23, 2016 at 10:06:15AM +0800, Caesar Wang wrote: >> 在 2016年11月23日 05:52, Brian Norris 写道: >>> On Tue, Nov 22, 2016 at 12:57:37PM -0800, Brian Norris wrote: >>>>> + if (temp < table->id[low].temp || temp > table->id[high].temp) >>>>> goto exit; >>> I was revisiting the logic here though, and I don't understand your >>> error case. You're treating "too low" and "too high" the same, and in >>> either case, you're choosing a value of ->data_mask. That doesn't make >>> sense to me, especially for ADC_DECREMENT cases like rk3288. In that >>> case, you're programming the trip to the lowest possible temperature. >> I admit that's not perfect, but that should conform to reality. >> >> Whichever is the adc value, 12it or 10bit. >> #define TSADCV2_DATA_MASK 0xfff >> #define TSADCV3_DATA_MASK 0x3ff >> >> The "too low" and "too high" are same, that should indicate that temperature is >> invalid or over table range. >> >> The currect code will return the max analog value to warn it. >> --- >> >> The temperature {-40C, 125C} is for rockchip SoCs, that should be >> similar with real world's temperature {-INT_MAX, INT_MAX}. > IIUC, "too high" should not be interpreted as TSADCV2_DATA_MASK on > rk3288, should it? That corresponds to -40C, which means you'll be > triggering the alarm temperature at a very *low* temperature, not a very > high one, no? The "too high" will correspond to -40C on rk3288, but shouldn't trigger the alarm temperature. Due to the alarm or tshut function will handle it. e.g.: static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table, int chn, void __iomem *regs, int temp) { u32 alarm_value, int_en; /* Make sure the value is valid */ alarm_value = rk_tsadcv2_temp_to_code(table, temp); if (alarm_value == table->data_mask) return; .... } or static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table, int chn, void __iomem *regs, int temp) { u32 tshut_value, val; /* Make sure the value is valid */ tshut_value = rk_tsadcv2_temp_to_code(table, temp); if (tshut_value == table->data_mask) return; ... } > > Brian > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
On Wed, Nov 23, 2016 at 11:03:33AM +0800, Caesar Wang wrote: > 在 2016年11月23日 10:33, Brian Norris 写道: > >IIUC, "too high" should not be interpreted as TSADCV2_DATA_MASK on > >rk3288, should it? That corresponds to -40C, which means you'll be > >triggering the alarm temperature at a very *low* temperature, not a very > >high one, no? > > The "too high" will correspond to -40C on rk3288, but shouldn't > trigger the alarm temperature. > > Due to the alarm or tshut function will handle it. > > e.g.: > static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table, > int chn, void __iomem *regs, int temp) > { > u32 alarm_value, int_en; > > /* Make sure the value is valid */ > alarm_value = rk_tsadcv2_temp_to_code(table, temp); > if (alarm_value == table->data_mask) > return; Ah, right. I keep forgetting about this odd error handling. That's still the wrong error handling though; the right response is never to avoid doing anything (and therefore returning "success" to the thermal core). You need to either program a high (or low) trip value, or else report an error (i.e., allow rk_tsadcv2_alarm_temp() to return an error code back to the calling function). Otherwise, this: echo -45000 > trip_0_temp will succeed without error, and: cat trip_0_temp -45000 will return the cached temperature from of-thermal, even though the trip point is programmed to something else entirely. Brian
diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c index 766486f..535f1fa 100644 --- a/drivers/thermal/rockchip_thermal.c +++ b/drivers/thermal/rockchip_thermal.c @@ -401,17 +401,15 @@ static u32 rk_tsadcv2_temp_to_code(const struct chip_tsadc_table *table, int temp) { int high, low, mid; - u32 error = 0; + u32 error = table->data_mask; low = 0; high = table->length - 1; mid = (high + low) / 2; /* Return mask code data when the temp is over table range */ - if (temp < table->id[low].temp || temp > table->id[high].temp) { - error = table->data_mask; + if (temp < table->id[low].temp || temp > table->id[high].temp) goto exit; - } while (low <= high) { if (temp == table->id[mid].temp)
The temp_to_code function will return 0 when we set the trip points value or valid temperature. This patch will prevent this case happening. Signed-off-by: Caesar Wang <wxt@rock-chips.com> --- drivers/thermal/rockchip_thermal.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)