Re: [lm-sensors] [PATCH v4] hwmon: Add driver for EXYNOS4 TMU

[Donggeun Kim <dg77.kim@samsung.com>]

On 2011년 09월 01일 14:22, Guenter Roeck wrote:
> On Wed, Aug 31, 2011 at 04:56:58AM -0400, Donggeun Kim wrote:
>> Signed-off-by: Donggeun Kim <dg77.kim@samsung.com>
>> Signed-off-by: MyungJoo Ham <myungjoo.ham@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
[snip]
>> +       struct exynos4_tmu_platform_data *pdata = data->pdata;
>> +       unsigned int temp_code;
>> +
>> +       switch (pdata->cal_type) {
>> +       case TYPE_TWO_POINT_TRIMMING:
>> +               temp_code = (temp - 25) *
>> +                   (data->temp_error2 - data->temp_error1) /
>> +                   (85 - 25) + data->temp_error1;
>> +               break;
>> +       case TYPE_ONE_POINT_TRIMMING:
>> +               temp_code = temp + data->temp_error1 - 25;
>> +               break;
>> +       default:
>> +               temp_code = temp + EXYNOS4_TMU_DEF_CODE_TO_TEMP_OFFSET;
>> +               break;
>> +       }
> 
> A bit of an overall question/concern - with all those calculations, can there
> be over- or underflows ? What if temp_code is < 0 (ie 0xffffffXX) or > 255 ?
> 
temp should range between 25 and 125. So, the argument should be checked
whether it is over or under the range.
I will fix it.
[snip]
>> +       struct exynos4_tmu_platform_data *pdata = data->pdata;
>> +       unsigned int temp;
>> +
>> +       switch (pdata->cal_type) {
>> +       case TYPE_TWO_POINT_TRIMMING:
>> +               temp = (temp_code - data->temp_error1) * (85 - 25) /
>> +                   (data->temp_error2 - data->temp_error1) + 25;
>> +               break;
>> +       case TYPE_ONE_POINT_TRIMMING:
>> +               temp = temp_code - data->temp_error1 + 25;
>> +               break;
>> +       default:
>> +               temp = temp_code - EXYNOS4_TMU_DEF_CODE_TO_TEMP_OFFSET;
>> +               break;
>> +       }
>> +
> Any over- or underflow concerns here ?
> 
temp_code, which is read from register, should range between 75 and 175.
It also should be checked.
[snip]
>> +static void exynos4_tmu_work(struct work_struct *work)
>> +{
>> +       struct exynos4_tmu_data *data = container_of(work,
>> +                       struct exynos4_tmu_data, irq_work);
>> +       char *envp[2];
>> +
>> +       mutex_lock(&data->lock);
>> +       clk_enable(data->clk);
>> +
>> +       data->interrupt_stat = readl(data->base + EXYNOS4_TMU_REG_INTSTAT);
>> +
>> +       writel(EXYNOS4_TMU_INTCLEAR_VAL, data->base + EXYNOS4_TMU_REG_INTCLEAR);
>> +
>> +       if (data->interrupt_stat & EXYNOS4_TMU_TRIG_LEVEL3_MASK) {
>> +               envp[0] = "TRIG_LEVEL=3";
>> +               sysfs_notify(&data->hwmon_dev->kobj, NULL,
>> +                       "temp1_emergency_alarm");
>> +       } else if (data->interrupt_stat & EXYNOS4_TMU_TRIG_LEVEL2_MASK) {
>> +               envp[0] = "TRIG_LEVEL=2";
>> +               sysfs_notify(&data->hwmon_dev->kobj, NULL,
>> +                       "temp1_crit_alarm");
>> +       } else if (data->interrupt_stat & EXYNOS4_TMU_TRIG_LEVEL1_MASK) {
>> +               envp[0] = "TRIG_LEVEL=1";
>> +               sysfs_notify(&data->hwmon_dev->kobj, NULL, "temp1_max_alarm");
>> +       } else
>> +               envp[0] = "TRIG_LEVEL=0";
>> +       envp[1] = NULL;
>> +
>> +       kobject_uevent_env(&data->hwmon_dev->kobj, KOBJ_CHANGE, envp);
>> +
> This is the big one. We'll have to decide how to handle this. There is currently 
> no ABI for uevents. If we permit uevents, I think there should be a common ABI,
> and we should avoid a situation where every driver returns a different set of events.
> 
If you have the common ABI for uevents in mind, I will follow it.
If calling 'kobject_uevent_env' is the matter, I consider replacing it
with koject_uevent function.

Thanks.



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors