[V2,1/9] pwm: core: Add support for PWM HW driver with pwm capture only

Message ID 1521607244-29734-2-git-send-email-rrajk@nvidia.com
State New
Headers show
Series
  • Implementation of Tegra Tachometer driver
Related show

Commit Message

Rajkumar Rampelli March 21, 2018, 4:40 a.m.
Add support for pwm HW driver which has only capture functionality.
This helps to implement the PWM based Tachometer driver which reads
the PWM output signals from electronic fans.

PWM Tachometer captures the period and duty cycle of the PWM signal

Add conditional checks for callabacks enable(), disable(), config()
to check if they are supported by the client driver or not. Skip these
callbacks if they are not supported.

Signed-off-by: Rajkumar Rampelli <rrajk@nvidia.com>
---

V2: Added if conditional checks for pwm callbacks since drivers may
    implements only pwm capture functionality.

 drivers/pwm/core.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

Comments

Thierry Reding April 30, 2018, 9:51 a.m. | #1
On Wed, Mar 21, 2018 at 10:10:36AM +0530, Rajkumar Rampelli wrote:
> Add support for pwm HW driver which has only capture functionality.
> This helps to implement the PWM based Tachometer driver which reads
> the PWM output signals from electronic fans.
> 
> PWM Tachometer captures the period and duty cycle of the PWM signal
> 
> Add conditional checks for callabacks enable(), disable(), config()
> to check if they are supported by the client driver or not. Skip these
> callbacks if they are not supported.
> 
> Signed-off-by: Rajkumar Rampelli <rrajk@nvidia.com>
> ---
> 
> V2: Added if conditional checks for pwm callbacks since drivers may
>     implements only pwm capture functionality.
> 
>  drivers/pwm/core.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 1581f6a..f70fe68 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -246,6 +246,10 @@ static bool pwm_ops_check(const struct pwm_ops *ops)
>  	if (ops->apply)
>  		return true;
>  
> +	/* driver supports capture operation */
> +	if (ops->capture)
> +		return true;
> +
>  	return false;
>  }
>  
> @@ -495,7 +499,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>  			 * ->apply().
>  			 */
>  			if (pwm->state.enabled) {
> -				pwm->chip->ops->disable(pwm->chip, pwm);
> +				if (pwm->chip->ops->disable)
> +					pwm->chip->ops->disable(pwm->chip, pwm);

This is not a good idea. It means that you'll be able to successfully
configure a capture-only PWM channel for output. I think all of the
output configuration functions should return an error (-ENOSYS?) for
capture-only devices, much like we return -ENOSYS for pwm_capture() if
the driver doesn't implement capture support.

Thierry

Patch

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6a..f70fe68 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -246,6 +246,10 @@  static bool pwm_ops_check(const struct pwm_ops *ops)
 	if (ops->apply)
 		return true;
 
+	/* driver supports capture operation */
+	if (ops->capture)
+		return true;
+
 	return false;
 }
 
@@ -495,7 +499,8 @@  int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 			 * ->apply().
 			 */
 			if (pwm->state.enabled) {
-				pwm->chip->ops->disable(pwm->chip, pwm);
+				if (pwm->chip->ops->disable)
+					pwm->chip->ops->disable(pwm->chip, pwm);
 				pwm->state.enabled = false;
 			}
 
@@ -509,22 +514,26 @@  int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 
 		if (state->period != pwm->state.period ||
 		    state->duty_cycle != pwm->state.duty_cycle) {
-			err = pwm->chip->ops->config(pwm->chip, pwm,
+			if (pwm->chip->ops->config) {
+				err = pwm->chip->ops->config(pwm->chip, pwm,
 						     state->duty_cycle,
 						     state->period);
-			if (err)
-				return err;
+				if (err)
+					return err;
+			}
 
 			pwm->state.duty_cycle = state->duty_cycle;
 			pwm->state.period = state->period;
 		}
 
 		if (state->enabled != pwm->state.enabled) {
-			if (state->enabled) {
+			if (state->enabled && pwm->chip->ops->enable) {
 				err = pwm->chip->ops->enable(pwm->chip, pwm);
 				if (err)
 					return err;
-			} else {
+			}
+
+			if (!state->enabled && pwm->chip->ops->disable) {
 				pwm->chip->ops->disable(pwm->chip, pwm);
 			}