Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Claudiu.Beznea@microchip.com
Cc: linux-pwm@vger.kernel.org, alexandre.belloni@bootlin.com,
	Ludovic.Desroches@microchip.com, thierry.reding@gmail.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 4/3] pwm: atmel: document known weaknesses of both hardware and software
Date: Mon, 19 Aug 2019 17:20:35 +0200
Message-ID: <20190819152035.lkemndmypcehb2a2@pengutronix.de> (raw)
In-Reply-To: <3029467b-be14-d78b-5580-8bd3842d86e2@microchip.com>

Hello Claudiu,

On Mon, Aug 19, 2019 at 12:28:59PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 19.08.2019 13:46, Uwe Kleine-König wrote:
> > On Mon, Aug 19, 2019 at 09:26:04AM +0000, Claudiu.Beznea@microchip.com wrote:
> >> On 16.08.2019 12:37, Uwe Kleine-König wrote:
> >>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> >>> ---
> >>>  drivers/pwm/pwm-atmel.c | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> >>> index 42fe7bc043a8..1ddb93db9627 100644
> >>> --- a/drivers/pwm/pwm-atmel.c
> >>> +++ b/drivers/pwm/pwm-atmel.c
> >>> @@ -7,6 +7,16 @@
> >>>   *
> >>>   * Reference manual for "atmel,at91sam9rl-pwm":
> >>>   *   http://ww1.microchip.com/downloads/en/DeviceDoc/Atmel-11032-32-bit-ARM926EJ-S-Microcontroller-SAM9G25_Datasheet.pdf
> >>> + *
> >>> + * Limitations:
> >>> + * - Periods start with the inactive level.
> >>
> >> Are you talking here about the normal polarity (from documentation: By
> >> definition, normal polarity characterizes a signal starts high for the
> >> duration of the duty cycle and goes low for the remainder of the period.)
> > 
> > When .polarity = PWM_POLARITY_NORMAL is passed to atmel_pwm_apply() the
> > drivers sets PWM_CMR_CPOL=0 which according to the datasheet (linked
> > above) means: "The output waveform starts at a low level."
> > 
> > So maybe just the logic has to be inverted there,
> 
> Agree.
> 
> > but then maybe the
> > output gets active instead of inactive when the PWM is disabled.
> 
> Yes, this would happen. Playing again with CPOL when disabling may be a
> solution.

Alternatively you could argue that it would be more sensible to drop the
requirement for a certain output level on disable. You would have my
support here.

> > (Which in my book is ok, but it's Thierry's opinion that counts here.)
> > 
> >> If yes, this should be solved by playing with CPOL bit of CMR.
> >>
> >>> + * - Hardware has to be stopped in general to update settings.
> >>
> >> Sama5d2 has duty cycle that could be updated on the fly.
> > 
> > There is some functionality in the 9G25, too. I didn't understand it
> > completely but maybe it only helps updating one of period or duty cycle.
> >  
> >>> + *
> >>> + * Software bugs/possible improvements:
> >>> + * - When atmel_pwm_apply() is called with state->enabled=false a change in
> >>> + *   state->polarity isn't honored.
> >>
> >> I know that when configuring a PWM one should get the current state of the
> >> PWM, change it, then pass it to the driver via pwm_apply_state().
> > 
> > That seems to be a common pattern at least. IMHO letting the consumer
> > just configure the state that should be used should be fine, too.>
> >> In case one would call the pwm_apply_state() with state->enabled =
> >> false the state would be stored in PWM specific object (of type struct
> >> pwm_device). On the next apply, with enabled = true, all the PWM
> >> parameters would be actually applied to hardware. So, until
> >> enable=true the PWM state would only be cached by PWM core specific
> >> objects (in pwm_apply_state()).
> > 
> > I fail to follow what you mean here. If a PWM runs with (say) normal
> > polarity and you call pwm_apply_state(mypwm, { .polarity =
> > PWM_POLARITY_INVERSED, .enabled = false, }); the apply callback of the
> > lowlevel driver is called and supposed to configure the output to yield
> > a constant high.
> 
> Ok, I see it now. I'll put it on my queue.

See above. The atmel driver is just part of my quest to get a general
picture what the common PWM implementation does. Thierry argued that it
is natural that a PWM drives the inactive level on disable.

I'd say that it would be more natural to not demand a certain level
because a) IMHO there are enough implementations that differ here and b)
consumers could just configure for duty_cycle=0 if they care.

In the case of the imx driver we could just put aside the discussions
about how we atomically switch the output to a GPIO to provide the
needed level. In case of the atmel driver you'd just invert polarity and
be done.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-15 21:41 [PATCH 1/3] pwm: atmel: Add link to reference manual Uwe Kleine-König
2019-08-15 21:41 ` [PATCH 2/3] pwm: atmel: use a constant for maximum prescale value Uwe Kleine-König
2019-08-19  9:27   ` Claudiu.Beznea
2019-08-15 21:41 ` [PATCH 3/3] pwm: atmel: replace loop in prescale calculation by ad-hoc calculation Uwe Kleine-König
2019-08-19  9:27   ` Claudiu.Beznea
2019-08-16  9:37 ` [PATCH 4/3] pwm: atmel: document known weaknesses of both hardware and software Uwe Kleine-König
2019-08-16 20:43   ` Alexandre Belloni
2019-08-19  8:10     ` Uwe Kleine-König
2019-08-19  9:26   ` Claudiu.Beznea
2019-08-19 10:46     ` Uwe Kleine-König
2019-08-19 12:28       ` Claudiu.Beznea
2019-08-19 15:20         ` Uwe Kleine-König [this message]
2019-08-19  9:26 ` [PATCH 1/3] pwm: atmel: Add link to reference manual Claudiu.Beznea
2019-08-19  9:46   ` Nicolas.Ferre

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190819152035.lkemndmypcehb2a2@pengutronix.de \
    --to=u.kleine-koenig@pengutronix.de \
    --cc=Claudiu.Beznea@microchip.com \
    --cc=Ludovic.Desroches@microchip.com \
    --cc=alexandre.belloni@bootlin.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-pwm@vger.kernel.org \
    --cc=thierry.reding@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git