"Gopinath, Thara" <thara@xxxxxx> writes: >>>-----Original Message----- >>>From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >>>Sent: Friday, October 22, 2010 10:22 PM >>>To: Gopinath, Thara >>>Cc: linux-omap@xxxxxxxxxxxxxxx; paul@xxxxxxxxx; Cousson, Benoit; Sripathy, >>>Vishwanath; Sawant, Anand >>>Subject: Re: [PATCH v3 08/11] OMAP3: PM: Adding debug support to Voltage and >>>Smartreflex drivers >>> >>>"Gopinath, Thara" <thara@xxxxxx> writes: >>> >>>>>>Thara Gopinath <thara@xxxxxx> writes: >>>>>> >>>>>>> This patch adds debug support to the voltage and smartreflex drivers. >>>>>>> This means a whole bunch of voltage processor and smartreflex >>>>>>> parameters are now visible through the pm debugfs. By default >>>>>>> only a read of these parameters are permitted. If you need to >>>>>>> write into them then >>>>>>> echo 1 > /pm_debug/enable_sr_vp_debug >>>>>> >>>>>>Why a read-only interface by default? As a debug interface it seems >>>>>>redundant to have to enable it. >>>>>> >>>> >>>> Read-only interface by default so that we can read these values from >>>> user space even if we do not want to manipulate it from user-side. >>>> >>> >>>If we do not want to manipulate it from the user-side, then simply don't >>>write to it. Remember, this is a debug interface, not a primary >>>interface. >>> >>>I think the enable_sr_vp_debug flag should disappear, and it should be a >>>read/write interface. >>> >>>If the values are changed via debugfs, then set some per-SR instance >>>flag that can be checked. >>> >>>Basically, the current code is confusing because you're using the the >>>flag called 'enable' to determine whether the user *might have* written >>>the values. >>> >>>[...] >>> >>>>>>> + /* >>>>>>> + * Getting vp errorgain based on the voltage. If the debug option >>>is >>>>>>> + * enabled allow the override of errorgain from user side. >>>>>>> + */ >>>>>> >>>>>>As suggested in earlier comment, please use a specific flag that this >>>>>>has been overridden instead of the 'debug enabled' flag (which should >>>>>>disappear, IMO) >>>> >>>> What do you mean by a separate flag. You want a flag to be maintained >>>> for just this purpose ? >>> >>>Yes. I want a flag to be maintained *specifically* for this purpose, >>>instead of using a much more general flag that only means a user *might* >>>have overridden the values, use one that specifically means a user *has* >>>overridden the values. > > Hello Kevin, > > I tried this. Couple of questions/concerns I have. > 1. If you take a look at the definition of these debugfs parameters, the omap_vdd_info struct is not passed as an argument. The actual variables are the parameters. I am not sure how to extract omap_vdd_info from this. Maybe container_of will help, but then it will be clumsy. Same concern for smartreflex err_minlimit variable. There is no way to get the sr instance except use container of which I am not sure will work or not > 2.Also in voltage layer we export out eight parameters tat can be over-ridden from the user side. I do not think we should be maintaining one flag per variable. The design will be too very clumsy. I didn't mean to suggest one flag per varialble. Just one flag to indicate that the user *has* overridden the defaults. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
