| Message ID | 1480331524-18741-4-git-send-email-wxt@rock-chips.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 28, 2016 at 07:12:02PM +0800, Caesar Wang wrote: > The temp_to_code function will return 0 when we set the temperature to a > invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch > will prevent this case happening. That will return the max analog value to > indicate the temperature is invalid or over table temperature range. > > Signed-off-by: Caesar Wang <wxt@rock-chips.com> > --- > > Changes in v3: > - fix trivial thing for error message nd return value. > > Changes in v2: > - As Brian commnets that restructure this to pass error codes back to the > upper layers. > - Improve the commit message. > > Changes in v1: None > > drivers/thermal/rockchip_thermal.c | 48 ++++++++++++++++++++++---------------- > 1 file changed, 28 insertions(+), 20 deletions(-) Looks better now. Reviewed-by: Brian Norris <briannorris@chromium.org>
Hey Caesar, Brian, On Mon, Nov 28, 2016 at 07:12:02PM +0800, Caesar Wang wrote: > The temp_to_code function will return 0 when we set the temperature to a > invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch > will prevent this case happening. That will return the max analog value to > indicate the temperature is invalid or over table temperature range. <cut> > > /* Make sure the value is valid */ > alarm_value = rk_tsadcv2_temp_to_code(table, temp); dummy question here, looking at your tables, if I did not miss something, looks like we have an accuracy of 5C steps. Not only, that, we also support only multiples of 5C temperatures. If that observation is correct, would it make more sense to simply check for this property, and min and max temperature check, instead of going through the binary search to check for valid temperature? > if (alarm_value == table->data_mask) > - return; > + return -ERANGE; > > writel_relaxed(alarm_value & table->data_mask, > regs + TSADCV2_COMP_INT(chn)); > @@ -667,23 +665,27 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table, > int_en = readl_relaxed(regs + TSADCV2_INT_EN); > int_en |= TSADCV2_INT_SRC_EN(chn); > writel_relaxed(int_en, regs + TSADCV2_INT_EN); > + > + return 0; > } > > -static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table, > - int chn, void __iomem *regs, int temp) > +static int 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; > + return -ERANGE; > > writel_relaxed(tshut_value, regs + TSADCV2_COMP_SHUT(chn)); > > /* TSHUT will be valid */ > val = readl_relaxed(regs + TSADCV2_AUTO_CON); > writel_relaxed(val | TSADCV2_AUTO_SRC_EN(chn), regs + TSADCV2_AUTO_CON); > + > + return 0; > } > > static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs, > @@ -886,10 +888,8 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high) > dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n", > __func__, sensor->id, low, high); > > - tsadc->set_alarm_temp(&tsadc->table, > - sensor->id, thermal->regs, high); > - > - return 0; > + return tsadc->set_alarm_temp(&tsadc->table, > + sensor->id, thermal->regs, high); > } > > static int rockchip_thermal_get_temp(void *_sensor, int *out_temp) > @@ -985,8 +985,12 @@ rockchip_thermal_register_sensor(struct platform_device *pdev, > int error; > > tsadc->set_tshut_mode(id, thermal->regs, thermal->tshut_mode); > - tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs, > + > + error = tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs, > thermal->tshut_temp); > + if (error) > + dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n", > + __func__, thermal->tshut_temp, error); > > sensor->thermal = thermal; > sensor->id = id; > @@ -1199,9 +1203,13 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev) > > thermal->chip->set_tshut_mode(id, thermal->regs, > thermal->tshut_mode); > - thermal->chip->set_tshut_temp(&thermal->chip->table, > + > + error = thermal->chip->set_tshut_temp(&thermal->chip->table, > id, thermal->regs, > thermal->tshut_temp); > + if (error) > + dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n", > + __func__, thermal->tshut_temp, error); > } > > thermal->chip->control(thermal->regs, true); > -- > 2.7.4 >
Hi Eduardo, I'm not sure I completely understand what you're asking, but I'll see what I can answer. On Mon, Nov 28, 2016 at 05:45:54PM -0800, Eduardo Valentin wrote: > On Mon, Nov 28, 2016 at 07:12:02PM +0800, Caesar Wang wrote: > > The temp_to_code function will return 0 when we set the temperature to a > > invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch > > will prevent this case happening. That will return the max analog value to > > indicate the temperature is invalid or over table temperature range. > > <cut> > > > > > /* Make sure the value is valid */ > > alarm_value = rk_tsadcv2_temp_to_code(table, temp); > > dummy question here, looking at your tables, if I did not miss > something, looks like we have an accuracy of 5C steps. Not only, that, > we also support only multiples of 5C temperatures. If that observation Currently, that's true I think. But patch 4 actually supports doing the linear interpolation that is claimed (but not fully implemented) in the comments today. So with that patch, we roughly support temperatures in between the 5C intervals. > is correct, would it make more sense to simply check for this property, I'm not quite sure what you mean by "this property." Do you mean to just assume that there will be 5C intervals, and jump ahead in the table accordingly? Seems a bit fragile; nothing really guarantees that a future ADC supported by this driver won't have 1, 2, 6, or 7C accuracy (and therefore a different set of steps). > and min and max temperature check, instead of going through the binary > search to check for valid temperature? I was thinking while reviewing that the binary search serves more to complicate things than to help -- it's much harder to read (and validate that the loop termination logic is correct). And searching through a few dozen table entries doesn't really get much benefit from a O(n) -> O(log(n)) speed improvement. Anyway, I'm not sure if you were thinking along the same lines as me. Brian
Hey Brian, On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote: > Hi Eduardo, > > I'm not sure I completely understand what you're asking, but I'll see > what I can answer. > > On Mon, Nov 28, 2016 at 05:45:54PM -0800, Eduardo Valentin wrote: > > On Mon, Nov 28, 2016 at 07:12:02PM +0800, Caesar Wang wrote: > > > The temp_to_code function will return 0 when we set the temperature to a > > > invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch > > > will prevent this case happening. That will return the max analog value to > > > indicate the temperature is invalid or over table temperature range. > > > > <cut> > > > > > > > > /* Make sure the value is valid */ > > > alarm_value = rk_tsadcv2_temp_to_code(table, temp); > > > > dummy question here, looking at your tables, if I did not miss > > something, looks like we have an accuracy of 5C steps. Not only, that, > > we also support only multiples of 5C temperatures. If that observation > > Currently, that's true I think. But patch 4 actually supports doing the > linear interpolation that is claimed (but not fully implemented) in the > comments today. So with that patch, we roughly support temperatures in > between the 5C intervals. > > > is correct, would it make more sense to simply check for this property, > > I'm not quite sure what you mean by "this property." Do you mean to just > assume that there will be 5C intervals, and jump ahead in the table > accordingly? Seems a bit fragile; nothing really guarantees that a > future ADC supported by this driver won't have 1, 2, 6, or 7C accuracy > (and therefore a different set of steps). I was thinking something even simpler. I just thought that you could avoid going into the binary search on the temp to code function by simply checking if the temperature in the temp parameter is a multiple of the table step. I agree that might be a bit of a strong assumption, but then again, one should avoid over engineering for future hardware, unless you already know that the coming ADC versions will have different steps, or even worse, no step pattern at all. > > > and min and max temperature check, instead of going through the binary > > search to check for valid temperature? > > I was thinking while reviewing that the binary search serves more to > complicate things than to help -- it's much harder to read (and validate > that the loop termination logic is correct). And searching through a few > dozen table entries doesn't really get much benefit from a O(n) -> > O(log(n)) speed improvement. true. but if in your code path you do several walks in the table just to check if parameters are valid, given that you could simply decide if they are valid or not with simpler if condition, then, still worth, no? :-) > > Anyway, I'm not sure if you were thinking along the same lines as me. > Something like that, except I though of something even simpler: + if ((temp % table->step) != 0) + return -ERANGE; If temp passes that check, then you go to the temp -> code conversion. > Brian
On Tue, Nov 29, 2016 at 09:02:42PM -0800, Eduardo Valentin wrote: > On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote: > > I was thinking while reviewing that the binary search serves more to > > complicate things than to help -- it's much harder to read (and validate > > that the loop termination logic is correct). And searching through a few > > dozen table entries doesn't really get much benefit from a O(n) -> > > O(log(n)) speed improvement. > > true. but if in your code path you do several walks in the table just to > check if parameters are valid, given that you could simply decide if > they are valid or not with simpler if condition, then, still worth, no? > :-) Yes, your suggestions seems like they would have made the code both (a little) more straightforward and efficient. But... > > Anyway, I'm not sure if you were thinking along the same lines as me. > > > > Something like that, except I though of something even simpler: > + if ((temp % table->step) != 0) > + return -ERANGE; > > If temp passes that check, then you go to the temp -> code conversion. ...that check isn't valid as of patch 4, where Caesar adds handling for intermediate steps. We really never should have been strictly snapping to the 5C steps in the first place; intermediate values are OK. So, we still need some kind of search to find the right step -- or closest bracketing range, to compute the interpolated value. We should only reject temperatures that are too high or too low for the ADC to represent. --- Side track --- BTW, when we're considering rejecting temperatures here: shouldn't this be fed back to the upper layers more nicely? We're improving the error handling for this driver in this series, but it still leaves things behaving a little odd. When I tested, I can do: ## set something obviously way too high echo 700000 > trip_point_X_temp and get a 0 (success) return code from the sysfs write() syscall, even though the rockchip driver rejected it with -ERANGE. Is there really no way to feed back thermal range limits of a sensor to the of-thermal framework? Brian
Hello, On Tue, Nov 29, 2016 at 09:59:28PM -0800, Brian Norris wrote: > On Tue, Nov 29, 2016 at 09:02:42PM -0800, Eduardo Valentin wrote: > > On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote: > > > I was thinking while reviewing that the binary search serves more to > > > complicate things than to help -- it's much harder to read (and validate > > > that the loop termination logic is correct). And searching through a few > > > dozen table entries doesn't really get much benefit from a O(n) -> > > > O(log(n)) speed improvement. > > > > true. but if in your code path you do several walks in the table just to > > check if parameters are valid, given that you could simply decide if > > they are valid or not with simpler if condition, then, still worth, no? > > :-) > > Yes, your suggestions seems like they would have made the code both (a > little) more straightforward and efficient. But... > > > > Anyway, I'm not sure if you were thinking along the same lines as me. > > > > > > > Something like that, except I though of something even simpler: > > + if ((temp % table->step) != 0) > > + return -ERANGE; > > > > If temp passes that check, then you go to the temp -> code conversion. > > ...that check isn't valid as of patch 4, where Caesar adds handling for > intermediate steps. We really never should have been strictly snapping > to the 5C steps in the first place; intermediate values are OK. > > So, we still need some kind of search to find the right step -- or > closest bracketing range, to compute the interpolated value. We should > only reject temperatures that are too high or too low for the ADC to > represent. Ok. got it. check small comment on patch 4 then. > > > --- Side track --- > > BTW, when we're considering rejecting temperatures here: shouldn't this > be fed back to the upper layers more nicely? We're improving the error > handling for this driver in this series, but it still leaves things > behaving a little odd. When I tested, I can do: > > ## set something obviously way too high > echo 700000 > trip_point_X_temp > > and get a 0 (success) return code from the sysfs write() syscall, even > though the rockchip driver rejected it with -ERANGE. Is there really no > way to feed back thermal range limits of a sensor to the of-thermal > framework? > well, that is a bit strange to me. Are you sure you are returning the -ERANGE? Because, my assumption is that the following of-thermal code path would return the error code back to core: 328 if (data->ops->set_trip_temp) { 329 int ret; 330 331 ret = data->ops->set_trip_temp(data->sensor_data, trip, temp); 332 if (ret) 333 return ret; 334 } And this part of thermal core would return it back to sysfs layer: 757 ret = tz->ops->set_trip_temp(tz, trip, temperature); 758 if (ret) 759 return ret; or am I missing something? > Brian
Hi 在 2016年11月30日 14:26, Eduardo Valentin 写道: > Hello, > > On Tue, Nov 29, 2016 at 09:59:28PM -0800, Brian Norris wrote: >> On Tue, Nov 29, 2016 at 09:02:42PM -0800, Eduardo Valentin wrote: >>> On Tue, Nov 29, 2016 at 01:57:45PM -0800, Brian Norris wrote: >>>> I was thinking while reviewing that the binary search serves more to >>>> complicate things than to help -- it's much harder to read (and validate >>>> that the loop termination logic is correct). And searching through a few >>>> dozen table entries doesn't really get much benefit from a O(n) -> >>>> O(log(n)) speed improvement. >>> true. but if in your code path you do several walks in the table just to >>> check if parameters are valid, given that you could simply decide if >>> they are valid or not with simpler if condition, then, still worth, no? >>> :-) >> Yes, your suggestions seems like they would have made the code both (a >> little) more straightforward and efficient. But... >> >>>> Anyway, I'm not sure if you were thinking along the same lines as me. >>>> >>> Something like that, except I though of something even simpler: >>> + if ((temp % table->step) != 0) >>> + return -ERANGE; >>> >>> If temp passes that check, then you go to the temp -> code conversion. >> ...that check isn't valid as of patch 4, where Caesar adds handling for >> intermediate steps. We really never should have been strictly snapping >> to the 5C steps in the first place; intermediate values are OK. >> >> So, we still need some kind of search to find the right step -- or >> closest bracketing range, to compute the interpolated value. We should >> only reject temperatures that are too high or too low for the ADC to >> represent. > Ok. got it. check small comment on patch 4 then. > >> >> --- Side track --- >> >> BTW, when we're considering rejecting temperatures here: shouldn't this >> be fed back to the upper layers more nicely? We're improving the error >> handling for this driver in this series, but it still leaves things >> behaving a little odd. When I tested, I can do: >> >> ## set something obviously way too high >> echo 700000 > trip_point_X_temp >> >> and get a 0 (success) return code from the sysfs write() syscall, even >> though the rockchip driver rejected it with -ERANGE. Is there really no >> way to feed back thermal range limits of a sensor to the of-thermal >> framework? >> > well, that is a bit strange to me. Are you sure you are returning the > -ERANGE? Because, my assumption is that the following of-thermal code > path would return the error code back to core: > 328 if (data->ops->set_trip_temp) { > 329 int ret; > 330 > 331 ret = data->ops->set_trip_temp(data->sensor_data, trip, temp); > 332 if (ret) > 333 return ret; > 334 } > > And this part of thermal core would return it back to sysfs layer: > 757 ret = tz->ops->set_trip_temp(tz, trip, temperature); > 758 if (ret) > 759 return ret; > > or am I missing something? that should be related to the many trips. The trips will search the next trips. So in general, trip_ponit_0_temp <= trip_ponit_1_temp <=trip_ponit_1_temp. I'm assume you set"echo 700000 > trip_point_0_temp", and you keep trip1 and trip2.... [ 34.449718] trip_point_temp_store, temp=700000 [ 34.454568] of_thermal_set_trip_temp:336,temp=700000 [ 34.459612] thermal_sys: thermal_zone_set_trips:583, temp=45000, trip_temp=700000, hys=2000 [ 34.468026] thermal_sys: thermal_zone_set_trips:583, temp=45000, trip_temp=85000, hys=2000 [ 34.476336] thermal_sys: thermal_zone_set_trips:583, temp=45000, trip_temp=95000, hys=2000 [ 34.484634] thermal thermal_zone0: new temperature boundaries: -2147483647 < x < 85000 [ 34.492619] rockchip-thermal ff260000.tsadc: rockchip_thermal_set_trips: sensor 0: low: -2147483647, high 85000 ===> So rockchip thermal will return 0. That should report error when you meet the needs of order. order--> trip_ponit_0_temp <= trip_ponit_1_temp <=trip_ponit_1_temp. Fox example: [ 100.898552] thermal_sys: thermal_zone_set_trips:583, temp=58333, trip_temp=700000, hys=2000 [ 100.906964] thermal_sys: thermal_zone_set_trips:583, temp=58333, trip_temp=710000, hys=2000 [ 100.916329] thermal_sys: thermal_zone_set_trips:583, temp=58333, trip_temp=720000, hys=2000 [ 100.924685] thermal thermal_zone0: new temperature boundaries: -2147483647 < x < 700000 [ 100.932965] rockchip-thermal ff260000.tsadc: rockchip_thermal_set_trips: sensor 0: low: -2147483647, high 700000 [ 100.943138] rk_tsadcv2_alarm_temp:682, temp=700000 [ 100.948201] rk_tsadcv2_temp_to_code: invalid temperature, temp=700000 error=1023 [ 100.955598] thermal thermal_zone0: Failed to set trips: -34 ===> So rockchip thermal will return error. - Caesar > >> Brian > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c index 766486f..ca1730e 100644 --- a/drivers/thermal/rockchip_thermal.c +++ b/drivers/thermal/rockchip_thermal.c @@ -120,10 +120,10 @@ struct rockchip_tsadc_chip { /* Per-sensor methods */ int (*get_temp)(const struct chip_tsadc_table *table, int chn, void __iomem *reg, int *temp); - void (*set_alarm_temp)(const struct chip_tsadc_table *table, - int chn, void __iomem *reg, int temp); - void (*set_tshut_temp)(const struct chip_tsadc_table *table, - int chn, void __iomem *reg, int temp); + int (*set_alarm_temp)(const struct chip_tsadc_table *table, + int chn, void __iomem *reg, int temp); + int (*set_tshut_temp)(const struct chip_tsadc_table *table, + int chn, void __iomem *reg, int temp); void (*set_tshut_mode)(int chn, void __iomem *reg, enum tshut_mode m); /* Per-table methods */ @@ -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) @@ -651,15 +649,15 @@ static int rk_tsadcv2_get_temp(const struct chip_tsadc_table *table, return rk_tsadcv2_code_to_temp(table, val, temp); } -static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table, - int chn, void __iomem *regs, int temp) +static int 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; + return -ERANGE; writel_relaxed(alarm_value & table->data_mask, regs + TSADCV2_COMP_INT(chn)); @@ -667,23 +665,27 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table, int_en = readl_relaxed(regs + TSADCV2_INT_EN); int_en |= TSADCV2_INT_SRC_EN(chn); writel_relaxed(int_en, regs + TSADCV2_INT_EN); + + return 0; } -static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table, - int chn, void __iomem *regs, int temp) +static int 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; + return -ERANGE; writel_relaxed(tshut_value, regs + TSADCV2_COMP_SHUT(chn)); /* TSHUT will be valid */ val = readl_relaxed(regs + TSADCV2_AUTO_CON); writel_relaxed(val | TSADCV2_AUTO_SRC_EN(chn), regs + TSADCV2_AUTO_CON); + + return 0; } static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs, @@ -886,10 +888,8 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high) dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n", __func__, sensor->id, low, high); - tsadc->set_alarm_temp(&tsadc->table, - sensor->id, thermal->regs, high); - - return 0; + return tsadc->set_alarm_temp(&tsadc->table, + sensor->id, thermal->regs, high); } static int rockchip_thermal_get_temp(void *_sensor, int *out_temp) @@ -985,8 +985,12 @@ rockchip_thermal_register_sensor(struct platform_device *pdev, int error; tsadc->set_tshut_mode(id, thermal->regs, thermal->tshut_mode); - tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs, + + error = tsadc->set_tshut_temp(&tsadc->table, id, thermal->regs, thermal->tshut_temp); + if (error) + dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n", + __func__, thermal->tshut_temp, error); sensor->thermal = thermal; sensor->id = id; @@ -1199,9 +1203,13 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev) thermal->chip->set_tshut_mode(id, thermal->regs, thermal->tshut_mode); - thermal->chip->set_tshut_temp(&thermal->chip->table, + + error = thermal->chip->set_tshut_temp(&thermal->chip->table, id, thermal->regs, thermal->tshut_temp); + if (error) + dev_err(&pdev->dev, "%s: invalid tshut=%d, error=%d\n", + __func__, thermal->tshut_temp, error); } thermal->chip->control(thermal->regs, true);
The temp_to_code function will return 0 when we set the temperature to a invalid value (e.g. 61C, 62C, 63C....), that's unpractical. This patch will prevent this case happening. That will return the max analog value to indicate the temperature is invalid or over table temperature range. Signed-off-by: Caesar Wang <wxt@rock-chips.com> --- Changes in v3: - fix trivial thing for error message nd return value. Changes in v2: - As Brian commnets that restructure this to pass error codes back to the upper layers. - Improve the commit message. Changes in v1: None drivers/thermal/rockchip_thermal.c | 48 ++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 20 deletions(-)