RE: [PATCH v2] Thermal: exynos: Add sysfs node supporting exynos'semulation mode.

[R, Durgadoss]

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
¢éì®&Þ~º&¶¬–+-±éÝ¥Šw®žË±Êâmébžìdz¹Þ)í…æèw*jg¬±¨¶‰šŽŠÝj/êäz¹ÞŠà2ŠÞ¨è­Ú&¢)ß«a¶Úþø®G«éh®æj:+v‰¨Šwè†Ù>Wš±êÞiÛaxPjØm¶ŸÿÃ-»+ƒùdš_