[PATCH v2] pwm: bcm2835: Support apply function for atomic configuration

[Uwe Kleine-König]

Hello Sean,

On Mon, Dec 07, 2020 at 09:43:20AM +0000, Sean Young wrote:
> Thank you for taking the time to explain your thinking.

I'm happy you have an open ear for it. With this I really enjoy spending
the time to find the right arguments and examples.

> On Mon, Dec 07, 2020 at 09:16:28AM +0100, Uwe Kleine-König wrote:
> > On Sun, Dec 06, 2020 at 02:19:41PM +0000, Sean Young wrote:
> > > On Sat, Dec 05, 2020 at 08:25:10PM +0100, Uwe Kleine-König wrote:
> > > > On Sat, Dec 05, 2020 at 05:34:44PM +0000, Sean Young wrote:
> > > > > What real life uses-cases are there for round down? If you want to round
> > > > > down, is there any need for round up?
> > > > 
> > > > The scenario I have in mind is for driving a motor. I have to admit
> > > > however that usually the period doesn't matter much and it's the
> > > > duty_cycle that defines the motor's speed. So for this case the
> > > > conservative behaviour is round-down to not make the motor run faster
> > > > than expected.
> > > 
> > > I am reading here that for driving motors, only the duty cycle matters,
> > > not the period.
> > 
> > There is an upper limit (usually around 1 ms) for the period, but if you
> > choose 0.1 ms or 0.001 ms doesn't matter much AFAICT.
> > 
> > @Thierry: Do you have further use cases in mind?

I asked in the hardware department of the company I work for and they
had another usecase: Motors where for example a 1 ms pulse means "move
forwards" and 2 ms means "move backwards". They had the same idea as I
had: You want to know beforehand the result of a given
pwm_apply_state().

> > > > For other usecases (fan, backlight, LED) exactness typically doesn't
> > > > matter that much.
> > > 
> > > So, the use-cases you have are driving motor, fan, backlight, and led.
> > > And in all these cases the exact Hz does not matter.
> > > 
> > > The only uses case where the exact Hz does matter is pwm-ir-tx. 
> > > 
> > > So, I gather there are no use-cases for round-down. Yes, should round-down
> > > be needed, then this is more difficult to implement if the driver always
> > > does a round-closest. But, since there is no reason to have round-down,
> > > this is all academic.
> > > 
> > > Your policy of forcing new pwm drivers to use round-down is breaking
> > > pwm-ir-tx.
> > 
> > So you're indeed suggesting that the "right" rounding strategy for
> > lowlevel drivers should be:
> > 
> >  - Use the period length closest to the requested period (in doubt round
> >    down?)
> >  - With the chosen period length use the biggest duty_cycle not bigger
> >    than the requested duty_cycle.
> > 
> > While this seems technically fine I think for maintenance this is a
> > nightmare.
> > 
> > My preference would be to stick to the rounding strategy we used so far
> > (i.e.:
> > 
> >  - Use the biggest period length not bigger than the requested period
> >  - With the chosen period length use the biggest duty_cycle not bigger
> >    than the requested duty_cycle.
> > 
> > ) and for pwm-ir-tx add support to the PWM API to still make it possible
> > (and easy) to select the best setting.
> > 
> > The reasons why I think that this rounding-down strategy is the best
> > are (in order of importance):
> > 
> >  - It is easier to implement correctly [1]
> 
> Yes, you are right. You have given a great example where a simple
> DIV_ROUND_CLOSEST() does not give the result you want.
> 
> >  - Same rounding method for period and duty cycle
> >  - most drivers already do this (I think)
> > 
> > The (IMHO nice) result would then mean:
> > 
> >  - All consumers can get the setting they want; and
> 
> Once there is a nice pwm api for selecting round-nearest, then yes.
> 
> For the uses cases you've given, fan, backlight, and led a round-nearest
> is the rounding they would want, I would expect.

maybe, yes. Maybe it is also not important enough to spend the extra
cycles getting round nearest and so sticking to round-down is good
enough.

> >  - Code in lowlevel drivers is simple and the complexity is in common
> >    code and so a single place.
> > 
> > And it would also allow the pwm-ir-tx driver to notice if the PWM to be
> > used can for example only support frequencies under 400 kHz.
> 
> I doubt pwm-ir-tx cares about this, however it is a nice-to-have. It would
> also be nice if the rounding could be used with atomic configuration
> as well.

I cannot follow, you created 11fc4edc483bea8bf0efa0cc726886d2342f6fa6
because 476.2 Mhz was too bad. So you seem to be interested in
deviations and part of the problem is that you don't get feedback about
how your request is fulfilled.

> Please let me know when/if this new API exists for pwm so that pwm-ir-tx
> can select the right rounding.

Given that the bcm2835 driver is quite trivial I would be happy to
create a series that "fixes" the driver to round down and provide a
prototype for pwm_round_nearest for you to test on pwm-ir-tx. A willing
tester and a real use-case were the single two things that stopped me
investing time here.

> > [1] Consider a PWM with a parent frequency of 66 MHz, to select the
> >     period you can pick an integer divider "div" resulting in the period
> >     4096 / (pclk * d). So the obvious implementation for round-nearest
> >     would be:
> > 
> >     	pclk = clk_get_rate(myclk);
> > 	div = DIV_ROUND_CLOSEST(NSEC_PER_SEC * 4096, targetperiod * pclk);
> 
> Note NSEC_PER_SEC * 4096 >> 2^32 so this would need to be
> DIV_ROUND_CLOSEST_ULL.

Yeah, I ignored all these nasty little details like ranges of integers
and the valid range for div etc. for the sake of simplicity.
 
> >     , right?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- 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-arm-kernel/attachments/20201207/a2f509af/attachment.sig>