Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
On Fri, Sep 17, 2010 at 05:37:06PM -0700, Kevin Hilman wrote: [trimmed Cc list a bit, as vger bounced my last reply due to header too long] Mark Brown broo...@opensource.wolfsonmicro.com writes: The enable/disable thing wasn't so noticable in the API itself, it was in the data structures that I found it confusing - the core has a different idea about what's going on with the system as a whole compared to the decision that an individual device is taking. Can you clarify your confusion here? I don't follow the problem you're raising. The confusion is between the enable flag meaning that the operating point is on the list that can be chosen from and it being the currently active one. It's clear once you understand what the API does but the current naming makes that harder to grasp. As I write this, I agree with what Phil pointed out; that 'available' is probably the right name for this flag, instead of 'enabled.' Yup, me too. Sure, I get that bit. What I meant was that I was expecting something that would say that changes had been made to the enabled/disabled sets and that it'd be a good idea to rescan, especially for cases where the devices change their requirements but the OPPs need to be done over a larger block. I guess you're thinking of notifiers, so if the list of available OPPs changes, a driver could (re)search to see if a more optimal OPP was available? Yup, or at least some suggestion in the API for how this should be done. Certainly sounds possible, but not sure how useful in practice. OPP change decisions are not very often, so any OPP database updates may not affect a device OPP change currently in progress, but would take effect the next time that device makes an OPP change. Like I say, the main use case would be when the device itself is not making the actual operating point decision but is feeding in to a wider block - if the device implements the decision then it really shouldn't need to notify itself about it, though I guess it might be handy. Either way, this is something that could easily be added if it seems useful. My concern there would be divergent idioms for working with the API. It'd seem better to start everyone off down the same path if we can rather than have differences between platforms which make life harder when moving between mulitple platforms. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
On Fri, Sep 17, 2010 at 07:45:43PM +0300, Phil Carmody wrote: On 17/09/10 17:36 +0200, ext Mark Brown wrote: It might be clearer to use some term other than enabled in the code - when reading I wasn't immediately sure if enabled meant that it was available to be selected or if it was the active operating point. How ^ about 'allowed' (though I'm not 100% happy with that)? I think your query contains its own answer. Yeah, I didn't pick that because all of them are available in the sense that they could in the future be enabled but I do think it's the best option. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
+struct opp_def {
+ unsigned long freq;
+ unsigned long u_volt;
+
+ bool enabled;
+};
It might be clearer to use some term other than enabled in the code -
when reading I wasn't immediately sure if enabled meant that it was
available to be selected or if it was the active operating point. How
about 'allowed' (though I'm not 100% happy with that)?
+static inline int opp_add(struct device *dev, const struct opp_def *opp_def)
+{
+ return ERR_PTR(-EINVAL);
+}
Mismatch with the return type and value here.
+/**
+ * opp_enable() - Enable a specific OPP
+ * @opp: Pointer to opp
+ *
+ * Enables a provided opp. If the operation is valid, this returns 0, else
the
+ * corresponding error value.
+ *
+ * OPP used here is from the the opp_is_valid/opp_has_freq or other search
+ * functions
+ */
+int opp_enable(struct opp *opp)
+{
+ if (unlikely(!opp || IS_ERR(opp))) {
+ pr_err(%s: Invalid parameters being passed\n, __func__);
+ return -EINVAL;
+ }
+
+ if (!opp-enabled opp-dev_opp)
+ opp-dev_opp-enabled_opp_count++;
+
+ opp-enabled = true;
+
+ return 0;
+}
When reading the description I'd expected to see some facility to
trigger selection of an active operating point in the library (possibly
as a separate call since you might have a bunch of operating points
being updated in quick succession) but it looks like that needs to be
supplied externally at the minute?
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
Mark Brown had written, on 09/17/2010 10:36 AM, the following:
On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
+struct opp_def {
+ unsigned long freq;
+ unsigned long u_volt;
+
+ bool enabled;
+};
It might be clearer to use some term other than enabled in the code -
when reading I wasn't immediately sure if enabled meant that it was
available to be selected or if it was the active operating point. How
about 'allowed' (though I'm not 100% happy with that)?
;).. The opp is enabled or disabled if it is populated, it is implicit
as being available but not enabled- how about active? this would change
the opp_enable/disable functions to opp_activate, opp_deactivate..
Recommendations folks?
+static inline int opp_add(struct device *dev, const struct opp_def *opp_def)
+{
+ return ERR_PTR(-EINVAL);
+}
Mismatch with the return type and value here.
/me kicks himself.. ouch.. thanks.. will fix in v2.
+/**
+ * opp_enable() - Enable a specific OPP
+ * @opp: Pointer to opp
+ *
+ * Enables a provided opp. If the operation is valid, this returns 0, else the
+ * corresponding error value.
+ *
+ * OPP used here is from the the opp_is_valid/opp_has_freq or other search
+ * functions
+ */
+int opp_enable(struct opp *opp)
+{
+ if (unlikely(!opp || IS_ERR(opp))) {
+ pr_err(%s: Invalid parameters being passed\n, __func__);
+ return -EINVAL;
+ }
+
+ if (!opp-enabled opp-dev_opp)
+ opp-dev_opp-enabled_opp_count++;
+
+ opp-enabled = true;
+
+ return 0;
+}
When reading the description I'd expected to see some facility to
trigger selection of an active operating point in the library (possibly
as a separate call since you might have a bunch of operating points
being updated in quick succession) but it looks like that needs to be
supplied externally at the minute?
The intent is we use the opp_search* functions to pick up the opp and
enable/activate it and disable/deactivate it.
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
On Fri, Sep 17, 2010 at 10:53:06AM -0500, Nishanth Menon wrote: Mark Brown had written, on 09/17/2010 10:36 AM, the following: It might be clearer to use some term other than enabled in the code - when reading I wasn't immediately sure if enabled meant that it was available to be selected or if it was the active operating point. How about 'allowed' (though I'm not 100% happy with that)? ;).. The opp is enabled or disabled if it is populated, it is implicit as being available but not enabled- how about active? this would change the opp_enable/disable functions to opp_activate, opp_deactivate.. Recommendations folks? The enable/disable thing wasn't so noticable in the API itself, it was in the data structures that I found it confusing - the core has a different idea about what's going on with the system as a whole compared to the decision that an individual device is taking. When reading the description I'd expected to see some facility to trigger selection of an active operating point in the library (possibly as a separate call since you might have a bunch of operating points being updated in quick succession) but it looks like that needs to be supplied externally at the minute? The intent is we use the opp_search* functions to pick up the opp and enable/activate it and disable/deactivate it. Sure, I get that bit. What I meant was that I was expecting something that would say that changes had been made to the enabled/disabled sets and that it'd be a good idea to rescan, especially for cases where the devices change their requirements but the OPPs need to be done over a larger block. -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
On Friday, September 17, 2010, Nishanth Menon wrote:
Mark Brown had written, on 09/17/2010 10:36 AM, the following:
On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
+struct opp_def {
+ unsigned long freq;
+ unsigned long u_volt;
+
+ bool enabled;
+};
It might be clearer to use some term other than enabled in the code -
when reading I wasn't immediately sure if enabled meant that it was
available to be selected or if it was the active operating point. How
about 'allowed' (though I'm not 100% happy with that)?
;).. The opp is enabled or disabled if it is populated, it is implicit
as being available but not enabled- how about active? this would change
the opp_enable/disable functions to opp_activate, opp_deactivate..
Would that mean that active is the one currently in use?
Rafael
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
Rafael J. Wysocki had written, on 09/17/2010 05:22 PM, the following:
On Friday, September 17, 2010, Nishanth Menon wrote:
Mark Brown had written, on 09/17/2010 10:36 AM, the following:
On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
+struct opp_def {
+ unsigned long freq;
+ unsigned long u_volt;
+
+ bool enabled;
+};
It might be clearer to use some term other than enabled in the code -
when reading I wasn't immediately sure if enabled meant that it was
available to be selected or if it was the active operating point. How
about 'allowed' (though I'm not 100% happy with that)?
;).. The opp is enabled or disabled if it is populated, it is implicit
as being available but not enabled- how about active? this would change
the opp_enable/disable functions to opp_activate, opp_deactivate..
Would that mean that active is the one currently in use?
I like the idea Phil pointed out[1] on using available instead..
opp_enable and disable will make the OPP available or not. does this
sound better?
[1] http://marc.info/?l=linux-arm-kernelm=128474217132058w=2
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
On Saturday, September 18, 2010, Nishanth Menon wrote:
Rafael J. Wysocki had written, on 09/17/2010 05:22 PM, the following:
On Friday, September 17, 2010, Nishanth Menon wrote:
Mark Brown had written, on 09/17/2010 10:36 AM, the following:
On Thu, Sep 16, 2010 at 08:29:33PM -0500, Nishanth Menon wrote:
+struct opp_def {
+unsigned long freq;
+unsigned long u_volt;
+
+bool enabled;
+};
It might be clearer to use some term other than enabled in the code -
when reading I wasn't immediately sure if enabled meant that it was
available to be selected or if it was the active operating point. How
about 'allowed' (though I'm not 100% happy with that)?
;).. The opp is enabled or disabled if it is populated, it is implicit
as being available but not enabled- how about active? this would change
the opp_enable/disable functions to opp_activate, opp_deactivate..
Would that mean that active is the one currently in use?
I like the idea Phil pointed out[1] on using available instead..
opp_enable and disable will make the OPP available or not. does this
sound better?
Yes, it does.
Rafael
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-pm] [PATCH] opp: introduce library for device-specific OPPs
[trimmed Cc list a bit, as vger bounced my last reply due to header too long] Mark Brown broo...@opensource.wolfsonmicro.com writes: On Fri, Sep 17, 2010 at 10:53:06AM -0500, Nishanth Menon wrote: Mark Brown had written, on 09/17/2010 10:36 AM, the following: It might be clearer to use some term other than enabled in the code - when reading I wasn't immediately sure if enabled meant that it was available to be selected or if it was the active operating point. How about 'allowed' (though I'm not 100% happy with that)? ;).. The opp is enabled or disabled if it is populated, it is implicit as being available but not enabled- how about active? this would change the opp_enable/disable functions to opp_activate, opp_deactivate.. Recommendations folks? The enable/disable thing wasn't so noticable in the API itself, it was in the data structures that I found it confusing - the core has a different idea about what's going on with the system as a whole compared to the decision that an individual device is taking. Can you clarify your confusion here? I don't follow the problem you're raising. The enabled flag in the internal data structure is set to true when opp_enable() is called and set to false when opp_disable() is called. I'm not sure At least as we're currently using it, this API has know knowlege of what OPP is currently active on the system. IOW, it has no idea what the current frequency/voltage a given device is set to. It's intended more like an OPP database with some convience functions to search through the OPPs and to make OPPs available (or not.) The users of this API (in our case, CPUfreq and voltage scaling code) are the ones which keep track of which OPP is actually in use. As I write this, I agree with what Phil pointed out; that 'available' is probably the right name for this flag, instead of 'enabled.' When reading the description I'd expected to see some facility to trigger selection of an active operating point in the library (possibly as a separate call since you might have a bunch of operating points being updated in quick succession) but it looks like that needs to be supplied externally at the minute? The intent is we use the opp_search* functions to pick up the opp and enable/activate it and disable/deactivate it. Sure, I get that bit. What I meant was that I was expecting something that would say that changes had been made to the enabled/disabled sets and that it'd be a good idea to rescan, especially for cases where the devices change their requirements but the OPPs need to be done over a larger block. I guess you're thinking of notifiers, so if the list of available OPPs changes, a driver could (re)search to see if a more optimal OPP was available? Certainly sounds possible, but not sure how useful in practice. OPP change decisions are not very often, so any OPP database updates may not affect a device OPP change currently in progress, but would take effect the next time that device makes an OPP change. Either way, this is something that could easily be added if it seems useful. Kevin -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
