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

[Alexandre Belloni]

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/