Re: [PATCHv5 1/2] pwm: Add Allwinner SoC support

[jonsmirl@xxxxxxxxx]

I was unaware of this driver and reimplemented it. Patch is attached.
Some differences I noticed...

1) I implemented bypass mode
2) I had to do math in picoseconds to avoid round off/truncation errors.
3) counter registers are different lengths on SUN4 vs rest.

Can someone with a scope verify if the prescaler of 1 works at high
frequencies? Like cycles/active of 2/1, 3/1, 4,1...

On Mon, Jun 23, 2014 at 1:01 PM, Alexandre Belloni
<alexandre.belloni@xxxxxxxxxxxxxxxxxx> wrote:
> On 18/06/2014 at 01:26:06 +0200, Thierry Reding wrote :
>> On Mon, May 19, 2014 at 08:10:02PM +0200, Alexandre Belloni wrote:
>> > +   /* By default, the polarity is inversed, set it to normal */
>> > +   sunxi_pwm_writel(sunxi_pwm, PWM_CTRL_REG,
>> > +                    BIT_CH(PWM_ACT_STATE, 0) |
>> > +                    BIT_CH(PWM_ACT_STATE, 1));
>> > +   clk_disable_unprepare(sunxi_pwm->clk);
>>
>> Why do you need to do this here? Doesn't this potentially cause
>> transients if a bootloader had this configured with inversed polarity?
>
>
> It was done a few months ago but what I remember is the following
> happens:
>
> The PWM subsystem assumes that the polarity is PWM_POLARITY_NORMAL
> because of the kzalloc pwmchip_add(). Would you prefer something like:
>
>         val = sunxi_pwm_readl(sunxi_pwm, PWM_CTRL_REG);
>         for (i = 0; i < sunxi_pwm->chip.npwm; i++) {
>                 if (!(val & BIT_CH(PWM_ACT_STATE, i)))
>                         sunxi_pwm->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
>         }
>
> Then, you would have a race where the PWM polarity is not correct in
> sysfs between pwmchip_add() and that code.
>
> Also, if you want to preserve the state set by the bootloader, you
> actually have an issue with getting back the other members of the
> pwm_device struct (duty, period) and more importantly the PWMF_ENABLED
> flag. It now assumed that the PWM channel is not enabled when
> registering the chip. If you now say that it may be enabled before linux
> is booting and you want to keep it running, then you have an
> inconsistency between the real state of the PWM (enabled, with a duty,
> period and polarity set) and what the PWM susbsytem actually knows about
> the PWM (not enabled, duty and period == 0 and polarity is normal).
>
> I would agree that the usual use case would be that another driver will
> take the PWM and set the duty, period and polarity anyway but the issue
> with the PWMF_ENABLED flag remains.
>
> How do you want to fix this? Would you add a new callback that would be
> called by pwmchip_add(), before pwmchip_sysfs_export()?
>
> I actually find it ugly to set the pwm_device members from the probe,
> especially the flags. I would prefer they stay hidden by the API.
>
>
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com
> --
> 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/



-- 
Jon Smirl
jonsmirl@xxxxxxxxx

Attachment: pwm
Description: Binary data