Hello,

On Fri, Dec 10, 2021 at 06:13:34PM +0500, Nikita Travkin wrote:

Uwe Kleine-König писал(а) 10.12.2021 03:05:

quoted

Hello,

On Thu, Dec 09, 2021 at 09:20:20PM +0500, Nikita Travkin wrote:

quoted

+#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. 

quoted

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.

quoted

quoted

+	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.

quoted

quoted

+	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/ |