[PATCH v3 5/5] pwm: airoha: Add support for EN7581 SoC

[Uwe Kleine-König]

Hello Benjamin,

On Thu, Sep 05, 2024 at 08:35:17PM +0200, Benjamin Larsson wrote:
> On 05/09/2024 17:39, Uwe Kleine-König wrote:
> > On Thu, Sep 05, 2024 at 02:18:41PM +0200, Benjamin Larsson wrote:
> > > On 2024-09-05 11:30, Uwe Kleine-König wrote:
> > > > 1 second long pulses with a period size of 1 second, so a constant high
> > > > signal?
> > > Hi, I think I was unclear. The SoC documentation is not that detailed. But I
> > > think I understand how it works now.
> > > 
> > > One register contains the minimum duration (d_min). And then there is one 8
> > > bit register for the signal low period (lp) and then another 8bit register
> > > for the high period (hp). Per my understanding a change of polarity is then
> > > just a swap of lp and hp.
> > that doesn't sound right.
> > 
> > A "normal" waveform with period = 10 ns and duty_cycle = 2 ns looks as
> > follows:
> > 
> >     _         _         _
> >    / \_______/ \_______/ \_______/
> >    ^         ^         ^         ^
> > 
> > assuming a monospace font that's 1 char per ns, the ^ marking the period
> > start.
> > 
> > Ignoring scaling, your hardware needs to have hp = 2 and lp = 8. If you
> > switch that (assuming you mean switching in the same way as I do) to hp
> > = 8 and lp = 2, you get:
> > 
> >     _______   _______   _______
> >    /       \_/       \_/       \_/
> >    ^         ^         ^         ^
> > 
> > which is still a "normal" polarity output as a period starts with the
> > active part.
> > 
> > I admit that's a bit artificial, because the waveform for
> > 
> > 	period = 10 ns
> > 	duty_cycle = 2 ns
> > 	polarity = inversed
> > 
> > looks as follows:
> > 
> >       _______   _______   _______
> >    \_/       \_/       \_/       \_/
> >    ^         ^         ^         ^
> > 
> > which isn't any different from the 2nd waveform above if you ignore the
> > period start markers (which are not observable apart from the moments
> > where you reconfigure the output).
> > 
> > However it matters if you have a chip with >1 output that are not
> > independent.
> 
> 
> Ok that was a clear explanation,

\o/

> anyway the pwm hardware is then not capable
> of a polarity change. It is possible to change the polarity via other means
> but there is no way for the pwm block (and driver) to handle this.

That's ok. Just do something like

	if (state->polarity != PWM_POLARITY_NORMAL)
		return -EINVAL;

It's quite usual that drivers only support a single polarity.

> > > This means that when requesting a period and duty cycle you need to search
> > > through the configuration space to find the optimal value.
> > Or restrict yourself consistently to something simpler than a exhaustive
> > search through the complete configuration space.
> 
> Is there a recommendation on what is more important? Period duration or duty
> cycle percentage?

That really depends on your usage domain. Just pick an algorithm that is
sound, ideally easy to review and serves your purpose. If you pick
something that is too simple for the next consumer, we can add the
needed complexity still later.

So in my book even something like restricting the period to a single
fixed value and just modify the duty cycle is fine. In that case add a
comment that there is room for improvement and I'm happy.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-mediatek/attachments/20240906/692aa83a/attachment-0001.sig>