| Message ID | 1426255082-25002-1-git-send-email-shobhit.kumar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote: > Some chips instead of using period_ns and duty_ns can be configured > using the clock divisor and duty percent. Adds an alternative > configuration method for such chips I don't see a need to introduce this alternative configuration mechanism. Most, of not all, of the other drivers program a clock divisor and some percentage of the duty cycle as well and it should be easy to convert to that internally from the period and duty_cycle parameters that you get in ->config(). Adding an alternative means of configuring the PWM also means that every user driver now potentially needs to support both the traditional and the alternative way because PWM providers may not implement both. Thierry
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 03/24/2015 01:53 PM, Thierry Reding wrote: > On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote: >> Some chips instead of using period_ns and duty_ns can be >> configured using the clock divisor and duty percent. Adds an >> alternative configuration method for such chips > > I don't see a need to introduce this alternative configuration > mechanism. Most, of not all, of the other drivers program a clock > divisor and some percentage of the duty cycle as well and it should > be easy to convert to that internally from the period and > duty_cycle parameters that you get in ->config(). Perhaps. Probably I misunderstood but as per Documentation/pwm.txt, it is suggested that rather than calculating in the driver, we can add additional helpers. So I tried doing just that. And it also means that the consumer(which is directly aware of the percent it wants) has to do the calculation and pass as ns values and we internally again convert back to percentage ? > > Adding an alternative means of configuring the PWM also means that > every user driver now potentially needs to support both the > traditional and the alternative way because PWM providers may not > implement both. I just assumed either or implementation should suffice. Even in my implementation the error checks assumes either of the two should be available else to fail the pwmchip_add Regards Shobhit > > Thierry > > > > _______________________________________________ Intel-gfx mailing > list Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVG5AiAAoJEHuQFv2//5Kq9LYIAKlRfsm4hnDFTlOmhBs5hPhT 2xOx03Vwf5V0U1FisNFr1ZhK9CuJJGGUq4zTvVDmokHNHEtQk4o751fxxY4eVE+a quw7o6BhLcQkwKdHvcHzLYHR+Szs5h60bR8qtAg1UhmGMpPkWiiPytIVOiKHjSfg yxHyAEqjxn9Q07yuhj0g2U/nqvNwODQ72cQXoI8nKNYJsRzNVhlJh8nZ1CxyYqBZ wubMJvjaM7jyFQZf3YDb7zaW3CXqkkxLhJpb/iL3grxQICO6DCnAbqnIagPgjq0H dNetFOAQfmL4i4+gonnSWEW+UBmszeDNlVKTiTH5DNuNigju+HEDGWZaF2e1hKY= =r1M/ -----END PGP SIGNATURE-----
On Wed, Apr 01, 2015 at 11:58:50AM +0530, Shobhit Kumar wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 03/24/2015 01:53 PM, Thierry Reding wrote: > > On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote: > >> Some chips instead of using period_ns and duty_ns can be > >> configured using the clock divisor and duty percent. Adds an > >> alternative configuration method for such chips > > > > I don't see a need to introduce this alternative configuration > > mechanism. Most, of not all, of the other drivers program a clock > > divisor and some percentage of the duty cycle as well and it should > > be easy to convert to that internally from the period and > > duty_cycle parameters that you get in ->config(). > > Perhaps. Probably I misunderstood but as per Documentation/pwm.txt, it > is suggested that rather than calculating in the driver, we can add > additional helpers. So I tried doing just that. And it also means that > the consumer(which is directly aware of the percent it wants) has to > do the calculation and pass as ns values and we internally again > convert back to percentage ? Yes. The interface assumes that you'll pass in absolute values for the period and duty cycle. Existing drivers, such as pwm-backlight, already convert a percentage or other internal representation to these absolute values. If your driver internally works with percent you can easily convert to that from the absolute values. The documentation only makes a suggestion. I think it'd be fine if you kept this conversion internal to the driver. We can turn it into a more generic helper if a second driver appears that needs the same conversion. > > Adding an alternative means of configuring the PWM also means that > > every user driver now potentially needs to support both the > > traditional and the alternative way because PWM providers may not > > implement both. > > I just assumed either or implementation should suffice. Even in my > implementation the error checks assumes either of the two should be > available else to fail the pwmchip_add Your implementation requires that users call either pwm_config() or pwm_config_alternate(). PWM drivers may only have to implement either callback, but users will be required to support both (or otherwise only work with a subset of PWM drivers). Thierry
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 04/10/2015 01:59 PM, Thierry Reding wrote: > On Wed, Apr 01, 2015 at 11:58:50AM +0530, Shobhit Kumar wrote: >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 >> >> On 03/24/2015 01:53 PM, Thierry Reding wrote: >>> On Fri, Mar 13, 2015 at 07:28:02PM +0530, Shobhit Kumar wrote: >>>> Some chips instead of using period_ns and duty_ns can be >>>> configured using the clock divisor and duty percent. Adds an >>>> alternative configuration method for such chips >>> >>> I don't see a need to introduce this alternative configuration >>> mechanism. Most, of not all, of the other drivers program a >>> clock divisor and some percentage of the duty cycle as well and >>> it should be easy to convert to that internally from the period >>> and duty_cycle parameters that you get in ->config(). >> >> Perhaps. Probably I misunderstood but as per >> Documentation/pwm.txt, it is suggested that rather than >> calculating in the driver, we can add additional helpers. So I >> tried doing just that. And it also means that the consumer(which >> is directly aware of the percent it wants) has to do the >> calculation and pass as ns values and we internally again convert >> back to percentage ? > > Yes. The interface assumes that you'll pass in absolute values for > the period and duty cycle. Existing drivers, such as pwm-backlight, > already convert a percentage or other internal representation to > these absolute values. If your driver internally works with percent > you can easily convert to that from the absolute values. > > The documentation only makes a suggestion. I think it'd be fine if > you kept this conversion internal to the driver. We can turn it > into a more generic helper if a second driver appears that needs > the same conversion. Okay, will change driver implementation and avoid this patch > >>> Adding an alternative means of configuring the PWM also means >>> that every user driver now potentially needs to support both >>> the traditional and the alternative way because PWM providers >>> may not implement both. >> >> I just assumed either or implementation should suffice. Even in >> my implementation the error checks assumes either of the two >> should be available else to fail the pwmchip_add > > Your implementation requires that users call either pwm_config() > or pwm_config_alternate(). PWM drivers may only have to implement > either callback, but users will be required to support both (or > otherwise only work with a subset of PWM drivers). Yeah, I overlooked this. Will push a new patch for the driver. Regards Shobhit > > Thierry > -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQEcBAEBAgAGBQJVK3gXAAoJEHuQFv2//5KqRhIIAMKvvSuJ3yyiPrBULOk6PyZg AyNpICHg/pwhnjdns45eui1YnWb6Hrasbs5+UZRxlUAubs/+CDa1ZvGvtAZZQCO0 g8YO0EiafdGUg8KMif2qblJZf0oJFWs1j8sUQaarA7Uh2/1m4elvijQ39J30yzCt +4N2JQ3Nazx2KWS5P8Wo9i2Km733vz7p8nY5lqXlstHer1x4QoaCz6utNPMgcUE+ N5wCUpOzEzqM4Lle63R2UO/uCfC+169Q+bZ2r9a1UxSeLhA+fhkZWgusaUeqi1UL kIy4YSyelTNYIBa8dufp+IQL1w2cSbZ9JoPj7Zc7agTYqbhOuLhbM1wC9DWMWW0= =1wV2 -----END PGP SIGNATURE-----
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 810aef3..604e93d 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -235,7 +235,8 @@ int pwmchip_add(struct pwm_chip *chip) unsigned int i; int ret; - if (!chip || !chip->dev || !chip->ops || !chip->ops->config || + if (!chip || !chip->dev || !chip->ops || + !(chip->ops->config || chip->ops->config_alternate) || !chip->ops->enable || !chip->ops->disable || !chip->npwm) return -EINVAL; @@ -422,6 +423,30 @@ int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns) EXPORT_SYMBOL_GPL(pwm_config); /** + * pwm_config_alternate() - change a PWM device configuration + * @pwm: PWM device + * @clk_div: Clock divider to configure + * @duty_percentage: duty cycle in percentage + */ +int pwm_config_alternate(struct pwm_device *pwm, int clk_div, int duty_percent) +{ + int err; + + if (!pwm || clk_div < 0 || duty_percent < 0 || duty_percent > 100) + return -EINVAL; + + err = pwm->chip->ops->config_alternate(pwm->chip, pwm, clk_div, duty_percent); + if (err) + return err; + + pwm->clk_div = clk_div; + pwm->duty_percent = duty_percent; + + return 0; +} +EXPORT_SYMBOL_GPL(pwm_config_alternate); + +/** * pwm_set_polarity() - configure the polarity of a PWM signal * @pwm: PWM device * @polarity: new polarity of the PWM signal diff --git a/include/linux/pwm.h b/include/linux/pwm.h index e90628c..739cb2b 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -24,6 +24,12 @@ void pwm_free(struct pwm_device *pwm); int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns); /* + * pwm_config_alternate - change a PWM device configuration + * based on clk_dic and duty_percent + */ +int pwm_config_alternate(struct pwm_device *pwm, int clk_div, int duty_percent); + +/* * pwm_enable - start a PWM output toggling */ int pwm_enable(struct pwm_device *pwm); @@ -89,6 +95,8 @@ struct pwm_device { unsigned int period; /* in nanoseconds */ unsigned int duty_cycle; /* in nanoseconds */ + unsigned int clk_div; + unsigned int duty_percent; enum pwm_polarity polarity; }; @@ -114,6 +122,28 @@ static inline unsigned int pwm_get_duty_cycle(struct pwm_device *pwm) return pwm ? pwm->duty_cycle : 0; } +static inline void pwm_set_clk_div(struct pwm_device *pwm, unsigned int clk_div) +{ + if (pwm) + pwm->clk_div = clk_div; +} + +static inline unsigned int pwm_get_clk_div(struct pwm_device *pwm) +{ + return pwm ? pwm->clk_div : 0; +} + +static inline void pwm_set_duty_percent(struct pwm_device *pwm, unsigned int duty_percent) +{ + if (pwm) + pwm->duty_percent = duty_percent; +} + +static inline unsigned int pwm_get_duty_percent(struct pwm_device *pwm) +{ + return pwm ? pwm->duty_percent : 0; +} + /* * pwm_set_polarity - configure the polarity of a PWM signal */ @@ -138,6 +168,9 @@ struct pwm_ops { int (*config)(struct pwm_chip *chip, struct pwm_device *pwm, int duty_ns, int period_ns); + int (*config_alternate)(struct pwm_chip *chip, + struct pwm_device *pwm, + int clk_div, int duty_percent); int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm, enum pwm_polarity polarity);
Some chips instead of using period_ns and duty_ns can be configured using the clock divisor and duty percent. Adds an alternative configuration method for such chips v2: Corrected the chip validation for config_alternate in pwmchip_add CC: Samuel Ortiz <sameo@linux.intel.com> Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Alexandre Courbot <gnurou@gmail.com> Cc: Thierry Reding <thierry.reding@gmail.com> Signed-off-by: Shobhit Kumar <shobhit.kumar@intel.com> --- drivers/pwm/core.c | 27 ++++++++++++++++++++++++++- include/linux/pwm.h | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-)