Re: [PATCH v4] media: rc: pwm-ir-tx: Switch to atomic PWM API

[Uwe Kleine-König]

On Sun, Oct 31, 2021 at 10:39:34AM +0000, Sean Young wrote:
> Hi Uwe,
> 
> On Thu, Oct 28, 2021 at 01:15:35PM +0200, Uwe Kleine-König wrote:
> > On Thu, Oct 28, 2021 at 10:14:42AM +0100, Sean Young wrote:
> > > We still have the problem that the pwm drivers calculate the period
> > > incorrectly by rounding down (except pwm-bcm2835). So the period is not
> > > as good as it could be in most cases, but this driver can't do anything
> > > about that.
> > 
> > Yeah, some time ago I started coding a round_state function
> > (wip at
> > https://git.pengutronix.de/cgit/ukl/linux/commit/?h=pwm-wip&id=ae348eb6a55d6526f30ef4a49819197d9616391e)
> > but this was pushed down on my todo-list by more important stuff.
> > 
> > If you want to experiment with that ...
> 
> I was thinking about this problem this morning. 
> 
> - The pwm-ir-tx driver gets a carrier set in Hz, which it has to convert to
>   a period (1e9 / carrier). There is loss of accuracy there.

Ack, and the loss is known.

> - When it gets to the pwm driver, the period is converted into the format
>   the pwm hardware expects. For example the pwm-bcm2835 driver converts
>   it into clock cycles (1e9 / 8e8).
> 
> Both calculations involve loss of accuracy because of integer representation.
> 
> Would it make more sense for the pwm interface to use numer/denom rational
> numbers?

This is quite a complication because then all lowlevel driver needs to
do fractal math. I don't want to open that can of worms.

> struct rational {
> 	u64 numer;
> 	u64 denom;
> };
> 
> If pwm-ir-tx would like to set the carrier, it could it like so:
> 
> 	struct rational period = {
> 		.numer = NUSEC_PER_SEC,
> 		.denom = carrier,
> 	};
> 
> 	pwm_set_period(&period);
> 
> Now pwm-bcm2835 could do it like so:
> 
> 	int bcm2835_set_period(struct rational *period)
> 	{
> 		struct rational rate = {
> 			.numer = NUSEC_PER_SEC,
> 			.denum = clk_get_rate(clk),
> 		};
> 
> 		rational_div(&rate, period);
> 
> 		int step = rational_to_u64(&rate);
> 	}
> 
> Alternatively, since most of the pwm hardware is doing scaling based on the
> clock (I think), would not make more sense for the pwm driver interface to
> take a frequency rather than a period? Then the integer calculations can be
> simpler: just divide the clock rate by the required frequency and you have
> the period.

I think the rounding approach is easier and gives you the optimal
setting, too. With a carrier of say 455 KHz you want a period of
1e9 / (455 kHz) = 2197.802197802198 ns. Now assuming a pwm clk of 10 MHz
the pwm-bcm2835 driver can offer you 2100 ns and 2200 ns and the
pwm-ir-tx driver can pick the one it prefers. (OK, this works so nicely
because the pwm-bcm2835 driver has a nice clk rate, but also if it were
say 66666000 Hz, it would offer you 2191 ns and 2206 ns and you could
pick your favourite. (In this case there is a rounding error because the
actual periods are 2190.021900219002 ns and 2205.022050220502 ns, but I
would expect that this doesn't influence the choice and to improve here
we probably would have to change the unit of clk_get_rate, too.)

Best regards
Uwe

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

Attachment: signature.asc
Description: PGP signature