Hello,
On Fri, Dec 10, 2021 at 06:13:34PM +0500, Nikita Travkin wrote:
> Uwe Kleine-König писал(а) 10.12.2021 03:05:
> > Hello,
> >
> > On Thu, Dec 09, 2021 at 09:20:20PM +0500, Nikita Travkin wrote:
> >> +#define to_pwm_clk_chip(_chip) container_of(_chip, struct pwm_clk_chip, chip)
> >> +
> >> +static int pwm_clk_apply(struct pwm_chip *pwm_chip, struct pwm_device *pwm,
> >> + const struct pwm_state *state)
> >> +{
> >> + struct pwm_clk_chip *chip = to_pwm_clk_chip(pwm_chip);
> >> + int ret;
> >> + u32 rate;
> >> +
> >> + if (!state->enabled && !pwm->state.enabled)
> >> + return 0;
> >> +
> >> + if (state->enabled && !pwm->state.enabled) {
> >> + ret = clk_enable(chip->clk);
> >> + if (ret)
> >> + return ret;
> >> + }
> >> +
> >> + if (!state->enabled && pwm->state.enabled) {
> >> + clk_disable(chip->clk);
> >> + return 0;
> >> + }
> >
> > This can be written a bit more compact as:
> >
> > if (!state->enabled) {
> > if (pwm->state.enabled)
> > clk_disable(chip->clk);
> > return 0;
> > } else if (!pwm->state.enabled) {
> > ret = clk_enable(chip->clk);
> > if (ret)
> > return ret;
> > }
> >
> > personally I find my version also easier to read, but that might be
> > subjective.
>
> Having three discrete checks for three possible outcomes is a bit
> easier for me to understand, but I have no preference and can change
> it to your version.
>
> > Missing handling for polarity. Either refuse inverted polarity, or set
> > the duty_cycle to state->period - state->duty_cycle in the inverted
> > case.
>
> Will add the latter.
>
> >
> >> + rate = div64_u64(NSEC_PER_SEC, state->period);
> >
> > Please round up here, as .apply() should never implement a period bigger
> > than requested. This also automatically improves the behaviour if
> > state->period > NSEC_PER_SEC.
>
> Will do. I'm not sure if the underlying clock drivers guarantee the
> chosen rate to be rounded in line with that however, e.g. qcom SoC
This is a problem that most drivers have. Very few use clk_set_rate, but
also clk_get_rate is subject to (much smaller) rounding issues.
There doesn't seem to be an agreement that this is important enough to
address.
> clocks that I target use lookup tables to find the closest rate
> with known M/N config values and set that. (So unless one makes sure
> the table has all the required rates, period is not guaranteed.)
>
> This is not an issue for my use cases though: Don't think any
> of the led or haptic motor controllers I've seen in my devices
> need a perfect rate.
>
> I think this is another line into the "Limitations" description
> that was suggested later.
>
> >
> >> + ret = clk_set_rate(chip->clk, rate);
> >> + if (ret)
> >> + return ret;
> >> +
> >> + return clk_set_duty_cycle(chip->clk, state->duty_cycle, state->period);
> >
> > Is it possible to enable only after the duty cycle is set? This way we
> > could prevent in some cases that a wrong setting makes it to the output.
> >
>
> Will move clk_enable() as the last action. After that it makes more
> sense to "squash" two leftover checks for disabled state so will do
> that as well.
>
> ... Except moving it caused a nice big WARNING from my clock controller...
>
> [ 76.353557] gcc_camss_gp1_clk status stuck at 'off'
> [ 76.353593] WARNING: CPU: 2 PID: 97 at drivers/clk/qcom/clk-branch.c:91 clk_branch_wait+0x144/0x160
> (...)
> [ 76.571531] Call trace:
> [ 76.578644] clk_branch_wait+0x144/0x160
> [ 76.580903] clk_branch2_enable+0x34/0x44
> [ 76.585069] clk_core_enable+0x6c/0xc0
> [ 76.588974] clk_enable+0x30/0x50
> [ 76.592620] pwm_clk_apply+0xb0/0xe4
> [ 76.596007] pwm_apply_state+0x6c/0x1ec
> [ 76.599651] sgm3140_brightness_set+0xb4/0x190 [leds_sgm3140]
>
> (Which doesn't stop it from working afaict, but very much undesirable
> for me.)
> Unsure if this is something common or just a quirk of this specific
> driver but I'd rather take a little glitch on the output than
> make clock driver unhappy knowing how picky this hardware sometimes is...
I think clk_set_rate for a disabled clk isn't well defined.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
