Dear Myungjoo,

Thanks for your review.

On 12/18/2014 03:24 PM, MyungJoo Ham wrote:

quoted

Hi Chanwoo,

I love the idea and I now have a little mechanical issues in your code.

quoted

---
 drivers/devfreq/Kconfig         |   2 +
 drivers/devfreq/Makefile        |   5 +-
 drivers/devfreq/devfreq-event.c | 449 ++++++++++++++++++++++++++++++++++++++++
 drivers/devfreq/event/Makefile  |   1 +
 include/linux/devfreq.h         | 160 ++++++++++++++
 5 files changed, 616 insertions(+), 1 deletion(-)
 create mode 100644 drivers/devfreq/devfreq-event.c
 create mode 100644 drivers/devfreq/event/Makefile

[]

quoted

[snip]

quoted

diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c
new file mode 100644
index 0000000..0e1948e
--- /dev/null
+++ b/drivers/devfreq/devfreq-event.c

@@ -0,0 +1,449 @@
+/*
+ * devfreq-event: Generic DEVFREQ Event class driver

DEVFREQ is a generic DVFS mechanism (or subsystem).

Plus, I thought devfreq-event is considered to be a "framework"
for devfreq event class drivers. Am I mistaken?

You're right. just "class driver" description is not proper.
I'll modify the description of devfreq-event.c as following:
or If you have other opinion, would you please let me know about it?

	devfreq-event: DEVFREQ-Event Framework to provide raw data of Non-CPU Devices.

devfreq-event: a framework to provide raw data and events of devfreq devices

should be enough.


[]

quoted

[snip / reversed maybe.. sorry]

quoted

+/**
+ * devfreq_event_is_enabled() - Check whether devfreq-event dev is enabled or
+ *                             not.
+ * @edev       : the devfreq-event device
+ *
+ * Note that this function check whether devfreq-event dev is enabled or not.
+ * If return true, the devfreq-event dev is enabeld. If return false, the
+ * devfreq-event dev is disabled.
+ */
+bool devfreq_event_is_enabled(struct devfreq_event_dev *edev)
+{
+       bool enabled = false;
+
+       if (!edev || !edev->desc)
+               return enabled;
+
+       mutex_lock(&edev->lock);
+
+       if (edev->enable_count > 0)
+               enabled = true;
+
+       if (edev->desc->ops && edev->desc->ops->is_enabled)
+               enabled |= edev->desc->ops->is_enabled(edev);

What does it mean when enabled_count > 0 and ops->is_enabled() is false? or..
What does it mean when enabled_count = 0 and ops->is_enabled() is true?

If you do enable_count in the subsystem, why would we rely on
ops->is_enabled()? Are you assuming that a device MAY turn itself off
without any kernel control (ops->disable()) and it is still a correct
behabior?

You're right. devfreq_event_is_enabled() has ambiguous operation according to your comment.

I'll only control the enable_count in the subsystem without ops->is_enabled()
and then remove the is_enabled function in the structre devfreq_event_ops.

Best Regards,
Chanwoo Choi

[Off-Topic]

The name of devfreq-event may look quite intersecting with irq-driven governors,
which are being proposed these days.

Although they may look intersecting, we can have them independently;
this as a sub-class and that as a governor. And we can consider to
provide a common infrastructure for irq-driven mechanisms in devfreq or
devfreq-event when we irq-driven DVFS become more general, which I
expect in 2 ~ 3 years.

So for now, both can go independently.


Cheers!
MyungJoo