Hi Michael,

On Fri, Aug 07, 2020 at 09:55:19AM +0200, Michael Walle wrote:

Am 2020-08-07 09:45, schrieb Uwe Kleine-König:

quoted

On Fri, Aug 07, 2020 at 09:28:31AM +0200, Michael Walle wrote:

quoted

Am 2020-08-06 10:40, schrieb Uwe Kleine-König:

quoted

On Mon, Aug 03, 2020 at 11:35:52AM +0200, Michael Walle wrote:

quoted

+static void sl28cpld_pwm_get_state(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   struct pwm_state *state)
+{
+	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
+	unsigned int reg;
+	int prescaler;
+
+	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CTRL, &reg);
+
+	state->enabled = reg & SL28CPLD_PWM_CTRL_ENABLE;
+
+	prescaler = FIELD_GET(SL28CPLD_PWM_CTRL_PRESCALER_MASK, reg);
+	state->period = SL28CPLD_PWM_PERIOD(prescaler);
+
+	sl28cpld_pwm_read(priv, SL28CPLD_PWM_CYCLE, &reg);
+	state->duty_cycle = SL28CPLD_PWM_TO_DUTY_CYCLE(reg);

Should reg be masked to SL28CPLD_PWM_CYCLE_MAX, or is it guaranteed that
the upper bits are zero?

Mh, the hardware guarantees that bit7 is zero. So masking with
SL28CPLD_PWM_CYCLE_MAX won't buy us much. But what I could think
could go wrong is this: someone set the prescaler to != 0 and the
duty cycle to a value greater than the max value for this particular
prescaler mode. For the above calculations this would result in a
duty_cycle greater than the period, if I'm not mistaken.

The behavior of the hardware is undefined in that case (at the moment
it will be always on, I guess). So this isn't a valid setting.
Nevertheless it might happen. So what about the following:

state->duty_cycle = min(state->duty_cycle, state->period);

If you care about this: This can also happen (at least shortly) in
sl28cpld_pwm_apply() as you write SL28CPLD_PWM_CTRL before
SL28CPLD_PWM_CYCLE there.

It could also happen if it was the other way around, couldn't it?
Changing modes might glitch.

If you want to prevent this, you have to order the writes depending on
prescaler increasing or decreasing.

Best regards
Uwe

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