? 2016?11?24? 02:35, Brian Norris ??:
> Hi Caesar,
>
> On Wed, Nov 23, 2016 at 10:29:32PM +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 at rock-chips.com>
>> ---
>>
>> 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 | 41 ++++++++++++++++++++++++++------------
>> 1 file changed, 28 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
>> index 766486f..e243e40 100644
>> --- a/drivers/thermal/rockchip_thermal.c
>> +++ b/drivers/thermal/rockchip_thermal.c
>> @@ -120,9 +120,9 @@ 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 (*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 (*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);
>>
>> @@ -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,7 +649,7 @@ 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,
>> +static int rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>> int chn, void __iomem *regs, int temp)
>> {
>> u32 alarm_value, int_en;
>> @@ -659,7 +657,7 @@ static void rk_tsadcv2_alarm_temp(const struct chip_tsadc_table *table,
>> /* 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,9 +665,11 @@ 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,
>> +static int rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
>> int chn, void __iomem *regs, int temp)
>> {
>> u32 tshut_value, val;
>> @@ -677,13 +677,15 @@ static void rk_tsadcv2_tshut_temp(const struct chip_tsadc_table *table,
>> /* 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,
>> @@ -882,12 +884,17 @@ static int rockchip_thermal_set_trips(void *_sensor, int low, int high)
>> struct rockchip_thermal_sensor *sensor = _sensor;
>> struct rockchip_thermal_data *thermal = sensor->thermal;
>> const struct rockchip_tsadc_chip *tsadc = thermal->chip;
>> + int ret;
>>
>> dev_dbg(&thermal->pdev->dev, "%s: sensor %d: low: %d, high %d\n",
>> __func__, sensor->id, low, high);
>>
>> - tsadc->set_alarm_temp(&tsadc->table,
>> + ret = tsadc->set_alarm_temp(&tsadc->table,
>> sensor->id, thermal->regs, high);
>> + if (ret)
>> + dev_err(&thermal->pdev->dev,
>> + "%s: disable sensor[%d] alarm temp: %d, ret: %d\n",
> You're not necessarily disabling the sensor; you're just not programming
> it. If this is the second time setting the trip points (e.g., using
> 'echo > trip_X_temp') then this just means you've retained the old
> value.
>
> Also, I'm not sure you need to print anything here, as the thermal core
> prints errors for you. Maybe make this dev_dbg() so it doesn't print by
> default?
Yeah, the thermal core will print something, look like I just print
"failed set the sensor*....
or return ret".
Anyway, that's not ugly thing.
>> + __func__, sensor->id, high, ret);
>>
>> return 0;
> Oh, but you missed the whole point of returning errors; you need to
> propagate 'ret' here! i.e.:
>
> return ret;
Right, thanks for tracking into it.
I will wait someone feedback for this series patches before posting new
patchset.
-Caesar
>
> Brian
>
>> }
>> @@ -985,8 +992,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 +1210,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
>>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
