On Mon, Apr 29, 2019 at 02:31:02PM +0200, Uwe Kleine-König wrote: > On Mon, Apr 29, 2019 at 01:03:54PM +0200, Thierry Reding wrote: > > On Mon, Apr 08, 2019 at 04:39:14PM +0200, Uwe Kleine-König wrote: > > > On Mon, Apr 01, 2019 at 03:45:47PM -0700, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <heiko@xxxxxxxxx> wrote: > > > > > > > > > > Hi, > > > > > > > > > > [adding two chromeos people, because veyron and gru are quite > > > > > heavy users of the rockchip pwm for both backlight and regulators] > > > > > > > > > > Doug, Brian: patchwork patch is here: > > > > > https://patchwork.kernel.org/patch/10851001/ > > > > > > > > > > Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König: > > > > > > The pwm-rockchip driver is one of only two PWM drivers which updates the > > > > > > state for the caller of pwm_apply_state(). This might have surprising > > > > > > results if the caller reuses the values expecting them to still > > > > > > represent the same state. > > > > > > > > It may or may not be surprising, but it is well documented. Specifically: > > > > > > > > * pwm_apply_state() - atomically apply a new state to a PWM device > > > > * @pwm: PWM device > > > > * @state: new state to apply. This can be adjusted by the PWM driver > > > > * if the requested config is not achievable, for example, > > > > * ->duty_cycle and ->period might be approximated. > > > > > > > > I don't think your series updates that documentation, right? > > > > > > > > > > > > > > Also note that this feedback was incomplete as > > > > > > the matching struct pwm_device::state wasn't updated and so > > > > > > pwm_get_state still returned the originally requested state. > > > > > > > > The framework handles that. Take a look at pwm_apply_state()? It does: > > > > > > > > --- > > > > > > > > err = pwm->chip->ops->apply(pwm->chip, pwm, state); > > > > if (err) > > > > return err; > > > > > > > > pwm->state = *state; > > > > > > > > > > > --- > > > > > > > > So I think it wasn't incomplete unless I misunderstood? > > > > > > You're right, I missed that the updated state was saved. > > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> > > > > > > > > > > I've tested this on both veyron and gru with backlight and pwm regulator > > > > > and at least both still come up, so > > > > > Tested-by: Heiko Stuebner <heiko@xxxxxxxxx> > > > > > > > > > > But hopefully Doug or Brian could also provide another test-point. > > > > > > > > I'd definitely be concerned by this change. Specifically for the PWM > > > > regulator little details about exactly what duty cycle / period you > > > > got could be pretty important. > > > > > > > > I guess the problem here is that pwm_get_state() doesn't actually call > > > > into the PWM drivers, it just returns the last state that was applied. > > > > How does one get the state? I guess you could change get_state() to > > > > actually call into the PWM driver's get_state if it exists? ...but > > > > your patch set doesn't change that behavior... > > > > > > My intention here is more to make all drivers behave the same way and > > > because only two drivers updated the pwm_state this was the variant I > > > removed. > > > > > > When you say that the caller might actually care about the exact > > > parameters I fully agree. In this case however the consumer should be > > > able to know the result before actually applying it. So if you do > > > > > > pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...}) > > > > > > and this results in .period = 100 and .duty_cycle = 0 then probably the > > > bad things you want to know about already happend. So my idea is a new > > > function pwm_round_state() that does the adaptions to pwm_state without > > > applying it to the hardware. After that pwm_apply_state could do the > > > following: > > > > > > rstate = pwm_round_state(pwm, state) > > > pwm.apply(pwm, state) > > > gstate = pwm_get_state(pwm) > > > > > > if rstate != gstate: > > > warn about problems > > > > I'm not sure this is really useful. I would expect that in most cases > > where it is necessary to have an exact match between the requested state > > and the actual state is important, you may not even get to warning about > > problems because the system may shut down (e.g. the regulator might not > > be outputting enough power to keep the system stable). > > > > I think it'd be far more useful to give consumers a way to request that > > the state be applied strictly. I'm not sure how realistic that is, > > though. Most PWMs have some sort of restrictions, and in most cases this > > might still be okay. Perhaps some sort of permissible relative deviation > > factor could be added to give more flexibility? > > I think in practise this isn't going to work. Consider the case that > Brian cares about: "we do need to be as precise as possible with the > duty:period ratio". So if we want 1/5 duty we might request: > > .duty_cycle = 100, .period = 500 > > an are using pwm_set_state_exact(). Now consider this fails. What is the > next value you should try? The idea is that if the driver fails to set the exact state if that's what was requested, then we just fail. If we really need an exact set of values, then it doesn't make sense to offer the user alternatives using rounding helpers. On the other hand, if we introduce an error margin, we could make the above work. Let's say the PWM regulator requires accuracy within 1%, we could do something like this: state.duty_cycle = 100; state.period = 500; state.accuracy = 1; /* there's a slew of ways to encode this */ pwm_apply_state(pwm, &state); That way, the PWM driver can determine whether or not the ratio of possible duty-cycle/period is accurate within that 1% requested by the user: ratio = duty-cycle / period requested = 100 / 500 possible = 99 / 498 possible / requested ~= 0.993 In other words, possible is > 99% of requested and therefore within the 1% accuracy that pwm-regulator requested. The PWM driver can therefore go ahead and program the selected set of values. > It's hard to say without knowing why it failed. If however you could do > > mystate.duty_cycle = 100 > mystate.period = 500 > pwm_round_state(pwm, &mystate); > > and after that we have: > > mystate.duty_cycle = 99; > mystate.period = 498; > > (because pwm_round_state is supposed to round down[1] and the hardware can > implement multiples of 3 only). Then it is easier to determine the next > state to try. No, it's really not any easier to determine the next state to try. The problem is that the consumer doesn't know anything about the restrictions of the driver, and it shouldn't need to know. Given the above, how is it supposed to know that the restriction is "multiple of 3". Just because the two values happen to be multiples of 3 doesn't mean that's the restriction. Also, we really don't want every consumer in the kernel to implement factorization (and whatnot) to guess what possible constraints there could be. Thierry > > Best regards > Uwe > > [1] this isn't fixed yet, but I think it's sensible. > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
_______________________________________________ Linux-rockchip mailing list Linux-rockchip@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-rockchip
