[PATCH v3 5/6] thermal: exynos: Register the tmu sensor with the kernel thermal layer

[Amit Kachhap]

On 9 May 2012 01:46, Andrew Morton <akpm at linux-foundation.org> wrote:
> On Tue,  8 May 2012 21:48:17 +0530
> Amit Daniel Kachhap <amit.kachhap at linaro.org> wrote:
>
>> This code added creates a link between temperature sensors, linux thermal
>> framework and cooling devices for samsung exynos platform. This layer
>> monitors the temperature from the sensor and informs the generic thermal
>> layer to take the necessary cooling action.
>>
>>
>> ...
>>
>> +static void exynos_report_trigger(void)
>> +{
>> +     unsigned int i;
>> +     char data[2];
>> +     char *envp[] = { data, NULL };
>> +
>> +     if (!th_zone || !th_zone->therm_dev)
>> +             return;
>> +
>> +     thermal_zone_device_update(th_zone->therm_dev);
>> +
>> +     mutex_lock(&th_zone->therm_dev->lock);
>> +     /* Find the level for which trip happened */
>> +     for (i = 0; i < th_zone->sensor_conf->trip_data.trip_count; i++) {
>> +             if (th_zone->therm_dev->last_temperature <
>> +                     th_zone->sensor_conf->trip_data.trip_val[i] * 1000)
>> +                     break;
>> +     }
>> +
>> +     if (th_zone->mode == THERMAL_DEVICE_ENABLED) {
>> +             if (i > 0)
>> +                     th_zone->therm_dev->polling_delay = ACTIVE_INTERVAL;
>> +             else
>> +                     th_zone->therm_dev->polling_delay = IDLE_INTERVAL;
>> +     }
>> +
>> +     sprintf(data, "%u", i);
>
> yikes, if `i' exceeds 9, we have a stack scribble.  Please review this
> and at least use snprintf(...  sizeof(data)) to prevent accidents.
Ok
>
>
>> +     kobject_uevent_env(&th_zone->therm_dev->device.kobj, KOBJ_CHANGE, envp);
>> +     mutex_unlock(&th_zone->therm_dev->lock);
>> +}
>> +
>>
>> ...
>>
>> +/* Get trip temperature callback functions for thermal zone */
>> +static int exynos_get_trip_temp(struct thermal_zone_device *thermal, int trip,
>> +                             unsigned long *temp)
>> +{
>> +     if (trip < 0 || trip > 2)
>
> I don't know what `trip' does and I don't know the meaning of the
> values 0, 1 and 2.  Documentation, please.
Agreed will put their description.
>
>> +             return -EINVAL;
>> +
>> +     *temp = th_zone->sensor_conf->trip_data.trip_val[trip];
>> +     /* convert the temperature into millicelsius */
>> +     *temp = *temp * 1000;
>> +
>> +     return 0;
>> +}
>> +
>>
>> ...
>>
>> +/* Bind callback functions for thermal zone */
>> +static int exynos_bind(struct thermal_zone_device *thermal,
>> +                     struct thermal_cooling_device *cdev)
>> +{
>> +     int ret = 0;
>> +
>> +     /* if the cooling device is the one from exynos4 bind it */
>> +     if (cdev != th_zone->cool_dev[0])
>> +             return 0;
>> +
>> +     if (thermal_zone_bind_cooling_device(thermal, 0, cdev)) {
>> +             pr_err("error binding cooling dev inst 0\n");
>> +             return -EINVAL;
>> +     }
>> +     if (thermal_zone_bind_cooling_device(thermal, 1, cdev)) {
>> +             pr_err("error binding cooling dev inst 1\n");
>> +             ret = -EINVAL;
>> +             goto error_bind1;
>> +     }
>
> There can never be more than two instances?
As of now it is fixed as we register only 2 zones
>
>> +     return ret;
>> +error_bind1:
>> +     thermal_zone_unbind_cooling_device(thermal, 0, cdev);
>> +     return ret;
>> +}
>> +
>>
>> ...
>>
>> +/* Get temperature callback functions for thermal zone */
>> +static int exynos_get_temp(struct thermal_zone_device *thermal,
>> +                     unsigned long *temp)
>> +{
>> +     void *data;
>> +
>> +     if (!th_zone->sensor_conf) {
>> +             pr_info("Temperature sensor not initialised\n");
>> +             return -EINVAL;
>> +     }
>> +     data = th_zone->sensor_conf->private_data;
>> +     *temp = th_zone->sensor_conf->read_temperature(data);
>> +     /* convert the temperature into millicelsius */
>> +     *temp = *temp * 1000;
>> +     return 0;
>> +}
>> +
>> +/* Operation callback functions for thermal zone */
>> +static struct thermal_zone_device_ops exynos_dev_ops = {
>
> Can it be const?  That sometimes saves space, as the table doesn't need
> to be moved into writeable storage at runtime.
Yes it can be made const
>
>> +     .bind = exynos_bind,
>> +     .unbind = exynos_unbind,
>> +     .get_temp = exynos_get_temp,
>> +     .get_mode = exynos_get_mode,
>> +     .set_mode = exynos_set_mode,
>> +     .get_trip_type = exynos_get_trip_type,
>> +     .get_trip_temp = exynos_get_trip_temp,
>> +     .get_crit_temp = exynos_get_crit_temp,
>> +};
>> +
>> +/* Register with the in-kernel thermal management */
>> +static int exynos_register_thermal(struct thermal_sensor_conf *sensor_conf)
>> +{
>> +     int ret, count, tab_size;
>> +     struct freq_clip_table *tab_ptr;
>> +
>> +     if (!sensor_conf || !sensor_conf->read_temperature) {
>> +             pr_err("Temperature sensor not initialised\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     th_zone = kzalloc(sizeof(struct exynos_thermal_zone), GFP_KERNEL);
>> +     if (!th_zone) {
>> +             ret = -ENOMEM;
>> +             goto err_unregister;
>
> This seems wrong?  If we need to call exynos_unregister_thermal() on
> this error path then we needed to call it on the predecing one?
> Perhaps?
ok my fault.
>
>> +     }
>> +
>> +     th_zone->sensor_conf = sensor_conf;
>> +
>> +     tab_ptr = (struct freq_clip_table *)sensor_conf->cooling_data.freq_data;
>> +     tab_size = sensor_conf->cooling_data.freq_clip_count;
>> +
>> +     /* Register the cpufreq cooling device */
>> +     th_zone->cool_dev_size = 1;
>> +     count = 0;
>> +     th_zone->cool_dev[count] = cpufreq_cooling_register(
>> +                     (struct freq_clip_table *)&(tab_ptr[count]),
>> +                     tab_size, cpumask_of(0));
>> +
>> +     if (IS_ERR(th_zone->cool_dev[count])) {
>> +             pr_err("Failed to register cpufreq cooling device\n");
>> +             ret = -EINVAL;
>> +             th_zone->cool_dev_size = 0;
>> +             goto err_unregister;
>> +     }
>> +
>> +     th_zone->therm_dev = thermal_zone_device_register(sensor_conf->name,
>> +                     3, NULL, &exynos_dev_ops, 0, 0, 0, IDLE_INTERVAL);
>> +
>> +     if (IS_ERR(th_zone->therm_dev)) {
>> +             pr_err("Failed to register thermal zone device\n");
>> +             ret = -EINVAL;
>> +             goto err_unregister;
>> +     }
>> +     th_zone->mode = THERMAL_DEVICE_ENABLED;
>> +
>> +     pr_info("Exynos: Kernel Thermal management registered\n");
>> +
>> +     return 0;
>> +
>> +err_unregister:
>> +     exynos_unregister_thermal();
>> +     return ret;
>> +}
>> +
>>
>> ...
>>
>