On Monday, July 04, 2011, MyungJoo Ham wrote:
> 2011/7/3 Rafael J. Wysocki <rjw@xxxxxxx>:
> > Hi,
> >
> > I'm not sure if spelling DEVFREQ in the kerneldoc comments with capitals is
> > a good idea, it looks kind of odd. I'm not sure what would be look better,
> > though.
>
> Umm.. what about "devfreq" using all non-capitals? It looks fine to me as well.
Works for me.
> >
> > On Friday, May 27, 2011, MyungJoo Ham wrote:
> >
> > ...
> >> +/**
> >> + * devfreq_monitor() - Regularly run devfreq_do() and support device DEVFREQ tickle.
> >
> > I'd say "periodically" istead of "regularly".
>
> Yes, that is the proper term. Thanks.
>
> >
> >> + * @work: the work struct used to run devfreq_monitor periodically.
> >
> > Also please say something more about the "tickle" thinkg here.
>
> Sure.
>
> >
> >> + */
> >> +static void devfreq_monitor(struct work_struct *work)
> >> +{
> >> + struct devfreq *devfreq;
> >> + int error;
> >> + bool continue_polling = false;
> >> + struct devfreq *to_remove = NULL;
> >> +
> >> + mutex_lock(&devfreq_list_lock);
> >> +
> >> + list_for_each_entry(devfreq, &devfreq_list, node) {
> >> + /* Remove the devfreq entry that failed */
> >> + if (to_remove) {
> >> + list_del(&to_remove->node);
> >> + kfree(to_remove);
> >> + to_remove = NULL;
> >> + }
> >> +
> >> + /*
> >> + * If the device is tickled and the tickle duration is left,
> >> + * do not change the frequency for a while
> >> + */
> >> + if (devfreq->tickle) {
> >> + continue_polling = true;
> >> + devfreq->tickle--;
> >> +
> >> + /*
> >> + * If the tickle is ending and the device is not going
> >> + * to poll, force the device to poll next time so that
> >> + * it can return to the original frequency afterwards.
> >> + * However, non-polling device will have 0 polling_ms,
> >> + * it will not poll again later.
> >> + */
> >> + if (devfreq->tickle == 0 && devfreq->next_polling == 0)
> >> + devfreq->next_polling = 1;
> >> +
> >> + continue;
> >> + }
> >> +
> >> + /* This device does not require polling */
> >> + if (devfreq->next_polling == 0)
> >> + continue;
> >> +
> >> + continue_polling = true;
> >> +
> >> + if (devfreq->next_polling == 1) {
> >> + /* This device is polling this time */
> >
> > I'd remove this comment, it's confusing IMO. Besides, it may be better
> > to structure the code like this:
> >
> > if (devfreq->next_polling-- == 1) {
> > }
> >
> > and then you wouldn't need the "else" at all.
>
> Ok, I'll try that style.
>
> >
> >> + error = devfreq_do(devfreq);
> >> + if (error && error != -EAGAIN) {
> >> + /*
> >> + * Remove a devfreq with error. However,
> >> + * We cannot remove it right here because the
> >
> > Comma after "here", please.
>
> "We" should be "we" here, but no comma there though:
> http://owl.english.purdue.edu/owl/resource/607/02/
OK, whatever.
> However, "However, because the ..... above, we cannot remove it right
> here" is correct.
Yes, it is.
> >
> >> + * devfreq pointer is going to be used by
> >> + * list_for_each_entry above. Thus, it is
> >> + * removed afterwards.
> >
> > Why don't you use list_for_each_entry_safe(), then?
>
> Ah.. that's a good idea. Thanks! Now, I can reorganize this one.
>
> >
> >> + */
> >> + to_remove = devfreq->dev;
> >> + dev_err(devfreq->dev, "devfreq_do error(%d). "
> >> + "DEVFREQ is removed from the device\n",
> >> + error);
> >> + continue;
> >> + }
> >> + devfreq->next_polling = DIV_ROUND_UP(
> >> + devfreq->profile->polling_ms,
> >> + DEVFREQ_INTERVAL);
> >> + } else {
> >> + /* The device will poll later when next_polling = 1 */
> >> + devfreq->next_polling--;
> >> + }
> >> + }
> >> +
> >> + if (to_remove) {
> >> + list_del(&to_remove->node);
> >> + kfree(to_remove);
> >> + to_remove = NULL;
> >> + }
> >> +
> >> + if (continue_polling) {
> >> + polling = true;
> >> + queue_delayed_work(devfreq_wq, &devfreq_work,
> >> + msecs_to_jiffies(DEVFREQ_INTERVAL));
> >> + } else {
> >> + polling = false;
> >> + }
> >
> > OK, so why exactly continue_polling is needed? It seems you might siply use
> > "polling" instead of it above.
>
> The purpose of continue_polling seems to disappear in the middle of
> development. I'll remove it.
>
> >
> >> +
> >> + mutex_unlock(&devfreq_list_lock);
> >> +}
> > ...
> >> +/**
> >> + * devfreq_update() - Notify that the device OPP has been changed.
> >> + * @dev: the device whose OPP has been changed.
> >> + * @may_not_exist: do not print error message even if the device
> >> + * does not have devfreq entry.
> >
> > This argument isn't used any more.
>
> Ah.. yes. sure.
>
> >
> >> + */
> >> +int devfreq_update(struct device *dev)
> >> +{
> >> + struct devfreq *devfreq;
> >> + int err = 0;
> >> +
> >> + mutex_lock(&devfreq_list_lock);
> >> +
> >> + devfreq = find_device_devfreq(dev);
> >> + if (IS_ERR(devfreq)) {
> >> + err = PTR_ERR(devfreq);
> >> + goto out;
> >> + }
> >> +
> >> + if (devfreq->tickle) {
> >> + /* If the max freq available is changed, re-tickle */
> >
> > It would be good to explain what "re-tickle" means.
>
> Of course. I'll explain that.
>
> >
> >> + unsigned long freq = devfreq->profile->max_freq;
> >> + struct opp *opp = opp_find_freq_floor(devfreq->dev, &freq);
> >> +
> >> + if (IS_ERR(opp)) {
> >> + err = PTR_ERR(opp);
> >> + goto out;
> >> + }
> >> +
> >> + /* Max freq available is not changed */
> >> + if (devfreq->previous_freq == freq)
> >> + goto out;
> >> +
> >> + err = devfreq->profile->target(devfreq->dev, opp);
> >> + if (!err)
> >> + devfreq->previous_freq = freq;
> >
> > Why don't we run devfreq_do() in this case?
>
> When the list of available frequencies is updated and the device is to
> be kept at its maximum frequency, we do not need to reevaluate the
> device's activities to setup the new frequency. In such cases, we only
> need to keep it at its maximum freuqency. If the maximum frequency is
> not changed, we don't need anything to do (when "tickling" is
> completed, devfreq_do() will reevaluate automatically); otherwise,
> tickling again at the new frequency is needed (re-tickle).
OK
> >> + } else {
> >> + /* Reevaluate the proper frequency */
> >> + err = devfreq_do(devfreq);
> >> + }
> >> +
> >> +out:
> >> + mutex_unlock(&devfreq_list_lock);
> >> + return err;
> >> +}
> >> +
> >
> > Add a kerneldoc comment here, please.
>
> Yes. I'll add one. (and for any other functions missing kerneldoc comments)
Well, if the function is a static one-liner, you can skip the kerneldoc IMHO
However, that one below is not really a one-liner.. :-)
> >> +static int _devfreq_tickle_device(struct devfreq *df, unsigned long delay)
> >> +{
> >> + int err = 0;
> >> + unsigned long freq;
> >> + struct opp *opp;
> >> +
> >> + freq = df->profile->max_freq;
> >> + opp = opp_find_freq_floor(df->dev, &freq);
> >> + if (IS_ERR(opp))
> >> + return PTR_ERR(opp);
> >> +
> >> + if (df->previous_freq != freq) {
> >> + err = df->profile->target(df->dev, opp);
> >> + if (!err)
> >> + df->previous_freq = freq;
> >> + }
> >> + if (err) {
> >> + dev_err(df->dev, "%s: Cannot set frequency.\n", __func__);
> >> + } else {
> >> + df->tickle = delay;
> >> + df->num_tickle++;
> >> + }
> >> +
> >> + if (devfreq_wq && !polling) {
> >> + polling = true;
> >> + queue_delayed_work(devfreq_wq, &devfreq_work,
> >> + msecs_to_jiffies(DEVFREQ_INTERVAL));
> >> + }
> >> +
> >> + return err;
> >> +}
> >
> > Thanks,
> > Rafael
> >
>
> Thank you so much for the comments!
You're welcome.
Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
