Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Tue, Oct 23, 2012 at 10:31:28AM +0100, Russell King - ARM Linux wrote: > On Tue, Oct 23, 2012 at 11:22:47AM +0200, Thierry Reding wrote: > > On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote: > > > Further to the discussion, my preference is still for of_clk_get() > > > (although I've changed the patch anyway as you saw because it makes no > > > difference in this case) :) > > > > > > clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to > > > allow platforms to convert to DT without having to update all their > > > drivers first. It only allows the first (default) clock, as your pointed > > > out. Getting a 2nd... clock relies on an optional property in DT (which > > > again, seems like it is there to support 'old' drivers) which allows you > > > to request clocks by name. > > > > > > of_clk_get() on the other hand seems like a properly native DT function. > > > You don't need to know anything about the clock, as long as the correct > > > clock is specified in the correct order as documented by the binding. > > > Relying on 'pre-OF' code for a OF-only driver also seems > > > counter-intuitive. > > > > I do agree with those arguments. What I was saying is that for drivers > > which aren't DT only, of_clk_get() is not an option and that maybe > > others would be encouraged by the example to not use the generic APIs > > even if their driver could be used in non-DT setups. But maybe I'm > > worrying needlessly. > > > > That said, maybe somebody with a broader view of things like Arnd > > (Cc'ed) could share his thoughts. > > As I have already said, the way the DT bindings were done for the clk > stuff was wrong. A little thought put into it would've come up with > a much better solution which wouldn't have needed of_clk_get() at all. > > How? > > The arguments for clk_get() are: > 1. the struct device, which you can get the OF-node from. > 2. a _device_ _specific_ _clock_ _input_ _name_ (or NULL if there's only >one.) > > So, we have something that defines a hardware clock input name, which > can be used to generate a property name for OF. So, what _could_ have > been done is this: > > clock- = < clk-output-index>; > > where the property name is generated by: > > snprintf(prop, sizeof(prop), "clk-%s", name ? name : "default"); But we already have this, only with slightly different syntax: clocks = < foo-index>, < bar-index>; clock-names = "foo", "bar"; > So I continue to assert that our current design is wrong - and it will > cause driver authors to pointlessly have to make a choice at every stage > between DT and non-DT based systems. I think the reason that Tony brought this up is that with this API, the clock-names property becomes mandatory if you have more than one input clock. Thierry pgpcF5n9mH33P.pgp Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Tue, Oct 23, 2012 at 11:22:47AM +0200, Thierry Reding wrote: > On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote: > > Further to the discussion, my preference is still for of_clk_get() > > (although I've changed the patch anyway as you saw because it makes no > > difference in this case) :) > > > > clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to > > allow platforms to convert to DT without having to update all their > > drivers first. It only allows the first (default) clock, as your pointed > > out. Getting a 2nd... clock relies on an optional property in DT (which > > again, seems like it is there to support 'old' drivers) which allows you > > to request clocks by name. > > > > of_clk_get() on the other hand seems like a properly native DT function. > > You don't need to know anything about the clock, as long as the correct > > clock is specified in the correct order as documented by the binding. > > Relying on 'pre-OF' code for a OF-only driver also seems > > counter-intuitive. > > I do agree with those arguments. What I was saying is that for drivers > which aren't DT only, of_clk_get() is not an option and that maybe > others would be encouraged by the example to not use the generic APIs > even if their driver could be used in non-DT setups. But maybe I'm > worrying needlessly. > > That said, maybe somebody with a broader view of things like Arnd > (Cc'ed) could share his thoughts. As I have already said, the way the DT bindings were done for the clk stuff was wrong. A little thought put into it would've come up with a much better solution which wouldn't have needed of_clk_get() at all. How? The arguments for clk_get() are: 1. the struct device, which you can get the OF-node from. 2. a _device_ _specific_ _clock_ _input_ _name_ (or NULL if there's only one.) So, we have something that defines a hardware clock input name, which can be used to generate a property name for OF. So, what _could_ have been done is this: clock- = < clk-output-index>; where the property name is generated by: snprintf(prop, sizeof(prop), "clk-%s", name ? name : "default"); So I continue to assert that our current design is wrong - and it will cause driver authors to pointlessly have to make a choice at every stage between DT and non-DT based systems. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote:
> On Mon, 2012-10-22 at 10:04 +0200, Thierry Reding wrote:
> > On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
> > > On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
> > > > On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> > > > > On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > > > > >
> > > > > > > > chip = devm_kzalloc(>dev, sizeof(*chip),
> > > > > > > > GFP_KERNEL);
> > > > > > > > if (chip == NULL) {
> > > > > > > > dev_err(>dev, "failed to allocate
> > > > > > > > memory\n");
> > > > > > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
> > > > > > > > platform_device *pdev)
> > > > > > > > chip->chip.ops = _pwm_ops;
> > > > > > > > chip->chip.base = -1;
> > > > > > > > chip->chip.npwm = VT8500_NR_PWMS;
> > > > > > > > + chip->clk = of_clk_get(np, 0);
> > > > > > >
> > > > > > > I thought this was supposed to work transparently across OF and
> > > > > > > !OF
> > > > > > > configurations by using just clk_get() or devm_clk_get()? I guess
> > > > > > > that
> > > > > > > if the driver depends on OF, then this would be moot, but we
> > > > > > > should
> > > > > > > probably stick to the standard usage anyway.
> > > > > > >
> > > > > > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd
> > > > > > > need to
> > > > > > > add explicit clk_put() in the error cleanup paths. One more
> > > > > > > argument in
> > > > > > > favour of using devm_clk_get() instead.
> > > > > >
> > > > > > Hmm good point. I stuck with of_ functions because its an OF only
> > > > > > driver
> > > > > > and it seemed 'backward' to mix old code with new. It does pose the
> > > > > > question of 'why have of_clk_get() if existing functions work
> > > > > > better'.
> > > > >
> > > > > Was about to fix this but noticed why it wasn't like this to start
> > > > > with :)
> > > > >
> > > > > struct clk *devm_clk_get(struct device *dev, const char *id);
> > > > > struct clk *of_clk_get(struct device_node *np, int index);
> > > > >
> > > > > devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and
> > > > > I
> > > > > believe a lot of other arch's) don't enforce names for clocks defined
> > > > > in
> > > > > devicetree, therefore there is no way for me to know what name the clk
> > > > > has unless I include in the binding that the clock must be named
> > > > > 'xxx'.
> > > >
> > > > I thought clk_get() was supposed to return the first clock specified in
> > > > DT if you pass NULL as the consumer name. I haven't tested this though.
> > > > And I haven't looked at the code.
> > > >
> > > > > of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> > > > > long as its the 1st clock listed.
> > > >
> > > > So the usual way to do this, I believe, is:
> > > >
> > > > clocks = <_foo>;
> > > > clock-names = "foo";
> > > >
> > > > Then use:
> > > >
> > > > clk = devm_clk_get(>dev, "foo");
> > > >
> > > > And as I said above, I was under the impression that the default would
> > > > be to use the first clock if NULL was specified instead of "foo".
> > > >
> > > > Thierry
> > >
> > > clock-names is an optional property (as defined in
> > > bindings/clock/clock-bindings.txt) so relying on it is .. well,
> > > unreliable.
> > >
> > > What you say makes sense, but it means the binding document has to make
> > > an optional property into a required property simply to use an 'old'
> > > function when a new function would 'work' (granted not as well, as you
> > > pointed out) without requiring the optional property.
> >
> > Okay, I've just checked the core clock code, and indeed if you run
> > clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
> > with index == 0. That's exactly what you want, right? The only setup
> > where this won't work out is if you need to handle multiple clocks, in
> > which case I think it would make sense to make the clock-names property
> > mandatory. But for this driver that won't be necessary, since it will
> > never use a second clock, right?
> >
> > > Your subsystem - your rules. Let me know if I've managed to sway you or
> > > not :)
> >
> > I'd rather persuade you than force the issue. =)
> >
> > Thierry
>
> Further to the discussion, my preference is still for of_clk_get()
> (although I've changed the patch anyway as you saw because it makes no
> difference in this case) :)
>
> clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to
> allow platforms to convert to DT without having to update all their
> drivers first. It only allows the first (default) clock, as your pointed
> out. Getting a 2nd... clock relies on an optional property in DT (which
> again, seems like it is there to support 'old' drivers) which allows you
> to request clocks by name.
>
> of_clk_get() on the other hand seems like a
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, 2012-10-22 at 10:04 +0200, Thierry Reding wrote:
> On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
> > On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
> > > On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> > > > On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > > > >
> > > > > > > chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
> > > > > > > if (chip == NULL) {
> > > > > > > dev_err(>dev, "failed to allocate memory\n");
> > > > > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
> > > > > > > platform_device *pdev)
> > > > > > > chip->chip.ops = _pwm_ops;
> > > > > > > chip->chip.base = -1;
> > > > > > > chip->chip.npwm = VT8500_NR_PWMS;
> > > > > > > + chip->clk = of_clk_get(np, 0);
> > > > > >
> > > > > > I thought this was supposed to work transparently across OF and !OF
> > > > > > configurations by using just clk_get() or devm_clk_get()? I guess
> > > > > > that
> > > > > > if the driver depends on OF, then this would be moot, but we should
> > > > > > probably stick to the standard usage anyway.
> > > > > >
> > > > > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need
> > > > > > to
> > > > > > add explicit clk_put() in the error cleanup paths. One more
> > > > > > argument in
> > > > > > favour of using devm_clk_get() instead.
> > > > >
> > > > > Hmm good point. I stuck with of_ functions because its an OF only
> > > > > driver
> > > > > and it seemed 'backward' to mix old code with new. It does pose the
> > > > > question of 'why have of_clk_get() if existing functions work better'.
> > > >
> > > > Was about to fix this but noticed why it wasn't like this to start
> > > > with :)
> > > >
> > > > struct clk *devm_clk_get(struct device *dev, const char *id);
> > > > struct clk *of_clk_get(struct device_node *np, int index);
> > > >
> > > > devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> > > > believe a lot of other arch's) don't enforce names for clocks defined in
> > > > devicetree, therefore there is no way for me to know what name the clk
> > > > has unless I include in the binding that the clock must be named 'xxx'.
> > >
> > > I thought clk_get() was supposed to return the first clock specified in
> > > DT if you pass NULL as the consumer name. I haven't tested this though.
> > > And I haven't looked at the code.
> > >
> > > > of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> > > > long as its the 1st clock listed.
> > >
> > > So the usual way to do this, I believe, is:
> > >
> > > clocks = <_foo>;
> > > clock-names = "foo";
> > >
> > > Then use:
> > >
> > > clk = devm_clk_get(>dev, "foo");
> > >
> > > And as I said above, I was under the impression that the default would
> > > be to use the first clock if NULL was specified instead of "foo".
> > >
> > > Thierry
> >
> > clock-names is an optional property (as defined in
> > bindings/clock/clock-bindings.txt) so relying on it is .. well,
> > unreliable.
> >
> > What you say makes sense, but it means the binding document has to make
> > an optional property into a required property simply to use an 'old'
> > function when a new function would 'work' (granted not as well, as you
> > pointed out) without requiring the optional property.
>
> Okay, I've just checked the core clock code, and indeed if you run
> clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
> with index == 0. That's exactly what you want, right? The only setup
> where this won't work out is if you need to handle multiple clocks, in
> which case I think it would make sense to make the clock-names property
> mandatory. But for this driver that won't be necessary, since it will
> never use a second clock, right?
>
> > Your subsystem - your rules. Let me know if I've managed to sway you or
> > not :)
>
> I'd rather persuade you than force the issue. =)
>
> Thierry
Further to the discussion, my preference is still for of_clk_get()
(although I've changed the patch anyway as you saw because it makes no
difference in this case) :)
clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to
allow platforms to convert to DT without having to update all their
drivers first. It only allows the first (default) clock, as your pointed
out. Getting a 2nd... clock relies on an optional property in DT (which
again, seems like it is there to support 'old' drivers) which allows you
to request clocks by name.
of_clk_get() on the other hand seems like a properly native DT function.
You don't need to know anything about the clock, as long as the correct
clock is specified in the correct order as documented by the binding.
Relying on 'pre-OF' code for a OF-only driver also seems
counter-intuitive.
Granted of_clk_get() doesn't provide the 'garbage-collection' of
devm_get_clk() but I think that is just an arguement to needing
of_devm_clk_get().
Just my two cents.
Regards
Tony P
--
To
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, 2012-10-22 at 10:04 +0200, Thierry Reding wrote:
On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
chip = devm_kzalloc(pdev-dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
platform_device *pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess
that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need
to
add explicit clk_put() in the error cleanup paths. One more
argument in
favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only
driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
Was about to fix this but noticed why it wasn't like this to start
with :)
struct clk *devm_clk_get(struct device *dev, const char *id);
struct clk *of_clk_get(struct device_node *np, int index);
devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
believe a lot of other arch's) don't enforce names for clocks defined in
devicetree, therefore there is no way for me to know what name the clk
has unless I include in the binding that the clock must be named 'xxx'.
I thought clk_get() was supposed to return the first clock specified in
DT if you pass NULL as the consumer name. I haven't tested this though.
And I haven't looked at the code.
of_clk_get retrieves it by the dt-node + index, so it doesn't care as
long as its the 1st clock listed.
So the usual way to do this, I believe, is:
clocks = clk_foo;
clock-names = foo;
Then use:
clk = devm_clk_get(pdev-dev, foo);
And as I said above, I was under the impression that the default would
be to use the first clock if NULL was specified instead of foo.
Thierry
clock-names is an optional property (as defined in
bindings/clock/clock-bindings.txt) so relying on it is .. well,
unreliable.
What you say makes sense, but it means the binding document has to make
an optional property into a required property simply to use an 'old'
function when a new function would 'work' (granted not as well, as you
pointed out) without requiring the optional property.
Okay, I've just checked the core clock code, and indeed if you run
clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
with index == 0. That's exactly what you want, right? The only setup
where this won't work out is if you need to handle multiple clocks, in
which case I think it would make sense to make the clock-names property
mandatory. But for this driver that won't be necessary, since it will
never use a second clock, right?
Your subsystem - your rules. Let me know if I've managed to sway you or
not :)
I'd rather persuade you than force the issue. =)
Thierry
Further to the discussion, my preference is still for of_clk_get()
(although I've changed the patch anyway as you saw because it makes no
difference in this case) :)
clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to
allow platforms to convert to DT without having to update all their
drivers first. It only allows the first (default) clock, as your pointed
out. Getting a 2nd... clock relies on an optional property in DT (which
again, seems like it is there to support 'old' drivers) which allows you
to request clocks by name.
of_clk_get() on the other hand seems like a properly native DT function.
You don't need to know anything about the clock, as long as the correct
clock is specified in the correct order as documented by the binding.
Relying on 'pre-OF' code for a OF-only driver also seems
counter-intuitive.
Granted of_clk_get() doesn't provide the 'garbage-collection' of
devm_get_clk() but I think that is just an arguement to needing
of_devm_clk_get().
Just my two cents.
Regards
Tony P
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 10:04 +0200, Thierry Reding wrote:
On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
chip = devm_kzalloc(pdev-dev, sizeof(*chip),
GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate
memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
platform_device *pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and
!OF
configurations by using just clk_get() or devm_clk_get()? I guess
that
if the driver depends on OF, then this would be moot, but we
should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd
need to
add explicit clk_put() in the error cleanup paths. One more
argument in
favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only
driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work
better'.
Was about to fix this but noticed why it wasn't like this to start
with :)
struct clk *devm_clk_get(struct device *dev, const char *id);
struct clk *of_clk_get(struct device_node *np, int index);
devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and
I
believe a lot of other arch's) don't enforce names for clocks defined
in
devicetree, therefore there is no way for me to know what name the clk
has unless I include in the binding that the clock must be named
'xxx'.
I thought clk_get() was supposed to return the first clock specified in
DT if you pass NULL as the consumer name. I haven't tested this though.
And I haven't looked at the code.
of_clk_get retrieves it by the dt-node + index, so it doesn't care as
long as its the 1st clock listed.
So the usual way to do this, I believe, is:
clocks = clk_foo;
clock-names = foo;
Then use:
clk = devm_clk_get(pdev-dev, foo);
And as I said above, I was under the impression that the default would
be to use the first clock if NULL was specified instead of foo.
Thierry
clock-names is an optional property (as defined in
bindings/clock/clock-bindings.txt) so relying on it is .. well,
unreliable.
What you say makes sense, but it means the binding document has to make
an optional property into a required property simply to use an 'old'
function when a new function would 'work' (granted not as well, as you
pointed out) without requiring the optional property.
Okay, I've just checked the core clock code, and indeed if you run
clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
with index == 0. That's exactly what you want, right? The only setup
where this won't work out is if you need to handle multiple clocks, in
which case I think it would make sense to make the clock-names property
mandatory. But for this driver that won't be necessary, since it will
never use a second clock, right?
Your subsystem - your rules. Let me know if I've managed to sway you or
not :)
I'd rather persuade you than force the issue. =)
Thierry
Further to the discussion, my preference is still for of_clk_get()
(although I've changed the patch anyway as you saw because it makes no
difference in this case) :)
clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to
allow platforms to convert to DT without having to update all their
drivers first. It only allows the first (default) clock, as your pointed
out. Getting a 2nd... clock relies on an optional property in DT (which
again, seems like it is there to support 'old' drivers) which allows you
to request clocks by name.
of_clk_get() on the other hand seems like a properly native DT function.
You don't need to know anything about the clock, as long as the correct
clock is specified in the correct order as documented by the binding.
Relying on 'pre-OF' code for a OF-only driver also seems
counter-intuitive.
I do agree with those arguments. What I was saying is that for drivers
which aren't DT only, of_clk_get() is not an option and that maybe
others would be encouraged by the example to not use the generic APIs
even if
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Tue, Oct 23, 2012 at 11:22:47AM +0200, Thierry Reding wrote: On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote: Further to the discussion, my preference is still for of_clk_get() (although I've changed the patch anyway as you saw because it makes no difference in this case) :) clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to allow platforms to convert to DT without having to update all their drivers first. It only allows the first (default) clock, as your pointed out. Getting a 2nd... clock relies on an optional property in DT (which again, seems like it is there to support 'old' drivers) which allows you to request clocks by name. of_clk_get() on the other hand seems like a properly native DT function. You don't need to know anything about the clock, as long as the correct clock is specified in the correct order as documented by the binding. Relying on 'pre-OF' code for a OF-only driver also seems counter-intuitive. I do agree with those arguments. What I was saying is that for drivers which aren't DT only, of_clk_get() is not an option and that maybe others would be encouraged by the example to not use the generic APIs even if their driver could be used in non-DT setups. But maybe I'm worrying needlessly. That said, maybe somebody with a broader view of things like Arnd (Cc'ed) could share his thoughts. As I have already said, the way the DT bindings were done for the clk stuff was wrong. A little thought put into it would've come up with a much better solution which wouldn't have needed of_clk_get() at all. How? The arguments for clk_get() are: 1. the struct device, which you can get the OF-node from. 2. a _device_ _specific_ _clock_ _input_ _name_ (or NULL if there's only one.) So, we have something that defines a hardware clock input name, which can be used to generate a property name for OF. So, what _could_ have been done is this: clock-input-name = provider-node clk-output-index; where the property name is generated by: snprintf(prop, sizeof(prop), clk-%s, name ? name : default); So I continue to assert that our current design is wrong - and it will cause driver authors to pointlessly have to make a choice at every stage between DT and non-DT based systems. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Tue, Oct 23, 2012 at 10:31:28AM +0100, Russell King - ARM Linux wrote: On Tue, Oct 23, 2012 at 11:22:47AM +0200, Thierry Reding wrote: On Tue, Oct 23, 2012 at 09:41:46PM +1300, Tony Prisk wrote: Further to the discussion, my preference is still for of_clk_get() (although I've changed the patch anyway as you saw because it makes no difference in this case) :) clk_get(x, NULL) and devm_clk_get(x, NULL) both seems like 'hacks' to allow platforms to convert to DT without having to update all their drivers first. It only allows the first (default) clock, as your pointed out. Getting a 2nd... clock relies on an optional property in DT (which again, seems like it is there to support 'old' drivers) which allows you to request clocks by name. of_clk_get() on the other hand seems like a properly native DT function. You don't need to know anything about the clock, as long as the correct clock is specified in the correct order as documented by the binding. Relying on 'pre-OF' code for a OF-only driver also seems counter-intuitive. I do agree with those arguments. What I was saying is that for drivers which aren't DT only, of_clk_get() is not an option and that maybe others would be encouraged by the example to not use the generic APIs even if their driver could be used in non-DT setups. But maybe I'm worrying needlessly. That said, maybe somebody with a broader view of things like Arnd (Cc'ed) could share his thoughts. As I have already said, the way the DT bindings were done for the clk stuff was wrong. A little thought put into it would've come up with a much better solution which wouldn't have needed of_clk_get() at all. How? The arguments for clk_get() are: 1. the struct device, which you can get the OF-node from. 2. a _device_ _specific_ _clock_ _input_ _name_ (or NULL if there's only one.) So, we have something that defines a hardware clock input name, which can be used to generate a property name for OF. So, what _could_ have been done is this: clock-input-name = provider-node clk-output-index; where the property name is generated by: snprintf(prop, sizeof(prop), clk-%s, name ? name : default); But we already have this, only with slightly different syntax: clocks = provider foo-index, provider bar-index; clock-names = foo, bar; So I continue to assert that our current design is wrong - and it will cause driver authors to pointlessly have to make a choice at every stage between DT and non-DT based systems. I think the reason that Tony brought this up is that with this API, the clock-names property becomes mandatory if you have more than one input clock. Thierry pgpcF5n9mH33P.pgp Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, 2012-10-22 at 17:08 +0200, Thierry Reding wrote: > On Mon, Oct 22, 2012 at 01:52:08PM +, Arnd Bergmann wrote: > > On Monday 22 October 2012, Thierry Reding wrote: > > > > As long as we get build warnings for leaving out the __devinit/__devexit > > > > annotations, I would generally recommend putting them in. If we do a > > > > patch to remove all of them, a couple extra instances will not cause > > > > any more troubles than we already have. > > > > > > I've never seen any build warnings for leaving __devinit/__devexit out. > > > Where does that happen? > > > > Section mismatches usually result into warnings from modpost, like > > > > WARNING: modpost: Found 1 section mismatch(es). > > To see full details build your kernel with: > > 'make CONFIG_DEBUG_SECTION_MISMATCH=y' > > > > Actually doing that gives you an output like this (currently on > > exynos_defconfig): > > > > $ make CONFIG_DEBUG_SECTION_MISMATCH=y > > WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch > > in reference from the function samsung_pinctrl_probe() to the function > > .init.text:samsung_gpiolib_register() > > The function __devinit samsung_pinctrl_probe() references > > a function __init samsung_gpiolib_register(). > > If samsung_gpiolib_register is only used by samsung_pinctrl_probe then > > annotate samsung_gpiolib_register with a matching annotation. > > > > or like this (now fixed in socfpga_defconfig): > > > > WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): > > Section mismatch in reference from the function stmmac_pltfr_probe() to the > > function .devinit.text:stmmac_probe_config_dt() > > The function stmmac_pltfr_probe() references > > the function __devinit stmmac_probe_config_dt(). > > This is often because stmmac_pltfr_probe lacks a __devinit > > annotation or the annotation of stmmac_probe_config_dt is wrong. > > > > I believe you normally don't get warnings for functions that could be > > marked __devinit and only call regular functions, but there are > > a couple of __devinit infrastructure functions that you can't call > > from a function that isn't __init or __devinit. > > Right. If you get those warnings you shouldn't be dropping the > annotations. But I don't think that is the case for this driver. Tony, > can you confirm that the driver still builds properly without warnings > if you drop the __devinit/__devexit? > > Thierry > ___ > linux-arm-kernel mailing list > linux-arm-ker...@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel Correct - it builds without warnings without __devinit/__devexit. If it had introduced warnings when I tested it, I would have mentioned it :) Regards Tony P -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 01:52:08PM +, Arnd Bergmann wrote: > On Monday 22 October 2012, Thierry Reding wrote: > > > As long as we get build warnings for leaving out the __devinit/__devexit > > > annotations, I would generally recommend putting them in. If we do a > > > patch to remove all of them, a couple extra instances will not cause > > > any more troubles than we already have. > > > > I've never seen any build warnings for leaving __devinit/__devexit out. > > Where does that happen? > > Section mismatches usually result into warnings from modpost, like > > WARNING: modpost: Found 1 section mismatch(es). > To see full details build your kernel with: > 'make CONFIG_DEBUG_SECTION_MISMATCH=y' > > Actually doing that gives you an output like this (currently on > exynos_defconfig): > > $ make CONFIG_DEBUG_SECTION_MISMATCH=y > WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch in > reference from the function samsung_pinctrl_probe() to the function > .init.text:samsung_gpiolib_register() > The function __devinit samsung_pinctrl_probe() references > a function __init samsung_gpiolib_register(). > If samsung_gpiolib_register is only used by samsung_pinctrl_probe then > annotate samsung_gpiolib_register with a matching annotation. > > or like this (now fixed in socfpga_defconfig): > > WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section > mismatch in reference from the function stmmac_pltfr_probe() to the function > .devinit.text:stmmac_probe_config_dt() > The function stmmac_pltfr_probe() references > the function __devinit stmmac_probe_config_dt(). > This is often because stmmac_pltfr_probe lacks a __devinit > annotation or the annotation of stmmac_probe_config_dt is wrong. > > I believe you normally don't get warnings for functions that could be > marked __devinit and only call regular functions, but there are > a couple of __devinit infrastructure functions that you can't call > from a function that isn't __init or __devinit. Right. If you get those warnings you shouldn't be dropping the annotations. But I don't think that is the case for this driver. Tony, can you confirm that the driver still builds properly without warnings if you drop the __devinit/__devexit? Thierry pgp7NZLqFpoFN.pgp Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Monday 22 October 2012, Thierry Reding wrote: > > As long as we get build warnings for leaving out the __devinit/__devexit > > annotations, I would generally recommend putting them in. If we do a > > patch to remove all of them, a couple extra instances will not cause > > any more troubles than we already have. > > I've never seen any build warnings for leaving __devinit/__devexit out. > Where does that happen? Section mismatches usually result into warnings from modpost, like WARNING: modpost: Found 1 section mismatch(es). To see full details build your kernel with: 'make CONFIG_DEBUG_SECTION_MISMATCH=y' Actually doing that gives you an output like this (currently on exynos_defconfig): $ make CONFIG_DEBUG_SECTION_MISMATCH=y WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch in reference from the function samsung_pinctrl_probe() to the function .init.text:samsung_gpiolib_register() The function __devinit samsung_pinctrl_probe() references a function __init samsung_gpiolib_register(). If samsung_gpiolib_register is only used by samsung_pinctrl_probe then annotate samsung_gpiolib_register with a matching annotation. or like this (now fixed in socfpga_defconfig): WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt() The function stmmac_pltfr_probe() references the function __devinit stmmac_probe_config_dt(). This is often because stmmac_pltfr_probe lacks a __devinit annotation or the annotation of stmmac_probe_config_dt is wrong. I believe you normally don't get warnings for functions that could be marked __devinit and only call regular functions, but there are a couple of __devinit infrastructure functions that you can't call from a function that isn't __init or __devinit. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 11:50:21AM +, Arnd Bergmann wrote:
> On Monday 22 October 2012, Thierry Reding wrote:
> > On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
> > > Replies to your comments inline:
> > >
> > > On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
> > > ...
> > > > > -static int __devinit pwm_probe(struct platform_device *pdev)
> > > > > +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> > > > > + { .compatible = "via,vt8500-pwm", },
> > > > > + { /* Sentinel */ }
> > > > > +};
> > > > > +
> > > > > +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
> > > >
> > > > Since you're changing this line anyway, maybe you should drop __devinit
> > > > (and __devexit for the .remove() callback). HOTPLUG is always enabled
> > > > nowadays and will go away eventually, in which case these will need to
> > > > be removed anyway.
> > >
> > > Will do. I must say the inconstancy among comments is rather
> > > frustrating. In another patch I sent out a few days ago (completely
> > > unrelated to this), I told to add __devexit to a remove() function :\
> >
> > This is a rather recent development, so maybe not everyone knows about
> > it yet. You can look at the following commit for the details:
> >
> > 45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2
> >
> > It's been in linux-next for about 6 weeks and has also gone into
> > 3.7-rc1.
>
> As long as we get build warnings for leaving out the __devinit/__devexit
> annotations, I would generally recommend putting them in. If we do a
> patch to remove all of them, a couple extra instances will not cause
> any more troubles than we already have.
I've never seen any build warnings for leaving __devinit/__devexit out.
Where does that happen?
Thierry
pgpC4rKOLX4xc.pgp
Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Monday 22 October 2012, Thierry Reding wrote:
> On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
> > Replies to your comments inline:
> >
> > On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
> > ...
> > > > -static int __devinit pwm_probe(struct platform_device *pdev)
> > > > +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> > > > + { .compatible = "via,vt8500-pwm", },
> > > > + { /* Sentinel */ }
> > > > +};
> > > > +
> > > > +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
> > >
> > > Since you're changing this line anyway, maybe you should drop __devinit
> > > (and __devexit for the .remove() callback). HOTPLUG is always enabled
> > > nowadays and will go away eventually, in which case these will need to
> > > be removed anyway.
> >
> > Will do. I must say the inconstancy among comments is rather
> > frustrating. In another patch I sent out a few days ago (completely
> > unrelated to this), I told to add __devexit to a remove() function :\
>
> This is a rather recent development, so maybe not everyone knows about
> it yet. You can look at the following commit for the details:
>
> 45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2
>
> It's been in linux-next for about 6 weeks and has also gone into
> 3.7-rc1.
As long as we get build warnings for leaving out the __devinit/__devexit
annotations, I would generally recommend putting them in. If we do a
patch to remove all of them, a couple extra instances will not cause
any more troubles than we already have.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
> On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
> > On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> > > On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > > >
> > > > > > chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
> > > > > > if (chip == NULL) {
> > > > > > dev_err(>dev, "failed to allocate memory\n");
> > > > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
> > > > > > platform_device *pdev)
> > > > > > chip->chip.ops = _pwm_ops;
> > > > > > chip->chip.base = -1;
> > > > > > chip->chip.npwm = VT8500_NR_PWMS;
> > > > > > + chip->clk = of_clk_get(np, 0);
> > > > >
> > > > > I thought this was supposed to work transparently across OF and !OF
> > > > > configurations by using just clk_get() or devm_clk_get()? I guess that
> > > > > if the driver depends on OF, then this would be moot, but we should
> > > > > probably stick to the standard usage anyway.
> > > > >
> > > > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > > > > add explicit clk_put() in the error cleanup paths. One more argument
> > > > > in
> > > > > favour of using devm_clk_get() instead.
> > > >
> > > > Hmm good point. I stuck with of_ functions because its an OF only driver
> > > > and it seemed 'backward' to mix old code with new. It does pose the
> > > > question of 'why have of_clk_get() if existing functions work better'.
> > >
> > > Was about to fix this but noticed why it wasn't like this to start
> > > with :)
> > >
> > > struct clk *devm_clk_get(struct device *dev, const char *id);
> > > struct clk *of_clk_get(struct device_node *np, int index);
> > >
> > > devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> > > believe a lot of other arch's) don't enforce names for clocks defined in
> > > devicetree, therefore there is no way for me to know what name the clk
> > > has unless I include in the binding that the clock must be named 'xxx'.
> >
> > I thought clk_get() was supposed to return the first clock specified in
> > DT if you pass NULL as the consumer name. I haven't tested this though.
> > And I haven't looked at the code.
> >
> > > of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> > > long as its the 1st clock listed.
> >
> > So the usual way to do this, I believe, is:
> >
> > clocks = <_foo>;
> > clock-names = "foo";
> >
> > Then use:
> >
> > clk = devm_clk_get(>dev, "foo");
> >
> > And as I said above, I was under the impression that the default would
> > be to use the first clock if NULL was specified instead of "foo".
> >
> > Thierry
>
> clock-names is an optional property (as defined in
> bindings/clock/clock-bindings.txt) so relying on it is .. well,
> unreliable.
>
> What you say makes sense, but it means the binding document has to make
> an optional property into a required property simply to use an 'old'
> function when a new function would 'work' (granted not as well, as you
> pointed out) without requiring the optional property.
Okay, I've just checked the core clock code, and indeed if you run
clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
with index == 0. That's exactly what you want, right? The only setup
where this won't work out is if you need to handle multiple clocks, in
which case I think it would make sense to make the clock-names property
mandatory. But for this driver that won't be necessary, since it will
never use a second clock, right?
> Your subsystem - your rules. Let me know if I've managed to sway you or
> not :)
I'd rather persuade you than force the issue. =)
Thierry
pgpi8MK3GIIzW.pgp
Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
> On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> > On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > > >
> > > > > chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
> > > > > if (chip == NULL) {
> > > > > dev_err(>dev, "failed to allocate memory\n");
> > > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
> > > > > platform_device *pdev)
> > > > > chip->chip.ops = _pwm_ops;
> > > > > chip->chip.base = -1;
> > > > > chip->chip.npwm = VT8500_NR_PWMS;
> > > > > + chip->clk = of_clk_get(np, 0);
> > > >
> > > > I thought this was supposed to work transparently across OF and !OF
> > > > configurations by using just clk_get() or devm_clk_get()? I guess that
> > > > if the driver depends on OF, then this would be moot, but we should
> > > > probably stick to the standard usage anyway.
> > > >
> > > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > > > add explicit clk_put() in the error cleanup paths. One more argument in
> > > > favour of using devm_clk_get() instead.
> > >
> > > Hmm good point. I stuck with of_ functions because its an OF only driver
> > > and it seemed 'backward' to mix old code with new. It does pose the
> > > question of 'why have of_clk_get() if existing functions work better'.
> >
> > Was about to fix this but noticed why it wasn't like this to start
> > with :)
> >
> > struct clk *devm_clk_get(struct device *dev, const char *id);
> > struct clk *of_clk_get(struct device_node *np, int index);
> >
> > devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> > believe a lot of other arch's) don't enforce names for clocks defined in
> > devicetree, therefore there is no way for me to know what name the clk
> > has unless I include in the binding that the clock must be named 'xxx'.
>
> I thought clk_get() was supposed to return the first clock specified in
> DT if you pass NULL as the consumer name. I haven't tested this though.
> And I haven't looked at the code.
>
> > of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> > long as its the 1st clock listed.
>
> So the usual way to do this, I believe, is:
>
> clocks = <_foo>;
> clock-names = "foo";
>
> Then use:
>
> clk = devm_clk_get(>dev, "foo");
>
> And as I said above, I was under the impression that the default would
> be to use the first clock if NULL was specified instead of "foo".
>
> Thierry
clock-names is an optional property (as defined in
bindings/clock/clock-bindings.txt) so relying on it is .. well,
unreliable.
What you say makes sense, but it means the binding document has to make
an optional property into a required property simply to use an 'old'
function when a new function would 'work' (granted not as well, as you
pointed out) without requiring the optional property.
Your subsystem - your rules. Let me know if I've managed to sway you or
not :)
Regards
Tony P
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
> On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> > >
> > > > chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
> > > > if (chip == NULL) {
> > > > dev_err(>dev, "failed to allocate memory\n");
> > > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
> > > > platform_device *pdev)
> > > > chip->chip.ops = _pwm_ops;
> > > > chip->chip.base = -1;
> > > > chip->chip.npwm = VT8500_NR_PWMS;
> > > > + chip->clk = of_clk_get(np, 0);
> > >
> > > I thought this was supposed to work transparently across OF and !OF
> > > configurations by using just clk_get() or devm_clk_get()? I guess that
> > > if the driver depends on OF, then this would be moot, but we should
> > > probably stick to the standard usage anyway.
> > >
> > > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > > add explicit clk_put() in the error cleanup paths. One more argument in
> > > favour of using devm_clk_get() instead.
> >
> > Hmm good point. I stuck with of_ functions because its an OF only driver
> > and it seemed 'backward' to mix old code with new. It does pose the
> > question of 'why have of_clk_get() if existing functions work better'.
>
> Was about to fix this but noticed why it wasn't like this to start
> with :)
>
> struct clk *devm_clk_get(struct device *dev, const char *id);
> struct clk *of_clk_get(struct device_node *np, int index);
>
> devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
> believe a lot of other arch's) don't enforce names for clocks defined in
> devicetree, therefore there is no way for me to know what name the clk
> has unless I include in the binding that the clock must be named 'xxx'.
I thought clk_get() was supposed to return the first clock specified in
DT if you pass NULL as the consumer name. I haven't tested this though.
And I haven't looked at the code.
> of_clk_get retrieves it by the dt-node + index, so it doesn't care as
> long as its the 1st clock listed.
So the usual way to do this, I believe, is:
clocks = <_foo>;
clock-names = "foo";
Then use:
clk = devm_clk_get(>dev, "foo");
And as I said above, I was under the impression that the default would
be to use the first clock if NULL was specified instead of "foo".
Thierry
pgpZZtzRRFCjG.pgp
Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
> Replies to your comments inline:
>
> On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
> ...
> > > -static int __devinit pwm_probe(struct platform_device *pdev)
> > > +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> > > + { .compatible = "via,vt8500-pwm", },
> > > + { /* Sentinel */ }
> > > +};
> > > +
> > > +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
> >
> > Since you're changing this line anyway, maybe you should drop __devinit
> > (and __devexit for the .remove() callback). HOTPLUG is always enabled
> > nowadays and will go away eventually, in which case these will need to
> > be removed anyway.
>
> Will do. I must say the inconstancy among comments is rather
> frustrating. In another patch I sent out a few days ago (completely
> unrelated to this), I told to add __devexit to a remove() function :\
This is a rather recent development, so maybe not everyone knows about
it yet. You can look at the following commit for the details:
45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2
It's been in linux-next for about 6 weeks and has also gone into
3.7-rc1.
> > > {
> > > struct vt8500_chip *chip;
> > > - struct resource *r;
> > > + struct device_node *np = pdev->dev.of_node;
> > > int ret;
> > >
> > > + if (!np) {
> > > + dev_err(>dev, "invalid devicetree node\n");
> > > + return -EINVAL;
> > > + }
> > > +
> >
> > This effectively makes DT support mandatory. Shouldn't you be adding a
> > "depends on OF" into the Kconfig section in that case?
> This driver depends on ARCH_VT8500, which only supports DT so a
> dependency on OF seemed redundant. If you think its still necessary, let
> me know and I'll add it anyway.
Okay, in that case you don't need another dependency. I've recently seen
comments about the check for !np being unnecessary because it can't be
NULL if you depend on OF. I suppose there's some truth in it but to be
honest I haven't made up my mind about it yet.
> > > chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
> > > if (chip == NULL) {
> > > dev_err(>dev, "failed to allocate memory\n");
> > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
> > > platform_device *pdev)
> > > chip->chip.ops = _pwm_ops;
> > > chip->chip.base = -1;
> > > chip->chip.npwm = VT8500_NR_PWMS;
> > > + chip->clk = of_clk_get(np, 0);
> >
> > I thought this was supposed to work transparently across OF and !OF
> > configurations by using just clk_get() or devm_clk_get()? I guess that
> > if the driver depends on OF, then this would be moot, but we should
> > probably stick to the standard usage anyway.
> >
> > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > add explicit clk_put() in the error cleanup paths. One more argument in
> > favour of using devm_clk_get() instead.
>
> Hmm good point. I stuck with of_ functions because its an OF only driver
> and it seemed 'backward' to mix old code with new. It does pose the
> question of 'why have of_clk_get() if existing functions work better'.
I think wherever possible you should use the generic calls, since they
are (usually) explicitly designed to work with OF and non-OF and you
never know what your driver might end up being used for. Then again, if
the driver is VT8500 specific and VT8500 doesn't support anything but
device-tree I suppose using the of_*() functions would be okay. Maybe
adding devm_of_clk_get() would be a worthwhile addition.
You should also consider that other people may look at your driver as a
reference and may end up using the OF-only variants even if their driver
is used in non-DT setups. I'm not sure how valid that argument is,
though, since code review is supposed to catch those mistakes.
[...]
> > > -MODULE_LICENSE("GPL");
> > > +MODULE_DESCRIPTION("VT8500 PWM Driver");
> > > +MODULE_AUTHOR("Tony Prisk ");
> > > +MODULE_LICENSE("GPL v2");
> >
> > IANAL, but I think you need the approval of all authors of this code
> > before changing the license. But I see that the file header actually
> > says that this code is GPL v2, so maybe this change could be considered
> > a bugfix. =)
>
> This is something I've already discussed with Alexey in regards to all
> the existing drivers he has in mainline. Since I have taken over as
> maintainer on this platform I have corrected the licenses as patch's
> have gone through. As you pointed out, it was already GPLv2 in the
> header, this is just a 'bugfix'.
Okay, works for me.
Thierry
pgpLRq4CNqFQL.pgp
Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
> >
> > > chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
> > > if (chip == NULL) {
> > > dev_err(>dev, "failed to allocate memory\n");
> > > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
> > > platform_device *pdev)
> > > chip->chip.ops = _pwm_ops;
> > > chip->chip.base = -1;
> > > chip->chip.npwm = VT8500_NR_PWMS;
> > > + chip->clk = of_clk_get(np, 0);
> >
> > I thought this was supposed to work transparently across OF and !OF
> > configurations by using just clk_get() or devm_clk_get()? I guess that
> > if the driver depends on OF, then this would be moot, but we should
> > probably stick to the standard usage anyway.
> >
> > Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> > add explicit clk_put() in the error cleanup paths. One more argument in
> > favour of using devm_clk_get() instead.
>
> Hmm good point. I stuck with of_ functions because its an OF only driver
> and it seemed 'backward' to mix old code with new. It does pose the
> question of 'why have of_clk_get() if existing functions work better'.
Was about to fix this but noticed why it wasn't like this to start
with :)
struct clk *devm_clk_get(struct device *dev, const char *id);
struct clk *of_clk_get(struct device_node *np, int index);
devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
believe a lot of other arch's) don't enforce names for clocks defined in
devicetree, therefore there is no way for me to know what name the clk
has unless I include in the binding that the clock must be named 'xxx'.
of_clk_get retrieves it by the dt-node + index, so it doesn't care as
long as its the 1st clock listed.
Welcome your feedback.
Regards
Tony P
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
Replies to your comments inline:
On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
...
> > -static int __devinit pwm_probe(struct platform_device *pdev)
> > +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> > + { .compatible = "via,vt8500-pwm", },
> > + { /* Sentinel */ }
> > +};
> > +
> > +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
>
> Since you're changing this line anyway, maybe you should drop __devinit
> (and __devexit for the .remove() callback). HOTPLUG is always enabled
> nowadays and will go away eventually, in which case these will need to
> be removed anyway.
Will do. I must say the inconstancy among comments is rather
frustrating. In another patch I sent out a few days ago (completely
unrelated to this), I told to add __devexit to a remove() function :\
> > {
> > struct vt8500_chip *chip;
> > - struct resource *r;
> > + struct device_node *np = pdev->dev.of_node;
> > int ret;
> >
> > + if (!np) {
> > + dev_err(>dev, "invalid devicetree node\n");
> > + return -EINVAL;
> > + }
> > +
>
> This effectively makes DT support mandatory. Shouldn't you be adding a
> "depends on OF" into the Kconfig section in that case?
This driver depends on ARCH_VT8500, which only supports DT so a
dependency on OF seemed redundant. If you think its still necessary, let
me know and I'll add it anyway.
>
> > chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
> > if (chip == NULL) {
> > dev_err(>dev, "failed to allocate memory\n");
> > @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device
> > *pdev)
> > chip->chip.ops = _pwm_ops;
> > chip->chip.base = -1;
> > chip->chip.npwm = VT8500_NR_PWMS;
> > + chip->clk = of_clk_get(np, 0);
>
> I thought this was supposed to work transparently across OF and !OF
> configurations by using just clk_get() or devm_clk_get()? I guess that
> if the driver depends on OF, then this would be moot, but we should
> probably stick to the standard usage anyway.
>
> Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
> add explicit clk_put() in the error cleanup paths. One more argument in
> favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
>
> > - r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > - if (r == NULL) {
> > - dev_err(>dev, "no memory resource defined\n");
> > - return -ENODEV;
> > + if (!chip->clk) {
> > + dev_err(>dev, "clock source not specified\n");
> > + return -EINVAL;
> > }
> >
> > - chip->base = devm_request_and_ioremap(>dev, r);
> > - if (chip->base == NULL)
> > + chip->base = of_iomap(np, 0);
>
> No need to change this. It should work with the standard calls as well.
Again, this was a conversion of use of_ functions rather than the 'old'
style.
>
> > + if (!chip->base) {
> > + dev_err(>dev, "memory resource not available\n");
> > return -EADDRNOTAVAIL;
> > + }
> > +
> > + clk_prepare_enable(chip->clk);
>
> Why does the clock need to be enabled here? Shouldn't it be postponed to
> the last possible moment to save power?
I didn't consider that - but the use case for everyone at present is
that they only need the PWM driver to control the backlight, and it's
going to be enabled at boot anyway - so one PWM will always be active.
Futureproofing is always good so I'll fix this.
...
> >
> > -MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("VT8500 PWM Driver");
> > +MODULE_AUTHOR("Tony Prisk ");
> > +MODULE_LICENSE("GPL v2");
>
> IANAL, but I think you need the approval of all authors of this code
> before changing the license. But I see that the file header actually
> says that this code is GPL v2, so maybe this change could be considered
> a bugfix. =)
This is something I've already discussed with Alexey in regards to all
the existing drivers he has in mainline. Since I have taken over as
maintainer on this platform I have corrected the licenses as patch's
have gone through. As you pointed out, it was already GPLv2 in the
header, this is just a 'bugfix'.
>
> > +MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
>
> I think it is customary to put this right after the device table
> definition.
Didn't know that - will fix.
>
> > --
> > 1.7.9.5
> >
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Fri, Oct 19, 2012 at 11:38:54PM +1300, Tony Prisk wrote:
> This patch updates pwm-vt8500.c to support devicetree probing and
> make use of the common clock subsystem.
>
> Signed-off-by: Tony Prisk
> ---
> drivers/pwm/pwm-vt8500.c | 79
> ++
> 1 file changed, 51 insertions(+), 28 deletions(-)
On the whole, this looks like a great cleanup. A few minor issues
inline.
I should start with the subject: please keep lowercase pwm: as the
prefix for consistency. Uppercase PWM is used to refer to PWM devices in
prose.
Also I'd rather see the clock subsystem changes and device tree changes
in separate patches, but since both are rather intertwined I won't force
the issue.
> diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
> index ad14389..c2a71ee 100644
> --- a/drivers/pwm/pwm-vt8500.c
> +++ b/drivers/pwm/pwm-vt8500.c
> @@ -1,7 +1,8 @@
> /*
> * drivers/pwm/pwm-vt8500.c
> *
> - * Copyright (C) 2010 Alexey Charkov
> + * Copyright (C) 2012 Tony Prisk
> + * Copyright (C) 2010 Alexey Charkov
> *
> * This software is licensed under the terms of the GNU General Public
> * License version 2, as published by the Free Software Foundation, and
> @@ -21,14 +22,24 @@
> #include
> #include
> #include
> +#include
>
> #include
>
> -#define VT8500_NR_PWMS 4
> +#include
> +#include
> +#include
> +
> +/*
> + * SoC architecture allocates register space for 4 PWMs but only
> + * 2 are currently implemented.
> + */
> +#define VT8500_NR_PWMS 2
>
> struct vt8500_chip {
> struct pwm_chip chip;
> void __iomem *base;
> + struct clk *clk;
> };
>
> #define to_vt8500_chip(chip) container_of(chip, struct vt8500_chip, chip)
> @@ -52,7 +63,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct
> pwm_device *pwm,
> unsigned long long c;
> unsigned long period_cycles, prescale, pv, dc;
>
> - c = 2500/2; /* wild guess --- need to implement clocks */
> + c = clk_get_rate(vt8500->clk);
> c = c * period_ns;
> do_div(c, 10);
> period_cycles = c;
> @@ -107,12 +118,22 @@ static struct pwm_ops vt8500_pwm_ops = {
> .owner = THIS_MODULE,
> };
>
> -static int __devinit pwm_probe(struct platform_device *pdev)
> +static const struct of_device_id vt8500_pwm_dt_ids[] = {
> + { .compatible = "via,vt8500-pwm", },
> + { /* Sentinel */ }
> +};
> +
> +static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
Since you're changing this line anyway, maybe you should drop __devinit
(and __devexit for the .remove() callback). HOTPLUG is always enabled
nowadays and will go away eventually, in which case these will need to
be removed anyway.
> {
> struct vt8500_chip *chip;
> - struct resource *r;
> + struct device_node *np = pdev->dev.of_node;
> int ret;
>
> + if (!np) {
> + dev_err(>dev, "invalid devicetree node\n");
> + return -EINVAL;
> + }
> +
This effectively makes DT support mandatory. Shouldn't you be adding a
"depends on OF" into the Kconfig section in that case?
> chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
> if (chip == NULL) {
> dev_err(>dev, "failed to allocate memory\n");
> @@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device
> *pdev)
> chip->chip.ops = _pwm_ops;
> chip->chip.base = -1;
> chip->chip.npwm = VT8500_NR_PWMS;
> + chip->clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument in
favour of using devm_clk_get() instead.
> - r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (r == NULL) {
> - dev_err(>dev, "no memory resource defined\n");
> - return -ENODEV;
> + if (!chip->clk) {
> + dev_err(>dev, "clock source not specified\n");
> + return -EINVAL;
> }
>
> - chip->base = devm_request_and_ioremap(>dev, r);
> - if (chip->base == NULL)
> + chip->base = of_iomap(np, 0);
No need to change this. It should work with the standard calls as well.
> + if (!chip->base) {
> + dev_err(>dev, "memory resource not available\n");
> return -EADDRNOTAVAIL;
> + }
> +
> + clk_prepare_enable(chip->clk);
Why does the clock need to be enabled here? Shouldn't it be postponed to
the last possible moment to save power?
>
> ret = pwmchip_add(>chip);
> - if (ret < 0)
> + if (ret < 0) {
> + dev_err(>dev, "failed to add pwmchip\n");
> return ret;
> + }
>
> platform_set_drvdata(pdev, chip);
>
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Fri, Oct 19, 2012 at 11:38:54PM +1300, Tony Prisk wrote:
This patch updates pwm-vt8500.c to support devicetree probing and
make use of the common clock subsystem.
Signed-off-by: Tony Prisk li...@prisktech.co.nz
---
drivers/pwm/pwm-vt8500.c | 79
++
1 file changed, 51 insertions(+), 28 deletions(-)
On the whole, this looks like a great cleanup. A few minor issues
inline.
I should start with the subject: please keep lowercase pwm: as the
prefix for consistency. Uppercase PWM is used to refer to PWM devices in
prose.
Also I'd rather see the clock subsystem changes and device tree changes
in separate patches, but since both are rather intertwined I won't force
the issue.
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index ad14389..c2a71ee 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -1,7 +1,8 @@
/*
* drivers/pwm/pwm-vt8500.c
*
- * Copyright (C) 2010 Alexey Charkov alch...@gmail.com
+ * Copyright (C) 2012 Tony Prisk li...@prisktech.co.nz
+ * Copyright (C) 2010 Alexey Charkov alch...@gmail.com
*
* This software is licensed under the terms of the GNU General Public
* License version 2, as published by the Free Software Foundation, and
@@ -21,14 +22,24 @@
#include linux/io.h
#include linux/pwm.h
#include linux/delay.h
+#include linux/clk.h
#include asm/div64.h
-#define VT8500_NR_PWMS 4
+#include linux/of.h
+#include linux/of_device.h
+#include linux/of_address.h
+
+/*
+ * SoC architecture allocates register space for 4 PWMs but only
+ * 2 are currently implemented.
+ */
+#define VT8500_NR_PWMS 2
struct vt8500_chip {
struct pwm_chip chip;
void __iomem *base;
+ struct clk *clk;
};
#define to_vt8500_chip(chip) container_of(chip, struct vt8500_chip, chip)
@@ -52,7 +63,7 @@ static int vt8500_pwm_config(struct pwm_chip *chip, struct
pwm_device *pwm,
unsigned long long c;
unsigned long period_cycles, prescale, pv, dc;
- c = 2500/2; /* wild guess --- need to implement clocks */
+ c = clk_get_rate(vt8500-clk);
c = c * period_ns;
do_div(c, 10);
period_cycles = c;
@@ -107,12 +118,22 @@ static struct pwm_ops vt8500_pwm_ops = {
.owner = THIS_MODULE,
};
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+ { .compatible = via,vt8500-pwm, },
+ { /* Sentinel */ }
+};
+
+static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
Since you're changing this line anyway, maybe you should drop __devinit
(and __devexit for the .remove() callback). HOTPLUG is always enabled
nowadays and will go away eventually, in which case these will need to
be removed anyway.
{
struct vt8500_chip *chip;
- struct resource *r;
+ struct device_node *np = pdev-dev.of_node;
int ret;
+ if (!np) {
+ dev_err(pdev-dev, invalid devicetree node\n);
+ return -EINVAL;
+ }
+
This effectively makes DT support mandatory. Shouldn't you be adding a
depends on OF into the Kconfig section in that case?
chip = devm_kzalloc(pdev-dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device
*pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument in
favour of using devm_clk_get() instead.
- r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (r == NULL) {
- dev_err(pdev-dev, no memory resource defined\n);
- return -ENODEV;
+ if (!chip-clk) {
+ dev_err(pdev-dev, clock source not specified\n);
+ return -EINVAL;
}
- chip-base = devm_request_and_ioremap(pdev-dev, r);
- if (chip-base == NULL)
+ chip-base = of_iomap(np, 0);
No need to change this. It should work with the standard calls as well.
+ if (!chip-base) {
+ dev_err(pdev-dev, memory resource not available\n);
return -EADDRNOTAVAIL;
+ }
+
+ clk_prepare_enable(chip-clk);
Why does the clock need to be enabled here? Shouldn't it be postponed to
the last possible moment to save power?
ret = pwmchip_add(chip-chip);
- if (ret 0)
+ if (ret 0) {
+ dev_err(pdev-dev, failed to add pwmchip\n);
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
Replies to your comments inline:
On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
...
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+ { .compatible = via,vt8500-pwm, },
+ { /* Sentinel */ }
+};
+
+static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
Since you're changing this line anyway, maybe you should drop __devinit
(and __devexit for the .remove() callback). HOTPLUG is always enabled
nowadays and will go away eventually, in which case these will need to
be removed anyway.
Will do. I must say the inconstancy among comments is rather
frustrating. In another patch I sent out a few days ago (completely
unrelated to this), I told to add __devexit to a remove() function :\
{
struct vt8500_chip *chip;
- struct resource *r;
+ struct device_node *np = pdev-dev.of_node;
int ret;
+ if (!np) {
+ dev_err(pdev-dev, invalid devicetree node\n);
+ return -EINVAL;
+ }
+
This effectively makes DT support mandatory. Shouldn't you be adding a
depends on OF into the Kconfig section in that case?
This driver depends on ARCH_VT8500, which only supports DT so a
dependency on OF seemed redundant. If you think its still necessary, let
me know and I'll add it anyway.
chip = devm_kzalloc(pdev-dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct platform_device
*pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument in
favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
- r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (r == NULL) {
- dev_err(pdev-dev, no memory resource defined\n);
- return -ENODEV;
+ if (!chip-clk) {
+ dev_err(pdev-dev, clock source not specified\n);
+ return -EINVAL;
}
- chip-base = devm_request_and_ioremap(pdev-dev, r);
- if (chip-base == NULL)
+ chip-base = of_iomap(np, 0);
No need to change this. It should work with the standard calls as well.
Again, this was a conversion of use of_ functions rather than the 'old'
style.
+ if (!chip-base) {
+ dev_err(pdev-dev, memory resource not available\n);
return -EADDRNOTAVAIL;
+ }
+
+ clk_prepare_enable(chip-clk);
Why does the clock need to be enabled here? Shouldn't it be postponed to
the last possible moment to save power?
I didn't consider that - but the use case for everyone at present is
that they only need the PWM driver to control the backlight, and it's
going to be enabled at boot anyway - so one PWM will always be active.
Futureproofing is always good so I'll fix this.
...
-MODULE_LICENSE(GPL);
+MODULE_DESCRIPTION(VT8500 PWM Driver);
+MODULE_AUTHOR(Tony Prisk li...@prisktech.co.nz);
+MODULE_LICENSE(GPL v2);
IANAL, but I think you need the approval of all authors of this code
before changing the license. But I see that the file header actually
says that this code is GPL v2, so maybe this change could be considered
a bugfix. =)
This is something I've already discussed with Alexey in regards to all
the existing drivers he has in mainline. Since I have taken over as
maintainer on this platform I have corrected the licenses as patch's
have gone through. As you pointed out, it was already GPLv2 in the
header, this is just a 'bugfix'.
+MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
I think it is customary to put this right after the device table
definition.
Didn't know that - will fix.
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
chip = devm_kzalloc(pdev-dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
platform_device *pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument in
favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
Was about to fix this but noticed why it wasn't like this to start
with :)
struct clk *devm_clk_get(struct device *dev, const char *id);
struct clk *of_clk_get(struct device_node *np, int index);
devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
believe a lot of other arch's) don't enforce names for clocks defined in
devicetree, therefore there is no way for me to know what name the clk
has unless I include in the binding that the clock must be named 'xxx'.
of_clk_get retrieves it by the dt-node + index, so it doesn't care as
long as its the 1st clock listed.
Welcome your feedback.
Regards
Tony P
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
Replies to your comments inline:
On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
...
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+ { .compatible = via,vt8500-pwm, },
+ { /* Sentinel */ }
+};
+
+static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
Since you're changing this line anyway, maybe you should drop __devinit
(and __devexit for the .remove() callback). HOTPLUG is always enabled
nowadays and will go away eventually, in which case these will need to
be removed anyway.
Will do. I must say the inconstancy among comments is rather
frustrating. In another patch I sent out a few days ago (completely
unrelated to this), I told to add __devexit to a remove() function :\
This is a rather recent development, so maybe not everyone knows about
it yet. You can look at the following commit for the details:
45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2
It's been in linux-next for about 6 weeks and has also gone into
3.7-rc1.
{
struct vt8500_chip *chip;
- struct resource *r;
+ struct device_node *np = pdev-dev.of_node;
int ret;
+ if (!np) {
+ dev_err(pdev-dev, invalid devicetree node\n);
+ return -EINVAL;
+ }
+
This effectively makes DT support mandatory. Shouldn't you be adding a
depends on OF into the Kconfig section in that case?
This driver depends on ARCH_VT8500, which only supports DT so a
dependency on OF seemed redundant. If you think its still necessary, let
me know and I'll add it anyway.
Okay, in that case you don't need another dependency. I've recently seen
comments about the check for !np being unnecessary because it can't be
NULL if you depend on OF. I suppose there's some truth in it but to be
honest I haven't made up my mind about it yet.
chip = devm_kzalloc(pdev-dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
platform_device *pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument in
favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
I think wherever possible you should use the generic calls, since they
are (usually) explicitly designed to work with OF and non-OF and you
never know what your driver might end up being used for. Then again, if
the driver is VT8500 specific and VT8500 doesn't support anything but
device-tree I suppose using the of_*() functions would be okay. Maybe
adding devm_of_clk_get() would be a worthwhile addition.
You should also consider that other people may look at your driver as a
reference and may end up using the OF-only variants even if their driver
is used in non-DT setups. I'm not sure how valid that argument is,
though, since code review is supposed to catch those mistakes.
[...]
-MODULE_LICENSE(GPL);
+MODULE_DESCRIPTION(VT8500 PWM Driver);
+MODULE_AUTHOR(Tony Prisk li...@prisktech.co.nz);
+MODULE_LICENSE(GPL v2);
IANAL, but I think you need the approval of all authors of this code
before changing the license. But I see that the file header actually
says that this code is GPL v2, so maybe this change could be considered
a bugfix. =)
This is something I've already discussed with Alexey in regards to all
the existing drivers he has in mainline. Since I have taken over as
maintainer on this platform I have corrected the licenses as patch's
have gone through. As you pointed out, it was already GPLv2 in the
header, this is just a 'bugfix'.
Okay, works for me.
Thierry
pgpLRq4CNqFQL.pgp
Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
chip = devm_kzalloc(pdev-dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
platform_device *pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument in
favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
Was about to fix this but noticed why it wasn't like this to start
with :)
struct clk *devm_clk_get(struct device *dev, const char *id);
struct clk *of_clk_get(struct device_node *np, int index);
devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
believe a lot of other arch's) don't enforce names for clocks defined in
devicetree, therefore there is no way for me to know what name the clk
has unless I include in the binding that the clock must be named 'xxx'.
I thought clk_get() was supposed to return the first clock specified in
DT if you pass NULL as the consumer name. I haven't tested this though.
And I haven't looked at the code.
of_clk_get retrieves it by the dt-node + index, so it doesn't care as
long as its the 1st clock listed.
So the usual way to do this, I believe, is:
clocks = clk_foo;
clock-names = foo;
Then use:
clk = devm_clk_get(pdev-dev, foo);
And as I said above, I was under the impression that the default would
be to use the first clock if NULL was specified instead of foo.
Thierry
pgpZZtzRRFCjG.pgp
Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
chip = devm_kzalloc(pdev-dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
platform_device *pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument in
favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
Was about to fix this but noticed why it wasn't like this to start
with :)
struct clk *devm_clk_get(struct device *dev, const char *id);
struct clk *of_clk_get(struct device_node *np, int index);
devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
believe a lot of other arch's) don't enforce names for clocks defined in
devicetree, therefore there is no way for me to know what name the clk
has unless I include in the binding that the clock must be named 'xxx'.
I thought clk_get() was supposed to return the first clock specified in
DT if you pass NULL as the consumer name. I haven't tested this though.
And I haven't looked at the code.
of_clk_get retrieves it by the dt-node + index, so it doesn't care as
long as its the 1st clock listed.
So the usual way to do this, I believe, is:
clocks = clk_foo;
clock-names = foo;
Then use:
clk = devm_clk_get(pdev-dev, foo);
And as I said above, I was under the impression that the default would
be to use the first clock if NULL was specified instead of foo.
Thierry
clock-names is an optional property (as defined in
bindings/clock/clock-bindings.txt) so relying on it is .. well,
unreliable.
What you say makes sense, but it means the binding document has to make
an optional property into a required property simply to use an 'old'
function when a new function would 'work' (granted not as well, as you
pointed out) without requiring the optional property.
Your subsystem - your rules. Let me know if I've managed to sway you or
not :)
Regards
Tony P
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 08:36:22PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 09:24 +0200, Thierry Reding wrote:
On Mon, Oct 22, 2012 at 08:09:07PM +1300, Tony Prisk wrote:
On Mon, 2012-10-22 at 19:51 +1300, Tony Prisk wrote:
chip = devm_kzalloc(pdev-dev, sizeof(*chip), GFP_KERNEL);
if (chip == NULL) {
dev_err(pdev-dev, failed to allocate memory\n);
@@ -123,26 +144,32 @@ static int __devinit pwm_probe(struct
platform_device *pdev)
chip-chip.ops = vt8500_pwm_ops;
chip-chip.base = -1;
chip-chip.npwm = VT8500_NR_PWMS;
+ chip-clk = of_clk_get(np, 0);
I thought this was supposed to work transparently across OF and !OF
configurations by using just clk_get() or devm_clk_get()? I guess that
if the driver depends on OF, then this would be moot, but we should
probably stick to the standard usage anyway.
Furthermore, of_clk_get() doesn't seem to be managed, so you'd need to
add explicit clk_put() in the error cleanup paths. One more argument
in
favour of using devm_clk_get() instead.
Hmm good point. I stuck with of_ functions because its an OF only driver
and it seemed 'backward' to mix old code with new. It does pose the
question of 'why have of_clk_get() if existing functions work better'.
Was about to fix this but noticed why it wasn't like this to start
with :)
struct clk *devm_clk_get(struct device *dev, const char *id);
struct clk *of_clk_get(struct device_node *np, int index);
devm_clk_get requires me to 'get' the clock by name. arch-vt8500 (and I
believe a lot of other arch's) don't enforce names for clocks defined in
devicetree, therefore there is no way for me to know what name the clk
has unless I include in the binding that the clock must be named 'xxx'.
I thought clk_get() was supposed to return the first clock specified in
DT if you pass NULL as the consumer name. I haven't tested this though.
And I haven't looked at the code.
of_clk_get retrieves it by the dt-node + index, so it doesn't care as
long as its the 1st clock listed.
So the usual way to do this, I believe, is:
clocks = clk_foo;
clock-names = foo;
Then use:
clk = devm_clk_get(pdev-dev, foo);
And as I said above, I was under the impression that the default would
be to use the first clock if NULL was specified instead of foo.
Thierry
clock-names is an optional property (as defined in
bindings/clock/clock-bindings.txt) so relying on it is .. well,
unreliable.
What you say makes sense, but it means the binding document has to make
an optional property into a required property simply to use an 'old'
function when a new function would 'work' (granted not as well, as you
pointed out) without requiring the optional property.
Okay, I've just checked the core clock code, and indeed if you run
clk_get() with con_id set to NULL, you'll eventually call of_clk_get()
with index == 0. That's exactly what you want, right? The only setup
where this won't work out is if you need to handle multiple clocks, in
which case I think it would make sense to make the clock-names property
mandatory. But for this driver that won't be necessary, since it will
never use a second clock, right?
Your subsystem - your rules. Let me know if I've managed to sway you or
not :)
I'd rather persuade you than force the issue. =)
Thierry
pgpi8MK3GIIzW.pgp
Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Monday 22 October 2012, Thierry Reding wrote:
On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
Replies to your comments inline:
On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
...
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+ { .compatible = via,vt8500-pwm, },
+ { /* Sentinel */ }
+};
+
+static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
Since you're changing this line anyway, maybe you should drop __devinit
(and __devexit for the .remove() callback). HOTPLUG is always enabled
nowadays and will go away eventually, in which case these will need to
be removed anyway.
Will do. I must say the inconstancy among comments is rather
frustrating. In another patch I sent out a few days ago (completely
unrelated to this), I told to add __devexit to a remove() function :\
This is a rather recent development, so maybe not everyone knows about
it yet. You can look at the following commit for the details:
45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2
It's been in linux-next for about 6 weeks and has also gone into
3.7-rc1.
As long as we get build warnings for leaving out the __devinit/__devexit
annotations, I would generally recommend putting them in. If we do a
patch to remove all of them, a couple extra instances will not cause
any more troubles than we already have.
Arnd
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 11:50:21AM +, Arnd Bergmann wrote:
On Monday 22 October 2012, Thierry Reding wrote:
On Mon, Oct 22, 2012 at 07:51:52PM +1300, Tony Prisk wrote:
Replies to your comments inline:
On Mon, 2012-10-22 at 08:34 +0200, Thierry Reding wrote:
...
-static int __devinit pwm_probe(struct platform_device *pdev)
+static const struct of_device_id vt8500_pwm_dt_ids[] = {
+ { .compatible = via,vt8500-pwm, },
+ { /* Sentinel */ }
+};
+
+static int __devinit vt8500_pwm_probe(struct platform_device *pdev)
Since you're changing this line anyway, maybe you should drop __devinit
(and __devexit for the .remove() callback). HOTPLUG is always enabled
nowadays and will go away eventually, in which case these will need to
be removed anyway.
Will do. I must say the inconstancy among comments is rather
frustrating. In another patch I sent out a few days ago (completely
unrelated to this), I told to add __devexit to a remove() function :\
This is a rather recent development, so maybe not everyone knows about
it yet. You can look at the following commit for the details:
45f035ab9b8f45aaf1eb2213218b7e9c14af3fc2
It's been in linux-next for about 6 weeks and has also gone into
3.7-rc1.
As long as we get build warnings for leaving out the __devinit/__devexit
annotations, I would generally recommend putting them in. If we do a
patch to remove all of them, a couple extra instances will not cause
any more troubles than we already have.
I've never seen any build warnings for leaving __devinit/__devexit out.
Where does that happen?
Thierry
pgpC4rKOLX4xc.pgp
Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Monday 22 October 2012, Thierry Reding wrote: As long as we get build warnings for leaving out the __devinit/__devexit annotations, I would generally recommend putting them in. If we do a patch to remove all of them, a couple extra instances will not cause any more troubles than we already have. I've never seen any build warnings for leaving __devinit/__devexit out. Where does that happen? Section mismatches usually result into warnings from modpost, like WARNING: modpost: Found 1 section mismatch(es). To see full details build your kernel with: 'make CONFIG_DEBUG_SECTION_MISMATCH=y' Actually doing that gives you an output like this (currently on exynos_defconfig): $ make CONFIG_DEBUG_SECTION_MISMATCH=y WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch in reference from the function samsung_pinctrl_probe() to the function .init.text:samsung_gpiolib_register() The function __devinit samsung_pinctrl_probe() references a function __init samsung_gpiolib_register(). If samsung_gpiolib_register is only used by samsung_pinctrl_probe then annotate samsung_gpiolib_register with a matching annotation. or like this (now fixed in socfpga_defconfig): WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt() The function stmmac_pltfr_probe() references the function __devinit stmmac_probe_config_dt(). This is often because stmmac_pltfr_probe lacks a __devinit annotation or the annotation of stmmac_probe_config_dt is wrong. I believe you normally don't get warnings for functions that could be marked __devinit and only call regular functions, but there are a couple of __devinit infrastructure functions that you can't call from a function that isn't __init or __devinit. Arnd -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, Oct 22, 2012 at 01:52:08PM +, Arnd Bergmann wrote: On Monday 22 October 2012, Thierry Reding wrote: As long as we get build warnings for leaving out the __devinit/__devexit annotations, I would generally recommend putting them in. If we do a patch to remove all of them, a couple extra instances will not cause any more troubles than we already have. I've never seen any build warnings for leaving __devinit/__devexit out. Where does that happen? Section mismatches usually result into warnings from modpost, like WARNING: modpost: Found 1 section mismatch(es). To see full details build your kernel with: 'make CONFIG_DEBUG_SECTION_MISMATCH=y' Actually doing that gives you an output like this (currently on exynos_defconfig): $ make CONFIG_DEBUG_SECTION_MISMATCH=y WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch in reference from the function samsung_pinctrl_probe() to the function .init.text:samsung_gpiolib_register() The function __devinit samsung_pinctrl_probe() references a function __init samsung_gpiolib_register(). If samsung_gpiolib_register is only used by samsung_pinctrl_probe then annotate samsung_gpiolib_register with a matching annotation. or like this (now fixed in socfpga_defconfig): WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt() The function stmmac_pltfr_probe() references the function __devinit stmmac_probe_config_dt(). This is often because stmmac_pltfr_probe lacks a __devinit annotation or the annotation of stmmac_probe_config_dt is wrong. I believe you normally don't get warnings for functions that could be marked __devinit and only call regular functions, but there are a couple of __devinit infrastructure functions that you can't call from a function that isn't __init or __devinit. Right. If you get those warnings you shouldn't be dropping the annotations. But I don't think that is the case for this driver. Tony, can you confirm that the driver still builds properly without warnings if you drop the __devinit/__devexit? Thierry pgp7NZLqFpoFN.pgp Description: PGP signature
Re: [PATCH 2/3] PWM: vt8500: Update vt8500 PWM driver support
On Mon, 2012-10-22 at 17:08 +0200, Thierry Reding wrote: On Mon, Oct 22, 2012 at 01:52:08PM +, Arnd Bergmann wrote: On Monday 22 October 2012, Thierry Reding wrote: As long as we get build warnings for leaving out the __devinit/__devexit annotations, I would generally recommend putting them in. If we do a patch to remove all of them, a couple extra instances will not cause any more troubles than we already have. I've never seen any build warnings for leaving __devinit/__devexit out. Where does that happen? Section mismatches usually result into warnings from modpost, like WARNING: modpost: Found 1 section mismatch(es). To see full details build your kernel with: 'make CONFIG_DEBUG_SECTION_MISMATCH=y' Actually doing that gives you an output like this (currently on exynos_defconfig): $ make CONFIG_DEBUG_SECTION_MISMATCH=y WARNING: drivers/pinctrl/built-in.o(.devinit.text+0x124): Section mismatch in reference from the function samsung_pinctrl_probe() to the function .init.text:samsung_gpiolib_register() The function __devinit samsung_pinctrl_probe() references a function __init samsung_gpiolib_register(). If samsung_gpiolib_register is only used by samsung_pinctrl_probe then annotate samsung_gpiolib_register with a matching annotation. or like this (now fixed in socfpga_defconfig): WARNING: drivers/net/ethernet/stmicro/stmmac/stmmac.o(.text+0x5d4c): Section mismatch in reference from the function stmmac_pltfr_probe() to the function .devinit.text:stmmac_probe_config_dt() The function stmmac_pltfr_probe() references the function __devinit stmmac_probe_config_dt(). This is often because stmmac_pltfr_probe lacks a __devinit annotation or the annotation of stmmac_probe_config_dt is wrong. I believe you normally don't get warnings for functions that could be marked __devinit and only call regular functions, but there are a couple of __devinit infrastructure functions that you can't call from a function that isn't __init or __devinit. Right. If you get those warnings you shouldn't be dropping the annotations. But I don't think that is the case for this driver. Tony, can you confirm that the driver still builds properly without warnings if you drop the __devinit/__devexit? Thierry ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel Correct - it builds without warnings without __devinit/__devexit. If it had introduced warnings when I tested it, I would have mentioned it :) Regards Tony P -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
