| Message ID | 1370502472-7249-2-git-send-email-l.majewski@samsung.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On 6 June 2013 12:37, Lukasz Majewski <l.majewski@samsung.com> wrote: > Subject: cpufreq: Define cpufreq_set_drv_attr_files() to add per CPU sysfs attributes Its not per-cpu. We just add it for policy->cpu and other routines actually create links. > The cpufreq_set_drv_attr_files() function creates sysfs file entry for > each available CPU. With it in place it is possible to add different > set of attributes without code duplication. Not for each available cpu but are linked to a policy->kobj and so shows up on each policy->cpus. > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com> > --- > drivers/cpufreq/cpufreq.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 1b8a48e..ca74e27 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -730,12 +730,23 @@ static int cpufreq_add_dev_symlink(unsigned int cpu, > return ret; > } > > +static int cpufreq_set_drv_attr_files(struct cpufreq_policy *policy, > + struct freq_attr **drv_attr) > +{ > + while ((drv_attr) && (*drv_attr)) { > + if (sysfs_create_file(&policy->kobj, &((*drv_attr)->attr))) > + return 1; You are changing the semantics here. We used to return error value from sysfs_create_file() and you are returning 1. > + drv_attr++; If drv_attr was valid initially, then drv_attr++ can't make it NULL. So, we don't need to check validity of drv_attr for every loop. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Viresh, > On 6 June 2013 12:37, Lukasz Majewski <l.majewski@samsung.com> wrote: > > > Subject: cpufreq: Define cpufreq_set_drv_attr_files() to add per > > CPU sysfs attributes > > Its not per-cpu. We just add it for policy->cpu and other routines > actually create links. > > > The cpufreq_set_drv_attr_files() function creates sysfs file entry > > for each available CPU. With it in place it is possible to add > > different set of attributes without code duplication. > > Not for each available cpu but are linked to a policy->kobj and so > shows up on each policy->cpus. Yes, you are right here. Thanks for detailed explanation. Being "per-cpu" comes from kobj embedded at policy, which has information about cpus affected. > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com> > > Signed-off-by: Myungjoo Ham <myungjoo.ham@samsung.com> > > --- > > drivers/cpufreq/cpufreq.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 1b8a48e..ca74e27 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -730,12 +730,23 @@ static int cpufreq_add_dev_symlink(unsigned > > int cpu, return ret; > > } > > > > +static int cpufreq_set_drv_attr_files(struct cpufreq_policy > > *policy, > > + struct freq_attr **drv_attr) > > +{ > > + while ((drv_attr) && (*drv_attr)) { > > + if (sysfs_create_file(&policy->kobj, > > &((*drv_attr)->attr))) > > + return 1; > > You are changing the semantics here. We used to return error > value from sysfs_create_file() and you are returning 1. Yes, correct. The ret from sysfs_create_file shall be returned. Returning 1 causes information lost. > > > + drv_attr++; > > If drv_attr was valid initially, then drv_attr++ can't make it NULL. > So, we don't need to check validity of drv_attr for every loop. I'm confused here. So you want to check dev_attr for NULL just after: drv_attr = cpufreq_driver->attr; if (!drv_attr) goto error; and skip the check at the while loop: while ((drv_attr) && (*drv_attr)) to while ((*drv_attr)) Am I correct?
On 6 June 2013 14:28, Lukasz Majewski <l.majewski@samsung.com> wrote: > I'm confused here. > > So you want to check dev_attr for NULL just after: > drv_attr = cpufreq_driver->attr; > if (!drv_attr) > goto error; > > and skip the check at the while loop: > while ((drv_attr) && (*drv_attr)) > > to > > while ((*drv_attr)) > > Am I correct? Bingo!! -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Viresh, > On 6 June 2013 14:28, Lukasz Majewski <l.majewski@samsung.com> wrote: > > I'm confused here. > > > > So you want to check dev_attr for NULL just after: > > drv_attr = cpufreq_driver->attr; > > if (!drv_attr) > > goto error; > > > > and skip the check at the while loop: > > while ((drv_attr) && (*drv_attr)) > > > > to > > > > while ((*drv_attr)) > > > > Am I correct? > > Bingo!! Ok, no problem :-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 1b8a48e..ca74e27 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -730,12 +730,23 @@ static int cpufreq_add_dev_symlink(unsigned int cpu, return ret; } +static int cpufreq_set_drv_attr_files(struct cpufreq_policy *policy, + struct freq_attr **drv_attr) +{ + while ((drv_attr) && (*drv_attr)) { + if (sysfs_create_file(&policy->kobj, &((*drv_attr)->attr))) + return 1; + drv_attr++; + } + + return 0; +} + static int cpufreq_add_dev_interface(unsigned int cpu, struct cpufreq_policy *policy, struct device *dev) { struct cpufreq_policy new_policy; - struct freq_attr **drv_attr; unsigned long flags; int ret = 0; unsigned int j; @@ -747,13 +758,9 @@ static int cpufreq_add_dev_interface(unsigned int cpu, return ret; /* set up files for this cpu device */ - drv_attr = cpufreq_driver->attr; - while ((drv_attr) && (*drv_attr)) { - ret = sysfs_create_file(&policy->kobj, &((*drv_attr)->attr)); - if (ret) - goto err_out_kobj_put; - drv_attr++; - } + if (cpufreq_set_drv_attr_files(policy, cpufreq_driver->attr)) + goto err_out_kobj_put; + if (cpufreq_driver->get) { ret = sysfs_create_file(&policy->kobj, &cpuinfo_cur_freq.attr); if (ret)