Messages in this thread | | | From | "R, Durgadoss" <> | Subject | RE: [PATCH v2] Thermal: exynos: Add sysfs node supporting exynos's emulation mode. | Date | Wed, 31 Oct 2012 08:49:36 +0000 |
| |
Hi,
[A big cut.] > >> + > >> + mutex_lock(&data->lock); > >> + clk_enable(data->clk); > >> + > >> + reg = readl(data->base + EXYNOS_EMUL_CON); > >> + enable = reg & EXYNOS_EMUL_ENABLE; > >> + if (!enable && !temp) > >> + goto out; > > I think you what you are trying to do here is this: > > If the emulation is already disabled, and 'this' write tries to > > disable it again, you jump to 'out' as an optimization. > > Please correct me if I am wrong. > Yes, you're right. If we try to disable the emulation mode despite it already is > disabled, > then emulation mode will be failed and we can't use it. Because emulation > mode has some > strange characteristic. > Let me explain more detail. Emulation mode requires synchronous of any > value changing and > enabling. It means you can't change any value (next target temperature, > sensing time delay) > without enabling. In this code, disabling overlapping makes problem at initial > state. At the initial, > there is no next target temperature and only value 1 for delay time. But if > you try to write 0 again > to sysfs node, temp, then it write delay time with 938us and next > temperature with minimum > temperature automatically. Value is changed but enable bit is still disabled. > This is what the > problem is happened.
Thanks for the detailed Explanation.
> >> } > >> +#ifdef CONFIG_EXYNOS_THERMAL_EMUL > >> + device_create_file(&pdev->dev, &dev_attr_emulation); > > Don't we want to capture the return value here ? > > Also, if we conditionally create this, we should remove this conditionally > also. > > I did not see a corresponding device_remove_file for this attr. > Sorry, It's my mistake. I'll fix it. > >> +#endif > > Can we club all the code inside CONFIG_*_EMUL at one place ? > > This will make it neat, and not spread the #ifdefs all over the driver. > I'd like to do either. But I just couldn't find a way. > Could you give me any idea how to do it ?
I was thinking something like below:
inside #ifdef CONFIG_*_EMUL int register_emulation_mode(...) { device_create_file(...) }; int unregister_emulation_mode(...) { device_remove_file(...) }; #else static inline int register..(...) { return 0; } static inline int unregister..(...) { return 0; } #endif
Then the rest of the driver can just use register/unregister, without worrying about #ifdef.
Thanks, Durga
| |