Messages in this thread Patch in this message | | | Date | Thu, 11 Apr 2013 15:33:46 -0400 | From | Eduardo Valentin <> | Subject | Re: [1/9] thermal: exynos: Adapt to temperature emulation core thermal framework |
| |
Hello Amit,
Couple of comments inline.
On 26-03-2013 07:33, Amit Daniel Kachhap wrote: > This removes the driver specific sysfs support of the temperature > emulation and uses the newly added core thermal framework for thermal > emulation. An exynos platform specific handler is added to support this. > > In this patch, the exynos senor(tmu) related code and exynos framework > related (thermal zone, cooling devices) code are intentionally kept separate. > So an emulated function pointer is passed from sensor to framework. This is > beneficial in adding more sensor support using the same framework code > which is an ongoing work. The goal is to finally split them totally. Even > the existing read_temperature also follows the same execution method. > > Acked-by: Kukjin Kim <kgene.kim@samsung.com> > Signed-off-by: Amit Daniel Kachhap <amit.daniel@samsung.com> > > --- > Documentation/thermal/exynos_thermal_emulation | 8 +- > drivers/thermal/Kconfig | 9 -- > drivers/thermal/exynos_thermal.c | 158 ++++++++++-------------- > 3 files changed, 67 insertions(+), 108 deletions(-) > > diff --git a/Documentation/thermal/exynos_thermal_emulation b/Documentation/thermal/exynos_thermal_emulation > index b73bbfb..36a3e79 100644 > --- a/Documentation/thermal/exynos_thermal_emulation > +++ b/Documentation/thermal/exynos_thermal_emulation > @@ -13,11 +13,11 @@ Thermal emulation mode supports software debug for TMU's operation. User can set > manually with software code and TMU will read current temperature from user value not from > sensor's value. > > -Enabling CONFIG_EXYNOS_THERMAL_EMUL option will make this support in available. > -When it's enabled, sysfs node will be created under > -/sys/bus/platform/devices/'exynos device name'/ with name of 'emulation'. > +Enabling CONFIG_THERMAL_EMULATION option will make this support available. > +When it's enabled, sysfs node will be created as > +/sys/devices/virtual/thermal/thermal_zone'zone id'/emul_temp. > > -The sysfs node, 'emulation', will contain value 0 for the initial state. When you input any > +The sysfs node, 'emul_node', will contain value 0 for the initial state. When you input any > temperature you want to update to sysfs node, it automatically enable emulation mode and > current temperature will be changed into it. > (Exynos also supports user changable delay time which would be used to delay of
not part of this patch but: s/changable/changeable
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index a764f16..da4c19e 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -117,15 +117,6 @@ config EXYNOS_THERMAL > If you say yes here you get support for TMU (Thermal Management > Unit) on SAMSUNG EXYNOS series of SoC. > > -config EXYNOS_THERMAL_EMUL > - bool "EXYNOS TMU emulation mode support" > - depends on EXYNOS_THERMAL > - help > - Exynos 4412 and 4414 and 5 series has emulation mode on TMU. > - Enable this option will be make sysfs node in exynos thermal platform > - device directory to support emulation mode. With emulation mode sysfs > - node, you can manually input temperature to TMU for simulation purpose. > - > config DOVE_THERMAL > tristate "Temperature sensor on Marvell Dove SoCs" > depends on ARCH_DOVE > diff --git a/drivers/thermal/exynos_thermal.c b/drivers/thermal/exynos_thermal.c > index 46568c0..1cd7837 100644 > --- a/drivers/thermal/exynos_thermal.c > +++ b/drivers/thermal/exynos_thermal.c > @@ -100,13 +100,13 @@ > #define IDLE_INTERVAL 10000 > #define MCELSIUS 1000 > > -#ifdef CONFIG_EXYNOS_THERMAL_EMUL > +#ifdef CONFIG_THERMAL_EMULATION > #define EXYNOS_EMUL_TIME 0x57F0 > #define EXYNOS_EMUL_TIME_SHIFT 16 > #define EXYNOS_EMUL_DATA_SHIFT 8 > #define EXYNOS_EMUL_DATA_MASK 0xFF > #define EXYNOS_EMUL_ENABLE 0x1 > -#endif /* CONFIG_EXYNOS_THERMAL_EMUL */ > +#endif /* CONFIG_THERMAL_EMULATION */ >
As the above is only used in one single function, I suggest moving it to same ifdef where the function belongs below. It reduces your ifdefery and also makes your code cleaner.
> /* CPU Zone information */ > #define PANIC_ZONE 4 > @@ -145,6 +145,7 @@ struct thermal_cooling_conf { > struct thermal_sensor_conf { > char name[SENSOR_NAME_LEN]; > int (*read_temperature)(void *data); > + int (*write_emul_temp)(void *drv_data, unsigned long temp);
I dont get why you need a private callback to do this, assuming you have only one sensor_conf for exynos.
> struct thermal_trip_point_conf trip_data; > struct thermal_cooling_conf cooling_data; > void *private_data; > @@ -369,6 +370,23 @@ static int exynos_get_temp(struct thermal_zone_device *thermal, > return 0; > } > > +/* Get temperature callback functions for thermal zone */ > +static int exynos_set_emul_temp(struct thermal_zone_device *thermal, > + unsigned long temp) > +{ > + void *data; > + int ret = -EINVAL;
In case you still want to keep your private callback, Id say -ENOTSUPP is better for the case you dont have the callback.
> + > + if (!th_zone->sensor_conf) { > + pr_info("Temperature sensor not initialised\n"); > + return -EINVAL; > + } > + data = th_zone->sensor_conf->private_data; > + if (th_zone->sensor_conf->write_emul_temp) > + ret = th_zone->sensor_conf->write_emul_temp(data, temp);
nip: a blank line.
> + return ret; > +} > + > /* Get the temperature trend */ > static int exynos_get_trend(struct thermal_zone_device *thermal, > int trip, enum thermal_trend *trend) > @@ -392,6 +410,7 @@ static struct thermal_zone_device_ops const exynos_dev_ops = { > .bind = exynos_bind, > .unbind = exynos_unbind, > .get_temp = exynos_get_temp, > + .set_emul_temp = exynos_set_emul_temp, > .get_trend = exynos_get_trend, > .get_mode = exynos_get_mode, > .set_mode = exynos_set_mode, > @@ -714,6 +733,47 @@ static int exynos_tmu_read(struct exynos_tmu_data *data) > return temp; > } > > +#ifdef CONFIG_THERMAL_EMULATION >
I think your code looks cleaner if you move the defines at the beginning of your patch to this point.
+static int exynos_tmu_set_emulation(void *drv_data, unsigned long temp) > +{ > + struct exynos_tmu_data *data = drv_data; > + unsigned int reg; > + int ret = -EINVAL; > + > + if (data->soc == SOC_ARCH_EXYNOS4210) > + goto out; > +
Can you resolve this by not defining this callback for the SOC_ARCH_EXYNOS4210 thermal zone ops?
> + if (temp && temp < MCELSIUS) > + goto out; > + > + mutex_lock(&data->lock); > + clk_enable(data->clk); > + > + reg = readl(data->base + EXYNOS_EMUL_CON); > + > + if (temp) { > + temp /= MCELSIUS; > + > + reg = (EXYNOS_EMUL_TIME << EXYNOS_EMUL_TIME_SHIFT) | > + (temp_to_code(data, temp) > + << EXYNOS_EMUL_DATA_SHIFT) | EXYNOS_EMUL_ENABLE; > + } else { > + reg &= ~EXYNOS_EMUL_ENABLE; > + } > + > + writel(reg, data->base + EXYNOS_EMUL_CON); > + > + clk_disable(data->clk); > + mutex_unlock(&data->lock);
nip: blank line.
> + return 0; > +out: > + return ret; > +} > +#else > +static int exynos_tmu_set_emulation(void *drv_data, unsigned long temp) unnecessary space |
> + { return -EINVAL; }
I believe if you do a static inline function return 0, the compiler should translate it into a nope.
> +#endif/*CONFIG_THERMAL_EMULATION*/ > + > static void exynos_tmu_work(struct work_struct *work) > { > struct exynos_tmu_data *data = container_of(work, > @@ -747,6 +807,7 @@ static irqreturn_t exynos_tmu_irq(int irq, void *id) > 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) > @@ -853,93 +914,6 @@ static inline struct exynos_tmu_platform_data *exynos_get_driver_data( > platform_get_device_id(pdev)->driver_data; > } > > -#ifdef CONFIG_EXYNOS_THERMAL_EMUL > -static ssize_t exynos_tmu_emulation_show(struct device *dev, > - struct device_attribute *attr, > - char *buf) > -{ > - struct platform_device *pdev = container_of(dev, > - struct platform_device, dev); > - struct exynos_tmu_data *data = platform_get_drvdata(pdev); > - unsigned int reg; > - u8 temp_code; > - int temp = 0; > - > - if (data->soc == SOC_ARCH_EXYNOS4210) > - goto out; > - > - mutex_lock(&data->lock); > - clk_enable(data->clk); > - reg = readl(data->base + EXYNOS_EMUL_CON); > - clk_disable(data->clk); > - mutex_unlock(&data->lock); > - > - if (reg & EXYNOS_EMUL_ENABLE) { > - reg >>= EXYNOS_EMUL_DATA_SHIFT; > - temp_code = reg & EXYNOS_EMUL_DATA_MASK; > - temp = code_to_temp(data, temp_code); > - } > -out: > - return sprintf(buf, "%d\n", temp * MCELSIUS); > -} > - > -static ssize_t exynos_tmu_emulation_store(struct device *dev, > - struct device_attribute *attr, > - const char *buf, size_t count) > -{ > - struct platform_device *pdev = container_of(dev, > - struct platform_device, dev); > - struct exynos_tmu_data *data = platform_get_drvdata(pdev); > - unsigned int reg; > - int temp; > - > - if (data->soc == SOC_ARCH_EXYNOS4210) > - goto out; > - > - if (!sscanf(buf, "%d\n", &temp) || temp < 0) > - return -EINVAL; > - > - mutex_lock(&data->lock); > - clk_enable(data->clk); > - > - reg = readl(data->base + EXYNOS_EMUL_CON); > - > - if (temp) { > - /* Both CELSIUS and MCELSIUS type are available for input */ > - if (temp > MCELSIUS) > - temp /= MCELSIUS; > - > - reg = (EXYNOS_EMUL_TIME << EXYNOS_EMUL_TIME_SHIFT) | > - (temp_to_code(data, (temp / MCELSIUS)) > - << EXYNOS_EMUL_DATA_SHIFT) | EXYNOS_EMUL_ENABLE; > - } else { > - reg &= ~EXYNOS_EMUL_ENABLE; > - } > - > - writel(reg, data->base + EXYNOS_EMUL_CON); > - > - clk_disable(data->clk); > - mutex_unlock(&data->lock); > - > -out: > - return count; > -} > - > -static DEVICE_ATTR(emulation, 0644, exynos_tmu_emulation_show, > - exynos_tmu_emulation_store); > -static int create_emulation_sysfs(struct device *dev) > -{ > - return device_create_file(dev, &dev_attr_emulation); > -} > -static void remove_emulation_sysfs(struct device *dev) > -{ > - device_remove_file(dev, &dev_attr_emulation); > -} > -#else > -static inline int create_emulation_sysfs(struct device *dev) { return 0; } > -static inline void remove_emulation_sysfs(struct device *dev) {} > -#endif > - > static int exynos_tmu_probe(struct platform_device *pdev) > { > struct exynos_tmu_data *data; > @@ -1039,10 +1013,6 @@ static int exynos_tmu_probe(struct platform_device *pdev) > goto err_clk; > } > > - ret = create_emulation_sysfs(&pdev->dev); > - if (ret) > - dev_err(&pdev->dev, "Failed to create emulation mode sysfs node\n"); > - > return 0; > err_clk: > platform_set_drvdata(pdev, NULL); > @@ -1054,8 +1024,6 @@ static int exynos_tmu_remove(struct platform_device *pdev) > { > struct exynos_tmu_data *data = platform_get_drvdata(pdev); > > - remove_emulation_sysfs(&pdev->dev); > - > exynos_tmu_control(pdev, false); > > exynos_unregister_thermal(); >
| |