| Message ID | 1350643135-13197-2-git-send-email-linux@prisktech.co.nz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <linux@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 <alchark@gmail.com> > + * Copyright (C) 2012 Tony Prisk <linux@prisktech.co.nz> > + * Copyright (C) 2010 Alexey Charkov <alchark@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 = 25000000/2; /* wild guess --- need to implement clocks */ > + c = clk_get_rate(vt8500->clk); > c = c * period_ns; > do_div(c, 1000000000); > 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"); > return ret; > + } > > platform_set_drvdata(pdev, chip); > return ret; > } > > -static int __devexit pwm_remove(struct platform_device *pdev) > +static int __devexit vt8500_pwm_remove(struct platform_device *pdev) > { > struct vt8500_chip *chip; > > @@ -150,28 +177,24 @@ static int __devexit pwm_remove(struct platform_device *pdev) > if (chip == NULL) > return -ENODEV; > > + clk_disable_unprepare(chip->clk); > + Again, shouldn't it be more efficient power-wise to disable this as soon as the last PWM device is disabled? > return pwmchip_remove(&chip->chip); > } > > -static struct platform_driver pwm_driver = { > +static struct platform_driver vt8500_pwm_driver = { > + .probe = vt8500_pwm_probe, > + .remove = __devexit_p(vt8500_pwm_remove), __devexit_p can also be removed. > .driver = { > .name = "vt8500-pwm", > .owner = THIS_MODULE, > + .of_match_table = vt8500_pwm_dt_ids, > }, > - .probe = pwm_probe, > - .remove = __devexit_p(pwm_remove), > }; > > -static int __init pwm_init(void) > -{ > - return platform_driver_register(&pwm_driver); > -} > -arch_initcall(pwm_init); > - > -static void __exit pwm_exit(void) > -{ > - platform_driver_unregister(&pwm_driver); > -} > -module_exit(pwm_exit); > +module_platform_driver(vt8500_pwm_driver); > > -MODULE_LICENSE("GPL"); > +MODULE_DESCRIPTION("VT8500 PWM Driver"); > +MODULE_AUTHOR("Tony Prisk <linux@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. =) > +MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids); I think it is customary to put this right after the device table definition. > -- > 1.7.9.5 > > >
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 <linux@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 > > > > > >
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
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 <linux@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
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
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
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
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
On Mon, Oct 22, 2012 at 11:50:21AM +0000, 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
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
On Mon, Oct 22, 2012 at 01:52:08PM +0000, 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
On Mon, 2012-10-22 at 17:08 +0200, Thierry Reding wrote: > On Mon, Oct 22, 2012 at 01:52:08PM +0000, 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-kernel@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
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
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 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. > 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(). That can be remedied by adding a corresponding function, so the argument doesn't really count. Thierry
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.
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
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 <alchark@gmail.com> + * Copyright (C) 2012 Tony Prisk <linux@prisktech.co.nz> + * Copyright (C) 2010 Alexey Charkov <alchark@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 = 25000000/2; /* wild guess --- need to implement clocks */ + c = clk_get_rate(vt8500->clk); c = c * period_ns; do_div(c, 1000000000); 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) { 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; + } + 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); - 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); + if (!chip->base) { + dev_err(&pdev->dev, "memory resource not available\n"); return -EADDRNOTAVAIL; + } + + clk_prepare_enable(chip->clk); ret = pwmchip_add(&chip->chip); - if (ret < 0) + if (ret < 0) { + dev_err(&pdev->dev, "failed to add pwmchip\n"); return ret; + } platform_set_drvdata(pdev, chip); return ret; } -static int __devexit pwm_remove(struct platform_device *pdev) +static int __devexit vt8500_pwm_remove(struct platform_device *pdev) { struct vt8500_chip *chip; @@ -150,28 +177,24 @@ static int __devexit pwm_remove(struct platform_device *pdev) if (chip == NULL) return -ENODEV; + clk_disable_unprepare(chip->clk); + return pwmchip_remove(&chip->chip); } -static struct platform_driver pwm_driver = { +static struct platform_driver vt8500_pwm_driver = { + .probe = vt8500_pwm_probe, + .remove = __devexit_p(vt8500_pwm_remove), .driver = { .name = "vt8500-pwm", .owner = THIS_MODULE, + .of_match_table = vt8500_pwm_dt_ids, }, - .probe = pwm_probe, - .remove = __devexit_p(pwm_remove), }; -static int __init pwm_init(void) -{ - return platform_driver_register(&pwm_driver); -} -arch_initcall(pwm_init); - -static void __exit pwm_exit(void) -{ - platform_driver_unregister(&pwm_driver); -} -module_exit(pwm_exit); +module_platform_driver(vt8500_pwm_driver); -MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("VT8500 PWM Driver"); +MODULE_AUTHOR("Tony Prisk <linux@prisktech.co.nz>"); +MODULE_LICENSE("GPL v2"); +MODULE_DEVICE_TABLE(of, vt8500_pwm_dt_ids);
This patch updates pwm-vt8500.c to support devicetree probing and make use of the common clock subsystem. Signed-off-by: Tony Prisk <linux@prisktech.co.nz> --- drivers/pwm/pwm-vt8500.c | 79 ++++++++++++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 28 deletions(-)