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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
