| Message ID | 1329923841-32017-4-git-send-email-thierry.reding@avionic-design.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wednesday 22 February 2012, Thierry Reding wrote: > This patch adds helpers to support device tree bindings for the generic > PWM API. Device tree binding documentation for PWM controllers is also > provided. > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > Documentation/devicetree/bindings/pwm/pwm.txt | 48 +++++++++ > drivers/of/Kconfig | 6 + > drivers/of/Makefile | 1 + > drivers/of/pwm.c | 130 +++++++++++++++++++++++++ > include/linux/of_pwm.h | 51 ++++++++++ > include/linux/pwm.h | 17 +++ I think it would make more sense to stick the device tree specific parts into drivers/pwm/of.c instead of drivers/of/pwm.c, but it's not a strong preference on my part. > @@ -0,0 +1,48 @@ > +Specifying PWM information for devices > +====================================== > + > +1) PWM user nodes > +----------------- > + > +PWM users should specify a list of PWM devices that they want to use > +with a property containing a 'pwm-list': > + > + pwm-list ::= <single-pwm> [pwm-list] > + single-pwm ::= <pwm-phandle> <pwm-specifier> > + pwm-phandle : phandle to PWM controller node > + pwm-specifier : array of #pwm-cells specifying the given PWM > + (controller specific) > + > +PWM properties should be named "[<name>-]pwms". Exact meaning of each > +pwms property must be documented in the device tree binding for each > +device. > + > +The following example could be used to describe a PWM-based backlight > +device: > + > + pwm: pwm { > + #pwm-cells = <2>; > + }; > + > + [...] > + > + bl: backlight { > + pwms = <&pwm 0 5000000>; > + }; > + > +pwm-specifier typically encodes the chip-relative PWM number and the PWM > +period in nanoseconds. I like these bindings, this looks very straightforward to use while also able to describe all possible cases. > +/** > + * of_node_to_pwmchip() - finds the PWM chip associated with a device node > + * @np: device node of the PWM chip > + * > + * Returns a pointer to the PWM chip associated with the specified device > + * node or NULL if the device node doesn't represent a PWM chip. > + */ > +struct pwm_chip *of_node_to_pwmchip(struct device_node *np) > +{ > + return pwmchip_find(np, of_pwmchip_is_match); > +} > + > +int of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args, > + struct pwm_spec *spec) > +{ > + if (pc->of_pwm_n_cells < 2) > + return -EINVAL; > + > + if (args->args_count < pc->of_pwm_n_cells) > + return -EINVAL; > + > + if (args->args[0] >= pc->npwm) > + return -EINVAL; > + > + if (spec) > + spec->period = args->args[1]; > + > + return args->args[0]; > +} I'm not sure why these functions are global. They are clearly marked so in your header file, but it seems that these are an implementation detail that neither a pwm controller driver not a driver using one would need to use directly. > +/** > + * of_get_named_pwm() - get a PWM number and period to use with the PWM API > + * @np: device node to get the PWM from > + * @propname: property name containing PWM specifier(s) > + * @index: index of the PWM > + * @spec: a pointer to a struct pwm_spec to fill in > + * > + * Returns PWM number to use with the Linux generic PWM API or a negative > + * error code on failure. If @spec is not NULL the function fills in the > + * values parsed from the device tree. > + */ > +int of_get_named_pwm(struct device_node *np, const char *propname, > + int index, struct pwm_spec *spec) > +{ This interface does not feel right to me, in multiple ways: * I would expect to pass a struct device in, not a device_node. * Why not include the pwm_request() call in this and return the pwm_device directly? You said that you want to get rid of the pwm_id eventually, which is a good idea, but this interface still forces one to use it. * It is not clear what a pwm_spec is used for, or why a device driver would need to be bothered by this. Maybe it just needs more explanation, but it seems to me that if you do the previous change, the pwm_spec would not be needed either. > +EXPORT_SYMBOL(of_get_named_pwm); EXPORT_SYMBOL_GPL? > +static inline int of_get_named_pwm(struct device_node *np, > + const char *propname, int index, > + unsigned int *period_ns) > +{ The function prototype does not match. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Arnd Bergmann wrote: > On Wednesday 22 February 2012, Thierry Reding wrote: > > This patch adds helpers to support device tree bindings for the generic > > PWM API. Device tree binding documentation for PWM controllers is also > > provided. > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > > > Documentation/devicetree/bindings/pwm/pwm.txt | 48 +++++++++ > > drivers/of/Kconfig | 6 + > > drivers/of/Makefile | 1 + > > drivers/of/pwm.c | 130 +++++++++++++++++++++++++ > > include/linux/of_pwm.h | 51 ++++++++++ > > include/linux/pwm.h | 17 +++ > > I think it would make more sense to stick the device tree specific parts > into drivers/pwm/of.c instead of drivers/of/pwm.c, but it's not a strong > preference on my part. I was just following what everybody else seemed to be doing. drivers/of/ already has support code for GPIO, IRQ, I2C, PCI and whatnot. [...] > > +/** > > + * of_node_to_pwmchip() - finds the PWM chip associated with a device node > > + * @np: device node of the PWM chip > > + * > > + * Returns a pointer to the PWM chip associated with the specified device > > + * node or NULL if the device node doesn't represent a PWM chip. > > + */ > > +struct pwm_chip *of_node_to_pwmchip(struct device_node *np) > > +{ > > + return pwmchip_find(np, of_pwmchip_is_match); > > +} > > + > > +int of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args, > > + struct pwm_spec *spec) > > +{ > > + if (pc->of_pwm_n_cells < 2) > > + return -EINVAL; > > + > > + if (args->args_count < pc->of_pwm_n_cells) > > + return -EINVAL; > > + > > + if (args->args[0] >= pc->npwm) > > + return -EINVAL; > > + > > + if (spec) > > + spec->period = args->args[1]; > > + > > + return args->args[0]; > > +} > > I'm not sure why these functions are global. They are clearly marked > so in your header file, but it seems that these are an implementation > detail that neither a pwm controller driver not a driver using one > would need to use directly. Yes, I think both can be made static. of_pwm_simple_xlate() is supposed to be the fallback when a chip doesn't explicitly specify one so it really isn't necessary to export it (for that to be useful it is missing an EXPORT_SYMBOL anyway). > > +/** > > + * of_get_named_pwm() - get a PWM number and period to use with the PWM API > > + * @np: device node to get the PWM from > > + * @propname: property name containing PWM specifier(s) > > + * @index: index of the PWM > > + * @spec: a pointer to a struct pwm_spec to fill in > > + * > > + * Returns PWM number to use with the Linux generic PWM API or a negative > > + * error code on failure. If @spec is not NULL the function fills in the > > + * values parsed from the device tree. > > + */ > > +int of_get_named_pwm(struct device_node *np, const char *propname, > > + int index, struct pwm_spec *spec) > > +{ > > This interface does not feel right to me, in multiple ways: > > * I would expect to pass a struct device in, not a device_node. I was following the GPIO DT support code here. The corresponding of_get_named_gpio_flags() takes a struct device_node. I guess that makes it more generic since you can potentially have a struct device_node without a corresponding struct device, right? > * Why not include the pwm_request() call in this and return the > pwm_device directly? You said that you want to get rid of the > pwm_id eventually, which is a good idea, but this interface still > forces one to use it. Okay, that sounds sensible. I propose to rename the function to something like of_request_pwm(). It would of course need an additional parameter (name) to forward to pwm_request(). > * It is not clear what a pwm_spec is used for, or why a device > driver would need to be bothered by this. Maybe it just needs > more explanation, but it seems to me that if you do the previous > change, the pwm_spec would not be needed either. pwm_spec is pretty much what the of_xlate() callback parses out of the data provided by the device tree. Currently, of_pwm_simple_xlate() only parses the period (in ns) but the idea was that if at some point in the future it was decided to provide more than the period via the device tree it could be extended without changing every call to of_get_named_pwm(). As I said, it also plays quite nicely with the concept of the of_xlate() callback and sort of serves as interface between the lower layer that retrieves PWM parameters and the upper layers that use it. Thinking about it, perhaps renaming it to pwm_params may be more descriptive. Also to avoid breakage or confusion if fields get added it may be good to provide a bitmask of valid fields filled in by the of_xlate() callback. enum { PWM_PARAMS_PERIOD, ... }; struct pwm_params { unsigned long fields; unsigned int period; }; Then again, maybe that's just over-engineering and directly returning via an unsigned int *period_ns parameter would be better? > > +EXPORT_SYMBOL(of_get_named_pwm); > > EXPORT_SYMBOL_GPL? It was brought up at some point that it might be nice to allow non-GPL drivers to use the PWM framework as well. I don't remember any discussion resulting from the comment. Perhaps we should have that discussion now and decide whether or not we want to keep it GPL-only or not. > > +static inline int of_get_named_pwm(struct device_node *np, > > + const char *propname, int index, > > + unsigned int *period_ns) > > +{ > > The function prototype does not match. Yes, I must have forgotten to convert that when I changed that to struct pwm_spec described above. Quite frankly I'm not sure about what is the best alternative. Can anybody come up with more advantages or disadvantages for both variations? Or perhaps there is even a better solution. Thierry
On Thursday 23 February 2012, Thierry Reding wrote: > * Arnd Bergmann wrote: > > On Wednesday 22 February 2012, Thierry Reding wrote: > > > This patch adds helpers to support device tree bindings for the generic > > > PWM API. Device tree binding documentation for PWM controllers is also > > > provided. > > > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > > > > > Documentation/devicetree/bindings/pwm/pwm.txt | 48 +++++++++ > > > drivers/of/Kconfig | 6 + > > > drivers/of/Makefile | 1 + > > > drivers/of/pwm.c | 130 +++++++++++++++++++++++++ > > > include/linux/of_pwm.h | 51 ++++++++++ > > > include/linux/pwm.h | 17 +++ > > > > I think it would make more sense to stick the device tree specific parts > > into drivers/pwm/of.c instead of drivers/of/pwm.c, but it's not a strong > > preference on my part. > > I was just following what everybody else seemed to be doing. drivers/of/ > already has support code for GPIO, IRQ, I2C, PCI and whatnot. Yes, as I said, it's not entirely clear what's best here, and it would be nice if other people could weigh in when they have a strong opinion one way or another. > > > +/** > > > + * of_get_named_pwm() - get a PWM number and period to use with the PWM API > > > + * @np: device node to get the PWM from > > > + * @propname: property name containing PWM specifier(s) > > > + * @index: index of the PWM > > > + * @spec: a pointer to a struct pwm_spec to fill in > > > + * > > > + * Returns PWM number to use with the Linux generic PWM API or a negative > > > + * error code on failure. If @spec is not NULL the function fills in the > > > + * values parsed from the device tree. > > > + */ > > > +int of_get_named_pwm(struct device_node *np, const char *propname, > > > + int index, struct pwm_spec *spec) > > > +{ > > > > This interface does not feel right to me, in multiple ways: > > > > * I would expect to pass a struct device in, not a device_node. > > I was following the GPIO DT support code here. The corresponding > of_get_named_gpio_flags() takes a struct device_node. I guess that makes it > more generic since you can potentially have a struct device_node without a > corresponding struct device, right? Yes, but I don't see that as important here. > > * Why not include the pwm_request() call in this and return the > > pwm_device directly? You said that you want to get rid of the > > pwm_id eventually, which is a good idea, but this interface still > > forces one to use it. > > Okay, that sounds sensible. I propose to rename the function to something like > of_request_pwm(). Sounds good. > It would of course need an additional parameter (name) to > forward to pwm_request(). Not necessarily, it could use the dev_name(device) or the name of the property, or a combination of the two. > > * It is not clear what a pwm_spec is used for, or why a device > > driver would need to be bothered by this. Maybe it just needs > > more explanation, but it seems to me that if you do the previous > > change, the pwm_spec would not be needed either. > > pwm_spec is pretty much what the of_xlate() callback parses out of the data > provided by the device tree. Currently, of_pwm_simple_xlate() only parses the > period (in ns) but the idea was that if at some point in the future it was > decided to provide more than the period via the device tree it could be > extended without changing every call to of_get_named_pwm(). As I said, it also > plays quite nicely with the concept of the of_xlate() callback and sort of > serves as interface between the lower layer that retrieves PWM parameters and > the upper layers that use it. > > Thinking about it, perhaps renaming it to pwm_params may be more descriptive. > Also to avoid breakage or confusion if fields get added it may be good to > provide a bitmask of valid fields filled in by the of_xlate() callback. > > enum { > PWM_PARAMS_PERIOD, > ... > }; > > struct pwm_params { > unsigned long fields; > unsigned int period; > }; > > Then again, maybe that's just over-engineering and directly returning via an > unsigned int *period_ns parameter would be better? It certainly sounds like over-engineering to me. Why not keep all that information hidden inside the struct pwm_device and provide accessor functions like this? unsigned int pwm_get_period(struct pwm_device *pwm); > > > +EXPORT_SYMBOL(of_get_named_pwm); > > > > EXPORT_SYMBOL_GPL? > > It was brought up at some point that it might be nice to allow non-GPL > drivers to use the PWM framework as well. I don't remember any discussion > resulting from the comment. Perhaps we should have that discussion now and > decide whether or not we want to keep it GPL-only or not. I would definitely use EXPORT_SYMBOL_GPL for all new code unless it replaces an earlier interface that was available as EXPORT_SYMBOL. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Arnd Bergmann wrote: > On Thursday 23 February 2012, Thierry Reding wrote: > > * Arnd Bergmann wrote: > > > On Wednesday 22 February 2012, Thierry Reding wrote: > > > > This patch adds helpers to support device tree bindings for the generic > > > > PWM API. Device tree binding documentation for PWM controllers is also > > > > provided. > > > > > > > > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> > > > > > > > > Documentation/devicetree/bindings/pwm/pwm.txt | 48 +++++++++ > > > > drivers/of/Kconfig | 6 + > > > > drivers/of/Makefile | 1 + > > > > drivers/of/pwm.c | 130 +++++++++++++++++++++++++ > > > > include/linux/of_pwm.h | 51 ++++++++++ > > > > include/linux/pwm.h | 17 +++ > > > > > > I think it would make more sense to stick the device tree specific parts > > > into drivers/pwm/of.c instead of drivers/of/pwm.c, but it's not a strong > > > preference on my part. > > > > I was just following what everybody else seemed to be doing. drivers/of/ > > already has support code for GPIO, IRQ, I2C, PCI and whatnot. > > Yes, as I said, it's not entirely clear what's best here, and it would > be nice if other people could weigh in when they have a strong opinion > one way or another. Okay, I'll wait some more for others to join in. [...] > > > * Why not include the pwm_request() call in this and return the > > > pwm_device directly? You said that you want to get rid of the > > > pwm_id eventually, which is a good idea, but this interface still > > > forces one to use it. > > > > Okay, that sounds sensible. I propose to rename the function to something like > > of_request_pwm(). > > Sounds good. > > > It would of course need an additional parameter (name) to > > forward to pwm_request(). > > Not necessarily, it could use the dev_name(device) or the name > of the property, or a combination of the two. The problem with that is that usually the device would be named something generic like "pwm", while in case where the PWM is used for the backlight it makes sense to label the PWM device "backlight". Looking at debugfs and seeing an entry "backlight" is much more straight- forward than "pwm.0". I mean "pwm.0" doesn't carry any useful information really, does it? > > > * It is not clear what a pwm_spec is used for, or why a device > > > driver would need to be bothered by this. Maybe it just needs > > > more explanation, but it seems to me that if you do the previous > > > change, the pwm_spec would not be needed either. > > > > pwm_spec is pretty much what the of_xlate() callback parses out of the data > > provided by the device tree. Currently, of_pwm_simple_xlate() only parses the > > period (in ns) but the idea was that if at some point in the future it was > > decided to provide more than the period via the device tree it could be > > extended without changing every call to of_get_named_pwm(). As I said, it also > > plays quite nicely with the concept of the of_xlate() callback and sort of > > serves as interface between the lower layer that retrieves PWM parameters and > > the upper layers that use it. > > > > Thinking about it, perhaps renaming it to pwm_params may be more descriptive. > > Also to avoid breakage or confusion if fields get added it may be good to > > provide a bitmask of valid fields filled in by the of_xlate() callback. > > > > enum { > > PWM_PARAMS_PERIOD, > > ... > > }; > > > > struct pwm_params { > > unsigned long fields; > > unsigned int period; > > }; > > > > Then again, maybe that's just over-engineering and directly returning via an > > unsigned int *period_ns parameter would be better? > > It certainly sounds like over-engineering to me. Why not keep all that > information hidden inside the struct pwm_device and provide accessor > functions like this? > > unsigned int pwm_get_period(struct pwm_device *pwm); Heh, I like that. It is the obvious thing to do. Why didn't I think of it? :-) > > > > +EXPORT_SYMBOL(of_get_named_pwm); > > > > > > EXPORT_SYMBOL_GPL? > > > > It was brought up at some point that it might be nice to allow non-GPL > > drivers to use the PWM framework as well. I don't remember any discussion > > resulting from the comment. Perhaps we should have that discussion now and > > decide whether or not we want to keep it GPL-only or not. > > I would definitely use EXPORT_SYMBOL_GPL for all new code unless it > replaces an earlier interface that was available as EXPORT_SYMBOL. I just grepped the code and noticed this: $ $ git grep -n 'EXPORT_SYMBOL.*(pwm_request)' arch/arm/mach-vt8500/pwm.c:139:EXPORT_SYMBOL(pwm_request); arch/arm/plat-mxc/pwm.c:183:EXPORT_SYMBOL(pwm_request); arch/arm/plat-samsung/pwm.c:83:EXPORT_SYMBOL(pwm_request); arch/unicore32/kernel/pwm.c:132:EXPORT_SYMBOL(pwm_request); drivers/mfd/twl6030-pwm.c:156:EXPORT_SYMBOL(pwm_request); drivers/misc/ab8500-pwm.c:108:EXPORT_SYMBOL(pwm_request); drivers/pwm/core.c:262:EXPORT_SYMBOL_GPL(pwm_request); It seems like the legacy PWM API used to be non-GPL. Should I switch it back? Also does it make sense to have something like of_request_pwm() GPL when the rest of the API isn't? Thierry
On Friday 24 February 2012, Thierry Reding wrote: > * Arnd Bergmann wrote: > > On Thursday 23 February 2012, Thierry Reding wrote: > > > * Arnd Bergmann wrote: > [...] > > > > * Why not include the pwm_request() call in this and return the > > > > pwm_device directly? You said that you want to get rid of the > > > > pwm_id eventually, which is a good idea, but this interface still > > > > forces one to use it. > > > > > > Okay, that sounds sensible. I propose to rename the function to something like > > > of_request_pwm(). > > > > Sounds good. On second thought, I would actually prefer starting the name with pwm_ and making it independent of device tree. There might be other ways how to find the pwm_device from a struct device in the future, but it should always be possible using a device together with a string and/or numeric identifier, much in the same way that we can get a resource from a platform_device. Ideally, there would be a common theme behind finding a memory region, irq, gpio pin, clock, regulator, dma-channel and pwm or anything else that requires a link between two device nodes. > > > It would of course need an additional parameter (name) to > > > forward to pwm_request(). > > > > Not necessarily, it could use the dev_name(device) or the name > > of the property, or a combination of the two. > > The problem with that is that usually the device would be named something > generic like "pwm", while in case where the PWM is used for the backlight > it makes sense to label the PWM device "backlight". > > Looking at debugfs and seeing an entry "backlight" is much more straight- > forward than "pwm.0". I mean "pwm.0" doesn't carry any useful information > really, does it? But the device name would be from the device using the pwm, not the pwm controller, so it should be something more helpful, no? > > > > > +EXPORT_SYMBOL(of_get_named_pwm); > > > > > > > > EXPORT_SYMBOL_GPL? > > > > > > It was brought up at some point that it might be nice to allow non-GPL > > > drivers to use the PWM framework as well. I don't remember any discussion > > > resulting from the comment. Perhaps we should have that discussion now and > > > decide whether or not we want to keep it GPL-only or not. > > > > I would definitely use EXPORT_SYMBOL_GPL for all new code unless it > > replaces an earlier interface that was available as EXPORT_SYMBOL. > > I just grepped the code and noticed this: > > $ $ git grep -n 'EXPORT_SYMBOL.*(pwm_request)' > arch/arm/mach-vt8500/pwm.c:139:EXPORT_SYMBOL(pwm_request); > arch/arm/plat-mxc/pwm.c:183:EXPORT_SYMBOL(pwm_request); > arch/arm/plat-samsung/pwm.c:83:EXPORT_SYMBOL(pwm_request); > arch/unicore32/kernel/pwm.c:132:EXPORT_SYMBOL(pwm_request); > drivers/mfd/twl6030-pwm.c:156:EXPORT_SYMBOL(pwm_request); > drivers/misc/ab8500-pwm.c:108:EXPORT_SYMBOL(pwm_request); > drivers/pwm/core.c:262:EXPORT_SYMBOL_GPL(pwm_request); > > It seems like the legacy PWM API used to be non-GPL. Should I switch it back? > Also does it make sense to have something like of_request_pwm() GPL when the > rest of the API isn't? I guess the choice is to make between you and Sascha. The implementation is new, so you could pick EXPORT_SYMBOL_GPL, but you could also try to keep to the current API. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 24, 2012 at 04:58:31PM +0000, Arnd Bergmann wrote: > On Friday 24 February 2012, Thierry Reding wrote: > > * Arnd Bergmann wrote: > > > On Thursday 23 February 2012, Thierry Reding wrote: > > > > * Arnd Bergmann wrote: > > > [...] > > > > > * Why not include the pwm_request() call in this and return the > > > > > pwm_device directly? You said that you want to get rid of the > > > > > pwm_id eventually, which is a good idea, but this interface still > > > > > forces one to use it. > > > > > > > > Okay, that sounds sensible. I propose to rename the function to something like > > > > of_request_pwm(). > > > > > > Sounds good. > > On second thought, I would actually prefer starting the name with pwm_ and > making it independent of device tree. There might be other ways how to > find the pwm_device from a struct device in the future, but it should always > be possible using a device together with a string and/or numeric identifier, > much in the same way that we can get a resource from a platform_device. > > Ideally, there would be a common theme behind finding a memory region, > irq, gpio pin, clock, regulator, dma-channel and pwm or anything else > that requires a link between two device nodes. > > > > > It would of course need an additional parameter (name) to > > > > forward to pwm_request(). > > > > > > Not necessarily, it could use the dev_name(device) or the name > > > of the property, or a combination of the two. > > > > The problem with that is that usually the device would be named something > > generic like "pwm", while in case where the PWM is used for the backlight > > it makes sense to label the PWM device "backlight". > > > > Looking at debugfs and seeing an entry "backlight" is much more straight- > > forward than "pwm.0". I mean "pwm.0" doesn't carry any useful information > > really, does it? > > But the device name would be from the device using the pwm, not the > pwm controller, so it should be something more helpful, no? > > > > > > > +EXPORT_SYMBOL(of_get_named_pwm); > > > > > > > > > > EXPORT_SYMBOL_GPL? > > > > > > > > It was brought up at some point that it might be nice to allow non-GPL > > > > drivers to use the PWM framework as well. I don't remember any discussion > > > > resulting from the comment. Perhaps we should have that discussion now and > > > > decide whether or not we want to keep it GPL-only or not. > > > > > > I would definitely use EXPORT_SYMBOL_GPL for all new code unless it > > > replaces an earlier interface that was available as EXPORT_SYMBOL. > > > > I just grepped the code and noticed this: > > > > $ $ git grep -n 'EXPORT_SYMBOL.*(pwm_request)' > > arch/arm/mach-vt8500/pwm.c:139:EXPORT_SYMBOL(pwm_request); > > arch/arm/plat-mxc/pwm.c:183:EXPORT_SYMBOL(pwm_request); > > arch/arm/plat-samsung/pwm.c:83:EXPORT_SYMBOL(pwm_request); > > arch/unicore32/kernel/pwm.c:132:EXPORT_SYMBOL(pwm_request); > > drivers/mfd/twl6030-pwm.c:156:EXPORT_SYMBOL(pwm_request); > > drivers/misc/ab8500-pwm.c:108:EXPORT_SYMBOL(pwm_request); > > drivers/pwm/core.c:262:EXPORT_SYMBOL_GPL(pwm_request); > > > > It seems like the legacy PWM API used to be non-GPL. Should I switch it back? > > Also does it make sense to have something like of_request_pwm() GPL when the > > rest of the API isn't? > > I guess the choice is to make between you and Sascha. The implementation is > new, so you could pick EXPORT_SYMBOL_GPL, but you could also try to > keep to the current API. I tend to use _GPL, but I have no strong objection using the non GPL variant. Sascha
On 25/02/12 23:33, Sascha Hauer wrote: > On Fri, Feb 24, 2012 at 04:58:31PM +0000, Arnd Bergmann wrote: >> On Friday 24 February 2012, Thierry Reding wrote: >>> * Arnd Bergmann wrote: >>>> On Thursday 23 February 2012, Thierry Reding wrote: >>>>> * Arnd Bergmann wrote: >> >>> [...] >>>>>> * Why not include the pwm_request() call in this and return the >>>>>> pwm_device directly? You said that you want to get rid of the >>>>>> pwm_id eventually, which is a good idea, but this interface still >>>>>> forces one to use it. >>>>> >>>>> Okay, that sounds sensible. I propose to rename the function to something like >>>>> of_request_pwm(). >>>> >>>> Sounds good. >> >> On second thought, I would actually prefer starting the name with pwm_ and >> making it independent of device tree. There might be other ways how to >> find the pwm_device from a struct device in the future, but it should always >> be possible using a device together with a string and/or numeric identifier, >> much in the same way that we can get a resource from a platform_device. >> >> Ideally, there would be a common theme behind finding a memory region, >> irq, gpio pin, clock, regulator, dma-channel and pwm or anything else >> that requires a link between two device nodes. >> >>>>> It would of course need an additional parameter (name) to >>>>> forward to pwm_request(). >>>> >>>> Not necessarily, it could use the dev_name(device) or the name >>>> of the property, or a combination of the two. >>> >>> The problem with that is that usually the device would be named something >>> generic like "pwm", while in case where the PWM is used for the backlight >>> it makes sense to label the PWM device "backlight". >>> >>> Looking at debugfs and seeing an entry "backlight" is much more straight- >>> forward than "pwm.0". I mean "pwm.0" doesn't carry any useful information >>> really, does it? >> >> But the device name would be from the device using the pwm, not the >> pwm controller, so it should be something more helpful, no? >> >>>>>>> +EXPORT_SYMBOL(of_get_named_pwm); >>>>>> >>>>>> EXPORT_SYMBOL_GPL? >>>>> >>>>> It was brought up at some point that it might be nice to allow non-GPL >>>>> drivers to use the PWM framework as well. I don't remember any discussion >>>>> resulting from the comment. Perhaps we should have that discussion now and >>>>> decide whether or not we want to keep it GPL-only or not. >>>> >>>> I would definitely use EXPORT_SYMBOL_GPL for all new code unless it >>>> replaces an earlier interface that was available as EXPORT_SYMBOL. >>> >>> I just grepped the code and noticed this: >>> >>> $ $ git grep -n 'EXPORT_SYMBOL.*(pwm_request)' >>> arch/arm/mach-vt8500/pwm.c:139:EXPORT_SYMBOL(pwm_request); >>> arch/arm/plat-mxc/pwm.c:183:EXPORT_SYMBOL(pwm_request); >>> arch/arm/plat-samsung/pwm.c:83:EXPORT_SYMBOL(pwm_request); >>> arch/unicore32/kernel/pwm.c:132:EXPORT_SYMBOL(pwm_request); >>> drivers/mfd/twl6030-pwm.c:156:EXPORT_SYMBOL(pwm_request); >>> drivers/misc/ab8500-pwm.c:108:EXPORT_SYMBOL(pwm_request); >>> drivers/pwm/core.c:262:EXPORT_SYMBOL_GPL(pwm_request); >>> >>> It seems like the legacy PWM API used to be non-GPL. Should I switch it back? >>> Also does it make sense to have something like of_request_pwm() GPL when the >>> rest of the API isn't? >> >> I guess the choice is to make between you and Sascha. The implementation is >> new, so you could pick EXPORT_SYMBOL_GPL, but you could also try to >> keep to the current API. > > I tend to use _GPL, but I have no strong objection using the non GPL > variant. I raised the question last time round. My understanding in that internal interfaces, those which should never be used by external modules, should be EXPORT_SYMBOL_GPL, but public interfaces should be EXPORT_SYMBOL. I'm not hugely against making the entire interface _GPL, I just wanted to make sure it was intended that way, and not just cut and paste :-). ~Ryan -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt new file mode 100644 index 0000000..9421fe7 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm.txt @@ -0,0 +1,48 @@ +Specifying PWM information for devices +====================================== + +1) PWM user nodes +----------------- + +PWM users should specify a list of PWM devices that they want to use +with a property containing a 'pwm-list': + + pwm-list ::= <single-pwm> [pwm-list] + single-pwm ::= <pwm-phandle> <pwm-specifier> + pwm-phandle : phandle to PWM controller node + pwm-specifier : array of #pwm-cells specifying the given PWM + (controller specific) + +PWM properties should be named "[<name>-]pwms". Exact meaning of each +pwms property must be documented in the device tree binding for each +device. + +The following example could be used to describe a PWM-based backlight +device: + + pwm: pwm { + #pwm-cells = <2>; + }; + + [...] + + bl: backlight { + pwms = <&pwm 0 5000000>; + }; + +pwm-specifier typically encodes the chip-relative PWM number and the PWM +period in nanoseconds. + +2) PWM controller nodes +----------------------- + +PWM controller nodes must specify the number of cells used for the +specifier using the '#pwm-cells' property. + +An example PWM controller might look like this: + + pwm: pwm@7000a000 { + compatible = "nvidia,tegra20-pwm"; + reg = <0x7000a000 0x100>; + #pwm-cells = <2>; + }; diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig index 6ea51dc..d47b8ee 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -57,6 +57,12 @@ config OF_GPIO help OpenFirmware GPIO accessors +config OF_PWM + def_bool y + depends on PWM + help + OpenFirmware PWM accessors + config OF_I2C def_tristate I2C depends on I2C && !SPARC diff --git a/drivers/of/Makefile b/drivers/of/Makefile index a73f5a5..3dd13e3 100644 --- a/drivers/of/Makefile +++ b/drivers/of/Makefile @@ -5,6 +5,7 @@ obj-$(CONFIG_OF_ADDRESS) += address.o obj-$(CONFIG_OF_IRQ) += irq.o obj-$(CONFIG_OF_DEVICE) += device.o platform.o obj-$(CONFIG_OF_GPIO) += gpio.o +obj-$(CONFIG_OF_PWM) += pwm.o obj-$(CONFIG_OF_I2C) += of_i2c.o obj-$(CONFIG_OF_NET) += of_net.o obj-$(CONFIG_OF_SPI) += of_spi.o diff --git a/drivers/of/pwm.c b/drivers/of/pwm.c new file mode 100644 index 0000000..d6f7f33 --- /dev/null +++ b/drivers/of/pwm.c @@ -0,0 +1,130 @@ +/* + * OF helpers for the PWM API + * + * Copyright (c) 2011 Avionic Design GmbH + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/errno.h> +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/pwm.h> + +static int of_pwmchip_is_match(struct pwm_chip *chip, void *data) +{ + return chip->dev ? chip->dev->of_node == data : 0; +} + +/** + * of_node_to_pwmchip() - finds the PWM chip associated with a device node + * @np: device node of the PWM chip + * + * Returns a pointer to the PWM chip associated with the specified device + * node or NULL if the device node doesn't represent a PWM chip. + */ +struct pwm_chip *of_node_to_pwmchip(struct device_node *np) +{ + return pwmchip_find(np, of_pwmchip_is_match); +} + +int of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args, + struct pwm_spec *spec) +{ + if (pc->of_pwm_n_cells < 2) + return -EINVAL; + + if (args->args_count < pc->of_pwm_n_cells) + return -EINVAL; + + if (args->args[0] >= pc->npwm) + return -EINVAL; + + if (spec) + spec->period = args->args[1]; + + return args->args[0]; +} + +void of_pwmchip_add(struct pwm_chip *chip) +{ + if (!chip->dev || !chip->dev->of_node) + return; + + if (!chip->of_xlate) { + chip->of_xlate = of_pwm_simple_xlate; + chip->of_pwm_n_cells = 2; + } + + of_node_get(chip->dev->of_node); +} + +void of_pwmchip_remove(struct pwm_chip *chip) +{ + if (chip->dev && chip->dev->of_node) + of_node_put(chip->dev->of_node); +} + +/** + * of_get_named_pwm() - get a PWM number and period to use with the PWM API + * @np: device node to get the PWM from + * @propname: property name containing PWM specifier(s) + * @index: index of the PWM + * @spec: a pointer to a struct pwm_spec to fill in + * + * Returns PWM number to use with the Linux generic PWM API or a negative + * error code on failure. If @spec is not NULL the function fills in the + * values parsed from the device tree. + */ +int of_get_named_pwm(struct device_node *np, const char *propname, + int index, struct pwm_spec *spec) +{ + struct of_phandle_args args; + struct pwm_chip *pc; + int ret; + + ret = of_parse_phandle_with_args(np, propname, "#pwm-cells", index, + &args); + if (ret) { + pr_debug("%s(): can't parse pwms property\n", __func__); + goto out; + } + + pc = of_node_to_pwmchip(args.np); + if (!pc) { + ret = -ENODEV; + goto put; + } + + if (args.args_count != pc->of_pwm_n_cells) { + pr_debug("%s: wrong #pwm-cells for %s\n", np->full_name, + args.np->full_name); + ret = -EINVAL; + goto put; + } + + /* + * reset the specifier structure since .of_xlate might decide not to + * fill it in + */ + if (spec) + spec->period = 0; + + ret = pc->of_xlate(pc, &args, spec); + if (ret < 0) + goto put; + + ret += pc->base; + +put: + of_node_put(args.np); +out: + pr_debug("%s() exited with status %d\n", __func__, ret); + return ret; +} +EXPORT_SYMBOL(of_get_named_pwm); diff --git a/include/linux/of_pwm.h b/include/linux/of_pwm.h new file mode 100644 index 0000000..a6af951 --- /dev/null +++ b/include/linux/of_pwm.h @@ -0,0 +1,51 @@ +/* + * OF helpers for the PWM API + * + * Copyright (c) 2011 Avionic Design GmbH + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#ifndef __LINUX_OF_PWM_H +#define __LINUX_OF_PWM_H + +#include <linux/pwm.h> + +struct device_node; + +#ifdef CONFIG_OF_PWM + +struct pwm_chip *of_node_to_pwmchip(struct device_node *np); +int of_get_named_pwm(struct device_node *np, const char *propname, + int index, struct pwm_spec *spec); +void of_pwmchip_add(struct pwm_chip *pc); +void of_pwmchip_remove(struct pwm_chip *pc); + +#else + +static inline struct pwm_chip *of_node_to_pwmchip(struct device_node *np) +{ + return NULL; +} + +static inline int of_get_named_pwm(struct device_node *np, + const char *propname, int index, + unsigned int *period_ns) +{ + return -ENOSYS; +} + +static inline void of_pwmchip_add(struct pwm_chip *pc) +{ +} + +static inline void of_pwmchip_remove(struct pwm_chip *pc) +{ +} + +#endif /* CONFIG_OF_PWM */ + +#endif /* __LINUX_OF_PWM_H */ diff --git a/include/linux/pwm.h b/include/linux/pwm.h index b00a91f..a7cb543 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -1,6 +1,8 @@ #ifndef __LINUX_PWM_H #define __LINUX_PWM_H +#include <linux/of.h> + struct pwm_device; /* @@ -75,6 +77,14 @@ struct pwm_ops { }; /** + * struct pwm_spec - device tree PWM specifier + * @period: PWM period (in nanoseconds) + */ +struct pwm_spec { + unsigned int period; +}; + +/** * struct pwm_chip - abstract a PWM controller * @dev: device providing the PWMs * @ops: callbacks for this PWM controller @@ -89,6 +99,13 @@ struct pwm_chip { unsigned int npwm; struct pwm_device *pwms; + +#ifdef CONFIG_OF_PWM + int (*of_xlate)(struct pwm_chip *pc, + const struct of_phandle_args *args, + struct pwm_spec *spec); + unsigned int of_pwm_n_cells; +#endif }; int pwm_set_chip_data(struct pwm_device *pwm, void *data);
This patch adds helpers to support device tree bindings for the generic PWM API. Device tree binding documentation for PWM controllers is also provided. Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de> --- Changes in v3: - none Changes in v2: - add device tree binding documentation - add of_xlate to parse controller-specific PWM-specifier Documentation/devicetree/bindings/pwm/pwm.txt | 48 +++++++++ drivers/of/Kconfig | 6 + drivers/of/Makefile | 1 + drivers/of/pwm.c | 130 +++++++++++++++++++++++++ include/linux/of_pwm.h | 51 ++++++++++ include/linux/pwm.h | 17 +++ 6 files changed, 253 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/pwm/pwm.txt create mode 100644 drivers/of/pwm.c create mode 100644 include/linux/of_pwm.h