Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1505844 > unrolled thread

Re: [PATCH 2/2] cpuidle: governors: Move the files to the upper directory

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] cpuidle: governors: Move the files to the upper directory "Rafael J. Wysocki" <rafael@kernel.org> - 2016-10-21 14:50 +0200
    Re: [PATCH 2/2] cpuidle: governors: Move the files to the upper  directory Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-10-21 15:20 +0200
      Re: [PATCH 2/2] cpuidle: governors: Move the files to the upper  directory Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-10-21 15:30 +0200
      Re: [PATCH 2/2] cpuidle: governors: Move the files to the upper directory "Rafael J. Wysocki" <rafael@kernel.org> - 2016-10-21 15:30 +0200

#1505844 — Re: [PATCH 2/2] cpuidle: governors: Move the files to the upper directory

On Wed, Oct 5, 2016 at 9:33 AM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> Currently the different governors are stored in the subdir
> 'governors'. That is not a problem.
>
> However, that forces to declare some private structure in the
> include/linux/cpuidle.h header because these governor files
> don't have access to the private 'cpuidle.h' located in
> drivers/cpuidle.
>
> Instead of having the governors in the separate directory, move
> them along with the drivers and prefix them with 'governor-',
> that allows to do a proper cleanup in the cpuidle headers.

While I'm not particularly against this change, I'm sort of wondering
about the reason.

What in particular would be wrong with doing

#include "../cpuidle.h"

in a governor .c file?

Thanks,
Rafael

[toc] | [next] | [standalone]

#1505925 — Re: [PATCH 2/2] cpuidle: governors: Move the files to the upper directory

On Fri, Oct 21, 2016 at 02:47:22PM +0200, Rafael J. Wysocki wrote:
> On Wed, Oct 5, 2016 at 9:33 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > Currently the different governors are stored in the subdir
> > 'governors'. That is not a problem.
> >
> > However, that forces to declare some private structure in the
> > include/linux/cpuidle.h header because these governor files
> > don't have access to the private 'cpuidle.h' located in
> > drivers/cpuidle.
> >
> > Instead of having the governors in the separate directory, move
> > them along with the drivers and prefix them with 'governor-',
> > that allows to do a proper cleanup in the cpuidle headers.
> 
> While I'm not particularly against this change, I'm sort of wondering
> about the reason.
> 
> What in particular would be wrong with doing
> 
> #include "../cpuidle.h"
> 
> in a governor .c file?

Hi Rafael,

there is nothing wrong by doing this relative inclusion. It is an alternative
to the proposed patch. I personally don't like relative inclusion but it is
a matter of taste and I am perfectly fine to resend the patch by just moving
the structure to the private header and change the inclusion.

On the other side, the cpufreq susbsytem has all the governors along with the
drivers in the same directory, so perhaps it makes sense to have a similar files
organization.

Actually, I'm fine with both approaches. Up to you to decide.

  -- Daniel

[toc] | [prev] | [next] | [standalone]

#1505934 — Re: [PATCH 2/2] cpuidle: governors: Move the files to the upper directory

On Fri, Oct 21, 2016 at 03:22:18PM +0200, Rafael J. Wysocki wrote:
> On Fri, Oct 21, 2016 at 3:09 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
> > On Fri, Oct 21, 2016 at 02:47:22PM +0200, Rafael J. Wysocki wrote:
> >> On Wed, Oct 5, 2016 at 9:33 AM, Daniel Lezcano
> >> <daniel.lezcano@linaro.org> wrote:
> >> > Currently the different governors are stored in the subdir
> >> > 'governors'. That is not a problem.
> >> >
> >> > However, that forces to declare some private structure in the
> >> > include/linux/cpuidle.h header because these governor files
> >> > don't have access to the private 'cpuidle.h' located in
> >> > drivers/cpuidle.
> >> >
> >> > Instead of having the governors in the separate directory, move
> >> > them along with the drivers and prefix them with 'governor-',
> >> > that allows to do a proper cleanup in the cpuidle headers.
> >>
> >> While I'm not particularly against this change, I'm sort of wondering
> >> about the reason.
> >>
> >> What in particular would be wrong with doing
> >>
> >> #include "../cpuidle.h"
> >>
> >> in a governor .c file?
> >
> > Hi Rafael,
> >
> > there is nothing wrong by doing this relative inclusion. It is an alternative
> > to the proposed patch. I personally don't like relative inclusion but it is
> > a matter of taste and I am perfectly fine to resend the patch by just moving
> > the structure to the private header and change the inclusion.
> >
> > On the other side, the cpufreq susbsytem has all the governors along with the
> > drivers in the same directory, so perhaps it makes sense to have a similar files
> > organization.
> >
> > Actually, I'm fine with both approaches. Up to you to decide.
> 
> I'm thinking let's keep the code where it is in case people depend on
> the current location somehow (ie. have patches out of the tree or
> similar).  We can still move it later if need be.

Ok, I will resend the patch [2/2] by moving the structure from the exported header
to the private header and add the relative inclusion path.

Thanks.

  -- Daniel

[toc] | [prev] | [next] | [standalone]

#1505950

On Fri, Oct 21, 2016 at 3:09 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On Fri, Oct 21, 2016 at 02:47:22PM +0200, Rafael J. Wysocki wrote:
>> On Wed, Oct 5, 2016 at 9:33 AM, Daniel Lezcano
>> <daniel.lezcano@linaro.org> wrote:
>> > Currently the different governors are stored in the subdir
>> > 'governors'. That is not a problem.
>> >
>> > However, that forces to declare some private structure in the
>> > include/linux/cpuidle.h header because these governor files
>> > don't have access to the private 'cpuidle.h' located in
>> > drivers/cpuidle.
>> >
>> > Instead of having the governors in the separate directory, move
>> > them along with the drivers and prefix them with 'governor-',
>> > that allows to do a proper cleanup in the cpuidle headers.
>>
>> While I'm not particularly against this change, I'm sort of wondering
>> about the reason.
>>
>> What in particular would be wrong with doing
>>
>> #include "../cpuidle.h"
>>
>> in a governor .c file?
>
> Hi Rafael,
>
> there is nothing wrong by doing this relative inclusion. It is an alternative
> to the proposed patch. I personally don't like relative inclusion but it is
> a matter of taste and I am perfectly fine to resend the patch by just moving
> the structure to the private header and change the inclusion.
>
> On the other side, the cpufreq susbsytem has all the governors along with the
> drivers in the same directory, so perhaps it makes sense to have a similar files
> organization.
>
> Actually, I'm fine with both approaches. Up to you to decide.

I'm thinking let's keep the code where it is in case people depend on
the current location somehow (ie. have patches out of the tree or
similar).  We can still move it later if need be.

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web