| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 12 Apr 2013 16:33:49 +0530
From: amit kachhap <amit.kachhap@...il.com>
To: Eduardo Valentin <eduardo.valentin@...com>
Cc: linux-pm@...r.kernel.org,
Thomas Abraham <thomas.abraham@...aro.org>,
Zhang Rui <rui.zhang@...el.com>,
linux-samsung-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
Kukjin Kim <kgene.kim@...sung.com>
Subject: Re: [2/9] thermal: exynos: Add support for instance based register/unregister
Hi Eduardo,
On Fri, Apr 12, 2013 at 1:39 AM, Eduardo Valentin
<eduardo.valentin@...com> wrote:
> Hey Amit,
>
>
> On 26-03-2013 07:33, Amit Daniel Kachhap wrote:
>>
>> This code modifies the thermal driver to have multiple thermal zone
>> support by replacing the global thermal zone varibale with device data
>
>
> s/varibale/variable
ok type mistake :)
>
>
>> member of thermal_zone_device.
>>
>> Signed-off-by: Amit Daniel Kachhap <amit.daniel@...sung.com>
>
>
> I understand the idea of your patch but I also see that you do at least to
> major changes. One is to set the thermal_device.devdata. The second is to
> split your internal reference private_data into driver_data and pzone_data.
> Is it possible to split this patch into two, one per modification? So it is
> easier to review your changes?
yes agreed..the kind of patch restructuring suggested by you makes sense.
>
>
>>
>> ---
>> drivers/thermal/exynos_thermal.c | 65
>> ++++++++++++++++++++++---------------
>> 1 files changed, 39 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/thermal/exynos_thermal.c
>> b/drivers/thermal/exynos_thermal.c
>> index 1cd7837..dc9b91b 100644
>> --- a/drivers/thermal/exynos_thermal.c
>> +++ b/drivers/thermal/exynos_thermal.c
>> @@ -148,7 +148,8 @@ struct thermal_sensor_conf {
>> int (*write_emul_temp)(void *drv_data, unsigned long temp);
>> struct thermal_trip_point_conf trip_data;
>> struct thermal_cooling_conf cooling_data;
>> - void *private_data;
>> + void *driver_data;
>> + void *pzone_data;
>> };
>>
>> struct exynos_thermal_zone {
>> @@ -161,14 +162,14 @@ struct exynos_thermal_zone {
>> bool bind;
>> };
>>
>> -static struct exynos_thermal_zone *th_zone;
>> -static void exynos_unregister_thermal(void);
>> +static void exynos_unregister_thermal(struct thermal_sensor_conf
>> *sensor_conf);
>> static int exynos_register_thermal(struct thermal_sensor_conf
>> *sensor_conf);
>>
>> /* Get mode callback functions for thermal zone */
>> static int exynos_get_mode(struct thermal_zone_device *thermal,
>> enum thermal_device_mode *mode)
>> {
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>> if (th_zone)
>> *mode = th_zone->mode;
>> return 0;
>> @@ -178,25 +179,26 @@ static int exynos_get_mode(struct
>> thermal_zone_device *thermal,
>> static int exynos_set_mode(struct thermal_zone_device *thermal,
>> enum thermal_device_mode mode)
>> {
>> - if (!th_zone->therm_dev) {
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>> + if (!th_zone) {
>> pr_notice("thermal zone not registered\n");
>> return 0;
>> }
>>
>> - mutex_lock(&th_zone->therm_dev->lock);
>> + mutex_lock(&thermal->lock);
>>
>> if (mode == THERMAL_DEVICE_ENABLED &&
>> !th_zone->sensor_conf->trip_data.trigger_falling)
>> - th_zone->therm_dev->polling_delay = IDLE_INTERVAL;
>> + thermal->polling_delay = IDLE_INTERVAL;
>> else
>> - th_zone->therm_dev->polling_delay = 0;
>> + thermal->polling_delay = 0;
>>
>> - mutex_unlock(&th_zone->therm_dev->lock);
>> + mutex_unlock(&thermal->lock);
>>
>> th_zone->mode = mode;
>> - thermal_zone_device_update(th_zone->therm_dev);
>> + thermal_zone_device_update(thermal);
>> pr_info("thermal polling set for duration=%d msec\n",
>> - th_zone->therm_dev->polling_delay);
>> + thermal->polling_delay);
>> return 0;
>> }
>>
>> @@ -223,6 +225,8 @@ static int exynos_get_trip_type(struct
>> thermal_zone_device *thermal, int trip,
>> static int exynos_get_trip_temp(struct thermal_zone_device *thermal, int
>> trip,
>> unsigned long *temp)
>> {
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>> +
>> if (trip < GET_TRIP(MONITOR_ZONE) || trip > GET_TRIP(PANIC_ZONE))
>> return -EINVAL;
>>
>> @@ -269,6 +273,7 @@ static int exynos_bind(struct thermal_zone_device
>> *thermal,
>> {
>> int ret = 0, i, tab_size, level;
>> struct freq_clip_table *tab_ptr, *clip_data;
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>> struct thermal_sensor_conf *data = th_zone->sensor_conf;
>>
>> tab_ptr = (struct freq_clip_table *)data->cooling_data.freq_data;
>> @@ -315,6 +320,7 @@ static int exynos_unbind(struct thermal_zone_device
>> *thermal,
>> struct thermal_cooling_device *cdev)
>> {
>> int ret = 0, i, tab_size;
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>> struct thermal_sensor_conf *data = th_zone->sensor_conf;
>>
>> if (th_zone->bind == false)
>> @@ -357,13 +363,14 @@ static int exynos_unbind(struct thermal_zone_device
>> *thermal,
>> static int exynos_get_temp(struct thermal_zone_device *thermal,
>> unsigned long *temp)
>> {
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>> void *data;
>>
>> if (!th_zone->sensor_conf) {
>> pr_info("Temperature sensor not initialised\n");
>> return -EINVAL;
>> }
>> - data = th_zone->sensor_conf->private_data;
>> + data = th_zone->sensor_conf->driver_data;
>> *temp = th_zone->sensor_conf->read_temperature(data);
>> /* convert the temperature into millicelsius */
>> *temp = *temp * MCELSIUS;
>> @@ -376,12 +383,13 @@ static int exynos_set_emul_temp(struct
>> thermal_zone_device *thermal,
>> {
>> void *data;
>> int ret = -EINVAL;
>> + struct exynos_thermal_zone *th_zone = thermal->devdata;
>>
>> if (!th_zone->sensor_conf) {
>> pr_info("Temperature sensor not initialised\n");
>> return -EINVAL;
>> }
>> - data = th_zone->sensor_conf->private_data;
>> + data = th_zone->sensor_conf->driver_data;
>> if (th_zone->sensor_conf->write_emul_temp)
>> ret = th_zone->sensor_conf->write_emul_temp(data, temp);
>> return ret;
>> @@ -391,7 +399,7 @@ static int exynos_set_emul_temp(struct
>> thermal_zone_device *thermal,
>> static int exynos_get_trend(struct thermal_zone_device *thermal,
>> int trip, enum thermal_trend *trend)
>> {
>> - int ret;
>> + int ret = 0;
>> unsigned long trip_temp;
>>
>> ret = exynos_get_trip_temp(thermal, trip, &trip_temp);
>> @@ -403,7 +411,7 @@ static int exynos_get_trend(struct thermal_zone_device
>> *thermal,
>> else
>> *trend = THERMAL_TREND_DROP_FULL;
>>
>> - return 0;
>> + return ret;
>> }
>> /* Operation callback functions for thermal zone */
>> static struct thermal_zone_device_ops const exynos_dev_ops = {
>> @@ -423,11 +431,12 @@ static struct thermal_zone_device_ops const
>> exynos_dev_ops = {
>> * This function may be called from interrupt based temperature sensor
>> * when threshold is changed.
>> */
>> -static void exynos_report_trigger(void)
>> +static void exynos_report_trigger(struct thermal_sensor_conf *conf)
>> {
>> unsigned int i;
>> char data[10];
>> char *envp[] = { data, NULL };
>> + struct exynos_thermal_zone *th_zone = conf->pzone_data;
>>
>> if (!th_zone || !th_zone->therm_dev)
>> return;
>> @@ -468,6 +477,7 @@ static int exynos_register_thermal(struct
>> thermal_sensor_conf *sensor_conf)
>> {
>> int ret;
>> struct cpumask mask_val;
>> + struct exynos_thermal_zone *th_zone;
>>
>> if (!sensor_conf || !sensor_conf->read_temperature) {
>> pr_err("Temperature sensor not initialised\n");
>> @@ -489,7 +499,7 @@ static int exynos_register_thermal(struct
>> thermal_sensor_conf *sensor_conf)
>> th_zone->cool_dev_size++;
>>
>> th_zone->therm_dev =
>> thermal_zone_device_register(sensor_conf->name,
>> - EXYNOS_ZONE_COUNT, 0, NULL, &exynos_dev_ops, NULL,
>> 0,
>> + EXYNOS_ZONE_COUNT, 0, th_zone, &exynos_dev_ops,
>> NULL, 0,
>> sensor_conf->trip_data.trigger_falling ?
>> 0 : IDLE_INTERVAL);
>>
>> @@ -499,20 +509,22 @@ static int exynos_register_thermal(struct
>> thermal_sensor_conf *sensor_conf)
>> goto err_unregister;
>> }
>> th_zone->mode = THERMAL_DEVICE_ENABLED;
>> + sensor_conf->pzone_data = th_zone;
>>
>> pr_info("Exynos: Kernel Thermal management registered\n");
>>
>> return 0;
>>
>> err_unregister:
>> - exynos_unregister_thermal();
>> + exynos_unregister_thermal(sensor_conf);
>> return ret;
>> }
>>
>> /* Un-Register with the in-kernel thermal management */
>> -static void exynos_unregister_thermal(void)
>> +static void exynos_unregister_thermal(struct thermal_sensor_conf
>> *sensor_conf)
>> {
>> int i;
>> + struct exynos_thermal_zone *th_zone = sensor_conf->pzone_data;
>>
>> if (!th_zone)
>> return;
>> @@ -774,12 +786,18 @@ static int exynos_tmu_set_emulation(void *drv_data,
>> unsigned long temp)
>> { return -EINVAL; }
>> #endif/*CONFIG_THERMAL_EMULATION*/
>>
>> +static struct thermal_sensor_conf exynos_sensor_conf = {
>> + .name = "exynos-therm",
>> + .read_temperature = (int (*)(void *))exynos_tmu_read,
>> + .write_emul_temp = exynos_tmu_set_emulation,
>> +};
>> +
>
>
> Assuming you have only one exynos_sensor_conf and this is the parameter for
> each call to exynos_register_thermal, if you plan to add support to
> different pzone_data, then the above needs to be allocated. Or there is no
> point in having this.
Yes creating this structure by allocation is a good idea. Actually In
my case I used a different file for another type of sensor so left it
like this. Anyway your suggestion is worth implementing.
>
>
>> static void exynos_tmu_work(struct work_struct *work)
>> {
>> struct exynos_tmu_data *data = container_of(work,
>> struct exynos_tmu_data, irq_work);
>>
>> - exynos_report_trigger();
>> + exynos_report_trigger(&exynos_sensor_conf);
>> mutex_lock(&data->lock);
>> clk_enable(data->clk);
>> if (data->soc == SOC_ARCH_EXYNOS)
>> @@ -804,11 +822,6 @@ static irqreturn_t exynos_tmu_irq(int irq, void *id)
>>
>> return IRQ_HANDLED;
>> }
>> -static struct thermal_sensor_conf exynos_sensor_conf = {
>> - .name = "exynos-therm",
>> - .read_temperature = (int (*)(void *))exynos_tmu_read,
>> - .write_emul_temp = exynos_tmu_set_emulation,
>> -};
>>
>> #if defined(CONFIG_CPU_EXYNOS4210)
>> static struct exynos_tmu_platform_data const exynos4210_default_tmu_data
>> = {
>> @@ -987,7 +1000,7 @@ static int exynos_tmu_probe(struct platform_device
>> *pdev)
>> exynos_tmu_control(pdev, true);
>>
>> /* Register the sensor with thermal management interface */
>> - (&exynos_sensor_conf)->private_data = data;
>> + (&exynos_sensor_conf)->driver_data = data;
>> exynos_sensor_conf.trip_data.trip_count = pdata->trigger_level0_en
>> +
>> pdata->trigger_level1_en +
>> pdata->trigger_level2_en +
>> pdata->trigger_level3_en;
>> @@ -1026,7 +1039,7 @@ static int exynos_tmu_remove(struct platform_device
>> *pdev)
>>
>> exynos_tmu_control(pdev, false);
>>
>> - exynos_unregister_thermal();
>> + exynos_unregister_thermal(&exynos_sensor_conf);
>>
>> clk_put(data->clk);
>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists