linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Support for configuring polarity of PWM
@ 2012-08-23  7:00 Philip, Avinash
  2012-08-23  7:00 ` [PATCH 1/2] pwm: pwm-tiecap: Add support " Philip, Avinash
  2012-08-23  7:00 ` [PATCH 2/2] pwm: pwm-tiehrpwm: " Philip, Avinash
  0 siblings, 2 replies; 5+ messages in thread
From: Philip, Avinash @ 2012-08-23  7:00 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-kernel, nsekhar, gururaja.hebbar, Philip, Avinash

These patch set adds support for configuring polarity of eCAP & eHRPWM.

Philip, Avinash (2):
  pwm: pwm-tiecap: Add support for configuring polarity of PWM
  pwm: pwm-tiehrpwm: Add support for configuring polarity of PWM

 drivers/pwm/pwm-tiecap.c   |   22 +++++++++
 drivers/pwm/pwm-tiehrpwm.c |  104 +++++++++++++++++++++++++++++++-------------
 2 files changed, 96 insertions(+), 30 deletions(-)


^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH 1/2] pwm: pwm-tiecap: Add support for configuring polarity of PWM
  2012-08-23  7:00 [PATCH 0/2] Support for configuring polarity of PWM Philip, Avinash
@ 2012-08-23  7:00 ` Philip, Avinash
  2012-08-23  7:00 ` [PATCH 2/2] pwm: pwm-tiehrpwm: " Philip, Avinash
  1 sibling, 0 replies; 5+ messages in thread
From: Philip, Avinash @ 2012-08-23  7:00 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-kernel, nsekhar, gururaja.hebbar, Philip, Avinash

ECAP APWM hardware supports polarity configuration of PWM output.
This commit adds support for polarity configuration of ECAP APWM.

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>

:100644 100644 4b66889... a3d21e7... M	drivers/pwm/pwm-tiecap.c

diff --git a/drivers/pwm/pwm-tiecap.c b/drivers/pwm/pwm-tiecap.c
index 4b66889..a3d21e7 100644
--- a/drivers/pwm/pwm-tiecap.c
+++ b/drivers/pwm/pwm-tiecap.c
@@ -32,6 +32,7 @@
 #define CAP3			0x10
 #define CAP4			0x14
 #define ECCTL2			0x2A
+#define ECCTL2_APWM_POL_LOW	BIT(10)
 #define ECCTL2_APWM_MODE	BIT(9)
 #define ECCTL2_SYNC_SEL_DISA	(BIT(7) | BIT(6))
 #define ECCTL2_TSCTR_FREERUN	BIT(4)
@@ -111,6 +112,26 @@ static int ecap_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+static int ecap_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+		enum pwm_polarity polarity)
+{
+	struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip);
+	unsigned short reg_val;
+
+	pm_runtime_get_sync(pc->chip.dev);
+	reg_val = readw(pc->mmio_base + ECCTL2);
+	if (polarity == PWM_POLARITY_INVERSED)
+		/* Duty cycle defines Low period of PWM */
+		reg_val |= ECCTL2_APWM_POL_LOW;
+	else
+		/* Duty cycle defines High period of PWM */
+		reg_val &= ~ECCTL2_APWM_POL_LOW;
+
+	writew(reg_val, pc->mmio_base + ECCTL2);
+	pm_runtime_put_sync(pc->chip.dev);
+	return 0;
+}
+
 static int ecap_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct ecap_pwm_chip *pc = to_ecap_pwm_chip(chip);
@@ -157,6 +178,7 @@ static void ecap_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 static const struct pwm_ops ecap_pwm_ops = {
 	.free		= ecap_pwm_free,
 	.config		= ecap_pwm_config,
+	.set_polarity	= ecap_pwm_set_polarity,
 	.enable		= ecap_pwm_enable,
 	.disable	= ecap_pwm_disable,
 	.owner		= THIS_MODULE,
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [PATCH 2/2] pwm: pwm-tiehrpwm: Add support for configuring polarity of PWM
  2012-08-23  7:00 [PATCH 0/2] Support for configuring polarity of PWM Philip, Avinash
  2012-08-23  7:00 ` [PATCH 1/2] pwm: pwm-tiecap: Add support " Philip, Avinash
@ 2012-08-23  7:00 ` Philip, Avinash
  2012-08-24 16:53   ` Thierry Reding
  1 sibling, 1 reply; 5+ messages in thread
From: Philip, Avinash @ 2012-08-23  7:00 UTC (permalink / raw)
  To: thierry.reding; +Cc: linux-kernel, nsekhar, gururaja.hebbar, Philip, Avinash

EHRPWM hardware supports polarity configuration of PWM output. However
configuration of polarity done in hardware only in .enable() to ensure
PWM output present only after enabling PWM. This commit adds support for
polarity configuration for EHRPWM.
When being here, remove configuring of polarity during .config() and do
it explicitly from .set_polarity().

Signed-off-by: Philip, Avinash <avinashphilip@ti.com>

:100644 100644 c3756d1... 99e5247... M	drivers/pwm/pwm-tiehrpwm.c

diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
index c3756d1..99e5247 100644
--- a/drivers/pwm/pwm-tiehrpwm.c
+++ b/drivers/pwm/pwm-tiehrpwm.c
@@ -81,6 +81,15 @@
 #define AQCTL_ZRO_FRCHIGH	BIT(1)
 #define AQCTL_ZRO_FRCTOGGLE	(BIT(1) | BIT(0))
 
+#define AQCTL_CHANA_POLNORMAL	(AQCTL_CAU_FRCHIGH | AQCTL_PRD_FRCLOW | \
+				AQCTL_ZRO_FRCLOW)
+#define AQCTL_CHANA_POLINVERSED	(AQCTL_CAU_FRCLOW | AQCTL_PRD_FRCHIGH | \
+				AQCTL_ZRO_FRCHIGH)
+#define AQCTL_CHANB_POLNORMAL	(AQCTL_CBU_FRCHIGH | AQCTL_PRD_FRCLOW | \
+				AQCTL_ZRO_FRCLOW)
+#define AQCTL_CHANB_POLINVERSED	(AQCTL_CBU_FRCLOW | AQCTL_PRD_FRCHIGH | \
+				AQCTL_ZRO_FRCHIGH)
+
 #define AQSFRC_RLDCSF_MASK	(BIT(7) | BIT(6))
 #define AQSFRC_RLDCSF_ZRO	0
 #define AQSFRC_RLDCSF_PRD	BIT(6)
@@ -100,10 +109,17 @@
 
 #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
 
+enum config {
+	config_dutycycle,
+	config_polarity,
+};
+
 struct ehrpwm_pwm_chip {
 	struct pwm_chip	chip;
 	unsigned int	clk_rate;
 	void __iomem	*mmio_base;
+	unsigned long duty_cycles;
+	enum pwm_polarity polarity;
 };
 
 static inline struct ehrpwm_pwm_chip *to_ehrpwm_pwm_chip(struct pwm_chip *chip)
@@ -165,38 +181,50 @@ static int set_prescale_div(unsigned long rqst_prescaler,
 }
 
 static void configure_chans(struct ehrpwm_pwm_chip *pc, int chan,
-		unsigned long duty_cycles)
+		enum config config)
 {
-	int cmp_reg, aqctl_reg;
-	unsigned short aqctl_val, aqctl_mask;
+	if (config == config_dutycycle) {
+		int cmp_reg;
+
+		if (chan == 1)
+			/* Channel 1 configured with compare B register */
+			cmp_reg = CMPB;
+		else
+			/* Channel 0 configured with compare A register */
+			cmp_reg = CMPA;
+
+		ehrpwm_write(pc->mmio_base, cmp_reg, pc->duty_cycles);
+	} else if (config == config_polarity) {
+		int aqctl_reg;
+		unsigned short aqctl_val, aqctl_mask;
+
+		/*
+		 * Configure PWM output to HIGH/LOW level on counter
+		 * reaches compare register value and LOW/HIGH level
+		 * on counter value reaches period register value and
+		 * zero value on counter
+		 */
+		if (chan == 1) {
+			aqctl_reg = AQCTLB;
+			aqctl_mask = AQCTL_CBU_MASK;
+
+			if (pc->polarity == PWM_POLARITY_INVERSED)
+				aqctl_val = AQCTL_CHANB_POLINVERSED;
+			else
+				aqctl_val = AQCTL_CHANB_POLNORMAL;
+		} else {
+			aqctl_reg = AQCTLA;
+			aqctl_mask = AQCTL_CAU_MASK;
+
+			if (pc->polarity == PWM_POLARITY_INVERSED)
+				aqctl_val = AQCTL_CHANA_POLINVERSED;
+			else
+				aqctl_val = AQCTL_CHANA_POLNORMAL;
+		}
 
-	/*
-	 * Channels can be configured from action qualifier module.
-	 * Channel 0 configured with compare A register and for
-	 * up-counter mode.
-	 * Channel 1 configured with compare B register and for
-	 * up-counter mode.
-	 */
-	if (chan == 1) {
-		aqctl_reg = AQCTLB;
-		cmp_reg = CMPB;
-		/* Configure PWM Low from compare B value */
-		aqctl_val = AQCTL_CBU_FRCLOW;
-		aqctl_mask = AQCTL_CBU_MASK;
-	} else {
-		cmp_reg = CMPA;
-		aqctl_reg = AQCTLA;
-		/* Configure PWM Low from compare A value*/
-		aqctl_val = AQCTL_CAU_FRCLOW;
-		aqctl_mask = AQCTL_CAU_MASK;
+		aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
+		ehrpwm_modify(pc->mmio_base, aqctl_reg, aqctl_mask, aqctl_val);
 	}
-
-	/* Configure PWM High from period value and zero value */
-	aqctl_val |= AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH;
-	aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
-	ehrpwm_modify(pc->mmio_base,  aqctl_reg, aqctl_mask, aqctl_val);
-
-	ehrpwm_write(pc->mmio_base,  cmp_reg, duty_cycles);
 }
 
 /*
@@ -254,12 +282,24 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CTRMODE_MASK,
 			TBCTL_CTRMODE_UP);
 
+	pc->duty_cycles = duty_cycles;
 	/* Configure the channel for duty cycle */
-	configure_chans(pc, pwm->hwpwm, duty_cycles);
+	configure_chans(pc, pwm->hwpwm, config_dutycycle);
+
 	pm_runtime_put_sync(chip->dev);
 	return 0;
 }
 
+static int ehrpwm_pwm_set_polarity(struct pwm_chip *chip,
+		struct pwm_device *pwm,	enum pwm_polarity polarity)
+{
+	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
+
+	/* Configuration of polarity in hardware delayed done at enable */
+	pc->polarity = polarity;
+	return 0;
+}
+
 static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
@@ -283,6 +323,9 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
 
+	/* Channels Polarity can be configured from action qualifier module */
+	configure_chans(pc, pwm->hwpwm, config_polarity);
+
 	/* Enable time counter for free_run */
 	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_RUN_MASK, TBCTL_FREE_RUN);
 	return 0;
@@ -329,6 +372,7 @@ static void ehrpwm_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 static const struct pwm_ops ehrpwm_pwm_ops = {
 	.free		= ehrpwm_pwm_free,
 	.config		= ehrpwm_pwm_config,
+	.set_polarity	= ehrpwm_pwm_set_polarity,
 	.enable		= ehrpwm_pwm_enable,
 	.disable	= ehrpwm_pwm_disable,
 	.owner		= THIS_MODULE,
-- 
1.7.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH 2/2] pwm: pwm-tiehrpwm: Add support for configuring polarity of PWM
  2012-08-23  7:00 ` [PATCH 2/2] pwm: pwm-tiehrpwm: " Philip, Avinash
@ 2012-08-24 16:53   ` Thierry Reding
  2012-08-28  4:12     ` Philip, Avinash
  0 siblings, 1 reply; 5+ messages in thread
From: Thierry Reding @ 2012-08-24 16:53 UTC (permalink / raw)
  To: Philip, Avinash; +Cc: linux-kernel, nsekhar, gururaja.hebbar

[-- Attachment #1: Type: text/plain, Size: 4767 bytes --]

On Thu, Aug 23, 2012 at 12:30:51PM +0530, Philip, Avinash wrote:
[...]
> diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
[...]
> @@ -100,10 +109,17 @@
>  
>  #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
>  
> +enum config {
> +	config_dutycycle,
> +	config_polarity,
> +};
> +

I don't think this makes sense, see below.

> @@ -165,38 +181,50 @@ static int set_prescale_div(unsigned long rqst_prescaler,
>  }
>  
>  static void configure_chans(struct ehrpwm_pwm_chip *pc, int chan,
> -		unsigned long duty_cycles)
> +		enum config config)
>  {
> -	int cmp_reg, aqctl_reg;
> -	unsigned short aqctl_val, aqctl_mask;
> +	if (config == config_dutycycle) {
> +		int cmp_reg;
> +
> +		if (chan == 1)
> +			/* Channel 1 configured with compare B register */
> +			cmp_reg = CMPB;
> +		else
> +			/* Channel 0 configured with compare A register */
> +			cmp_reg = CMPA;
> +
> +		ehrpwm_write(pc->mmio_base, cmp_reg, pc->duty_cycles);
> +	} else if (config == config_polarity) {
> +		int aqctl_reg;
> +		unsigned short aqctl_val, aqctl_mask;
> +
> +		/*
> +		 * Configure PWM output to HIGH/LOW level on counter
> +		 * reaches compare register value and LOW/HIGH level
> +		 * on counter value reaches period register value and
> +		 * zero value on counter
> +		 */
> +		if (chan == 1) {
> +			aqctl_reg = AQCTLB;
> +			aqctl_mask = AQCTL_CBU_MASK;
> +
> +			if (pc->polarity == PWM_POLARITY_INVERSED)
> +				aqctl_val = AQCTL_CHANB_POLINVERSED;
> +			else
> +				aqctl_val = AQCTL_CHANB_POLNORMAL;
> +		} else {
> +			aqctl_reg = AQCTLA;
> +			aqctl_mask = AQCTL_CAU_MASK;
> +
> +			if (pc->polarity == PWM_POLARITY_INVERSED)
> +				aqctl_val = AQCTL_CHANA_POLINVERSED;
> +			else
> +				aqctl_val = AQCTL_CHANA_POLNORMAL;
> +		}
>  
> -	/*
> -	 * Channels can be configured from action qualifier module.
> -	 * Channel 0 configured with compare A register and for
> -	 * up-counter mode.
> -	 * Channel 1 configured with compare B register and for
> -	 * up-counter mode.
> -	 */
> -	if (chan == 1) {
> -		aqctl_reg = AQCTLB;
> -		cmp_reg = CMPB;
> -		/* Configure PWM Low from compare B value */
> -		aqctl_val = AQCTL_CBU_FRCLOW;
> -		aqctl_mask = AQCTL_CBU_MASK;
> -	} else {
> -		cmp_reg = CMPA;
> -		aqctl_reg = AQCTLA;
> -		/* Configure PWM Low from compare A value*/
> -		aqctl_val = AQCTL_CAU_FRCLOW;
> -		aqctl_mask = AQCTL_CAU_MASK;
> +		aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
> +		ehrpwm_modify(pc->mmio_base, aqctl_reg, aqctl_mask, aqctl_val);
>  	}
> -
> -	/* Configure PWM High from period value and zero value */
> -	aqctl_val |= AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH;
> -	aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
> -	ehrpwm_modify(pc->mmio_base,  aqctl_reg, aqctl_mask, aqctl_val);
> -
> -	ehrpwm_write(pc->mmio_base,  cmp_reg, duty_cycles);
>  }

I think it might be better to split this into two separate functions.
Both branches have absolutely no code in common, so splitting them off
would decrease the indentation level and make this much more readable
and wouldn't require the configuration enumeration from above.

>  
>  /*
> @@ -254,12 +282,24 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CTRMODE_MASK,
>  			TBCTL_CTRMODE_UP);
>  
> +	pc->duty_cycles = duty_cycles;
>  	/* Configure the channel for duty cycle */
> -	configure_chans(pc, pwm->hwpwm, duty_cycles);
> +	configure_chans(pc, pwm->hwpwm, config_dutycycle);
> +
>  	pm_runtime_put_sync(chip->dev);
>  	return 0;
>  }
>  
> +static int ehrpwm_pwm_set_polarity(struct pwm_chip *chip,
> +		struct pwm_device *pwm,	enum pwm_polarity polarity)
> +{
> +	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> +
> +	/* Configuration of polarity in hardware delayed done at enable */
> +	pc->polarity = polarity;
> +	return 0;
> +}
> +

The problem here, which is true for both of the .set_polarity() and
.config() implementations is that both channels share a single duty
cycle and polarity. What if the two channels are configured with
conflicting settings? Shouldn't that at least give a warning or even
return an error?

>  static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  {
>  	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> @@ -283,6 +323,9 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  
>  	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
>  
> +	/* Channels Polarity can be configured from action qualifier module */
> +	configure_chans(pc, pwm->hwpwm, config_polarity);
> +

What's this "action qualifier module"?

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH 2/2] pwm: pwm-tiehrpwm: Add support for configuring polarity of PWM
  2012-08-24 16:53   ` Thierry Reding
@ 2012-08-28  4:12     ` Philip, Avinash
  0 siblings, 0 replies; 5+ messages in thread
From: Philip, Avinash @ 2012-08-28  4:12 UTC (permalink / raw)
  To: Thierry Reding; +Cc: linux-kernel, Nori, Sekhar, Hebbar, Gururaja

On Fri, Aug 24, 2012 at 22:23:14, Thierry Reding wrote:
> On Thu, Aug 23, 2012 at 12:30:51PM +0530, Philip, Avinash wrote:
> [...]
> > diff --git a/drivers/pwm/pwm-tiehrpwm.c b/drivers/pwm/pwm-tiehrpwm.c
> [...]
> > @@ -100,10 +109,17 @@
> >  
> >  #define NUM_PWM_CHANNEL		2	/* EHRPWM channels */
> >  
> > +enum config {
> > +	config_dutycycle,
> > +	config_polarity,
> > +};
> > +
> 
> I don't think this makes sense, see below.
> 
> > @@ -165,38 +181,50 @@ static int set_prescale_div(unsigned long rqst_prescaler,
> >  }
> >  
> >  static void configure_chans(struct ehrpwm_pwm_chip *pc, int chan,
> > -		unsigned long duty_cycles)
> > +		enum config config)
> >  {
> > -	int cmp_reg, aqctl_reg;
> > -	unsigned short aqctl_val, aqctl_mask;
> > +	if (config == config_dutycycle) {
> > +		int cmp_reg;
> > +
> > +		if (chan == 1)
> > +			/* Channel 1 configured with compare B register */
> > +			cmp_reg = CMPB;
> > +		else
> > +			/* Channel 0 configured with compare A register */
> > +			cmp_reg = CMPA;
> > +
> > +		ehrpwm_write(pc->mmio_base, cmp_reg, pc->duty_cycles);
> > +	} else if (config == config_polarity) {
> > +		int aqctl_reg;
> > +		unsigned short aqctl_val, aqctl_mask;
> > +
> > +		/*
> > +		 * Configure PWM output to HIGH/LOW level on counter
> > +		 * reaches compare register value and LOW/HIGH level
> > +		 * on counter value reaches period register value and
> > +		 * zero value on counter
> > +		 */
> > +		if (chan == 1) {
> > +			aqctl_reg = AQCTLB;
> > +			aqctl_mask = AQCTL_CBU_MASK;
> > +
> > +			if (pc->polarity == PWM_POLARITY_INVERSED)
> > +				aqctl_val = AQCTL_CHANB_POLINVERSED;
> > +			else
> > +				aqctl_val = AQCTL_CHANB_POLNORMAL;
> > +		} else {
> > +			aqctl_reg = AQCTLA;
> > +			aqctl_mask = AQCTL_CAU_MASK;
> > +
> > +			if (pc->polarity == PWM_POLARITY_INVERSED)
> > +				aqctl_val = AQCTL_CHANA_POLINVERSED;
> > +			else
> > +				aqctl_val = AQCTL_CHANA_POLNORMAL;
> > +		}
> >  
> > -	/*
> > -	 * Channels can be configured from action qualifier module.
> > -	 * Channel 0 configured with compare A register and for
> > -	 * up-counter mode.
> > -	 * Channel 1 configured with compare B register and for
> > -	 * up-counter mode.
> > -	 */
> > -	if (chan == 1) {
> > -		aqctl_reg = AQCTLB;
> > -		cmp_reg = CMPB;
> > -		/* Configure PWM Low from compare B value */
> > -		aqctl_val = AQCTL_CBU_FRCLOW;
> > -		aqctl_mask = AQCTL_CBU_MASK;
> > -	} else {
> > -		cmp_reg = CMPA;
> > -		aqctl_reg = AQCTLA;
> > -		/* Configure PWM Low from compare A value*/
> > -		aqctl_val = AQCTL_CAU_FRCLOW;
> > -		aqctl_mask = AQCTL_CAU_MASK;
> > +		aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
> > +		ehrpwm_modify(pc->mmio_base, aqctl_reg, aqctl_mask, aqctl_val);
> >  	}
> > -
> > -	/* Configure PWM High from period value and zero value */
> > -	aqctl_val |= AQCTL_PRD_FRCHIGH | AQCTL_ZRO_FRCHIGH;
> > -	aqctl_mask |= AQCTL_PRD_MASK | AQCTL_ZRO_MASK;
> > -	ehrpwm_modify(pc->mmio_base,  aqctl_reg, aqctl_mask, aqctl_val);
> > -
> > -	ehrpwm_write(pc->mmio_base,  cmp_reg, duty_cycles);
> >  }
> 
> I think it might be better to split this into two separate functions.
> Both branches have absolutely no code in common, so splitting them off
> would decrease the indentation level and make this much more readable
> and wouldn't require the configuration enumeration from above.

EHRPWM supports two channels & can be configured for duty cycle
and polarity independently. That's why I wanted to use channel
configuration API.
I will correct it.

> 
> >  
> >  /*
> > @@ -254,12 +282,24 @@ static int ehrpwm_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  	ehrpwm_modify(pc->mmio_base, TBCTL, TBCTL_CTRMODE_MASK,
> >  			TBCTL_CTRMODE_UP);
> >  
> > +	pc->duty_cycles = duty_cycles;
> >  	/* Configure the channel for duty cycle */
> > -	configure_chans(pc, pwm->hwpwm, duty_cycles);
> > +	configure_chans(pc, pwm->hwpwm, config_dutycycle);
> > +
> >  	pm_runtime_put_sync(chip->dev);
> >  	return 0;
> >  }
> >  
> > +static int ehrpwm_pwm_set_polarity(struct pwm_chip *chip,
> > +		struct pwm_device *pwm,	enum pwm_polarity polarity)
> > +{
> > +	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> > +
> > +	/* Configuration of polarity in hardware delayed done at enable */
> > +	pc->polarity = polarity;
> > +	return 0;
> > +}
> > +
> 
> The problem here, which is true for both of the .set_polarity() and
> .config() implementations is that both channels share a single duty
> cycle and polarity. What if the two channels are configured with
> conflicting settings? Shouldn't that at least give a warning or even
> return an error?

I missed this. I will correct it.

> 
> >  static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  {
> >  	struct ehrpwm_pwm_chip *pc = to_ehrpwm_pwm_chip(chip);
> > @@ -283,6 +323,9 @@ static int ehrpwm_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> >  
> >  	ehrpwm_modify(pc->mmio_base, AQCSFRC, aqcsfrc_mask, aqcsfrc_val);
> >  
> > +	/* Channels Polarity can be configured from action qualifier module */
> > +	configure_chans(pc, pwm->hwpwm, config_polarity);
> > +
> 
> What's this "action qualifier module"?

It is one of the sub modules in EHRPWM used for configuring channel PWM features 
(Polarity, Duty cycle, PWM output state etc.)

Thanks
Avinash

> 
> Thierry
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-08-28  4:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-23  7:00 [PATCH 0/2] Support for configuring polarity of PWM Philip, Avinash
2012-08-23  7:00 ` [PATCH 1/2] pwm: pwm-tiecap: Add support " Philip, Avinash
2012-08-23  7:00 ` [PATCH 2/2] pwm: pwm-tiehrpwm: " Philip, Avinash
2012-08-24 16:53   ` Thierry Reding
2012-08-28  4:12     ` Philip, Avinash

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).