Re: [PATCH v2 2/4] pwm: kona: Fix incorrect enable after channel polarity change

[Tim Kryger]

On Fri, Nov 28, 2014 at 3:48 PM, Arun Ramamurthy
<arun.ramamurthy@xxxxxxxxxxxx> wrote:
>
>
> On 14-11-25 10:22 PM, Tim Kryger wrote:
>>
>> On Tue, Nov 25, 2014 at 11:40 AM, Scott Branden <sbranden@xxxxxxxxxxxx>
>> wrote:
>>>
>>> From: Arun Ramamurthy <arunrama@xxxxxxxxxxxx>
>>>
>>> The pwm core code requires a separate call for enabling the channel
>>> and hence the driver does not need to set pwm_trigger after a
>>> polarity change
>>
>>
>> The framework does restrict when polarity changes can occur but it
>> isn't clear to me that there is any reason to delay applying the
>> polarity change.
>
> I examined several other drivers such as pwm-atmel-tcb.c, pwm-ep93xx.c,
> pwm-renesas-tpu.c, pwm-samsung.c in the 3.17 kernel tree and none of them
> enable the channel after changing polarity. We would be the first driver to
> do so.

We are not "enabling" the channel as much as we are "triggering an
application of settings" by hardware.

Both pwm-ep93xx and pwm-samsung write polarity settings to the
hardware immediately, presumably resulting in an immediate change in
output.

Alternatively, the pwm-atmel-tcb and pwm-renesas-tpu drivers save the
polarity and write it to hardware later.

There may be advantages to deferring the application of the polarity
till a subsequent enable but both approaches appear to be acceptable.

>
>> Keep in mind that polarity matters even when a PWM
>> is disabled.  While disabled, the output should be equivalent to an
>> enabled configuration with zero duty.  Thus for normal polarity the
>> output is constant low and for inversed polarity the output is
>> constant high.
>
> The driver does set the duty cycle to zero when disabling the pwm
> channel.However since the frame work prevents polarity change when the pwm
> is enabled, I donât see how one could expect the polarity change to be
> reflected immediately without a separate call to pwm enable.
>
>
>> I believe there is an expectation that the output is
>> updated to reflect the requested polarity change prior to returning to
>> the caller.
>
>
> Once again I disagree with this based on other pwm drivers which only change
> the polarity and do not enable the channel when their set polarity functions
> are called.

I don't know why you keep calling this an enable.  Its not an enable,
it is only a trigger.

Perhaps this would be best explained with an example:

# Export PWM for access by userspace
cd /sys/class/pwm/pwmchip0
echo 0 > export
cd pwm0

# Request 50% duty output when PWM is enabled
echo 50000 > duty_cycle
echo 100000 > period

# Command Inversed Polarity
echo inversed > polarity

# Command Normal Polarity
echo normal > polarity

# Enable PWM
echo 1 > enable

The polarity changes trigger immediate output updates but the PWM is
not enabled until the end.

Prior to the last step the output is either a constant high or low
signal, not the 50% duty waveform.

>
>
>>
>>>
>>> Signed-off-by: Arun Ramamurthy <arunrama@xxxxxxxxxxxx>
>>> Reviewed-by: Ray Jui <rjui@xxxxxxxxxxxx>
>>> Signed-off-by: Scott Branden <sbranden@xxxxxxxxxxxx>
>>> ---
>>>   drivers/pwm/pwm-bcm-kona.c | 5 -----
>>>   1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-bcm-kona.c b/drivers/pwm/pwm-bcm-kona.c
>>> index 29eef9e..fa0b5bf 100644
>>> --- a/drivers/pwm/pwm-bcm-kona.c
>>> +++ b/drivers/pwm/pwm-bcm-kona.c
>>> @@ -173,11 +173,6 @@ static int kona_pwmc_set_polarity(struct pwm_chip
>>> *chip, struct pwm_device *pwm,
>>>
>>>          writel(value, kp->base + PWM_CONTROL_OFFSET);
>>>
>>> -       kona_pwmc_apply_settings(kp, chan);
>>> -
>>> -       /* Wait for waveform to settle before gating off the clock */
>>> -       ndelay(400);
>>> -
>>>          clk_disable_unprepare(kp->clk);
>>>
>>>          return 0;
>>> --
>>> 2.1.3
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/