Hello Nicolas,

On Thu, Mar 11, 2021 at 02:01:00PM +0100, Nicolas Saenz Julienne wrote:

On Wed, 2021-03-10 at 12:50 +0100, Uwe Kleine-König wrote:

quoted

On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:

[...]

quoted

quoted

+	/*
+	 * This sets the default duty cycle after resetting the board, we
+	 * updated it every time to mimic Raspberry Pi's downstream's driver
+	 * behaviour.
+	 */
+	ret = raspberrypi_pwm_set_property(rpipwm->firmware, RPI_PWM_DEF_DUTY_REG,
+					   duty_cycle);
+	if (ret) {
+		dev_err(chip->dev, "Failed to set default duty cycle: %pe\n",
+			ERR_PTR(ret));
+		return ret;

This only has an effect for the next reboot, right?

It effects all reboots until it's further changed.

quoted

If so I wonder if it is a good idea in general. (Think: The current PWM
setting enables a motor that makes a self-driving car move at 100 km/h.
Consider the rpi crashes, do I want to car to pick up driving 100 km/h at
power up even before Linux is up again?)

I get your point. But this isn't used as a general purpose PWM. For now the
interface is solely there to drive a PWM fan that's arguably harmless. This
doesn't mean that the RPi foundation will not reuse the firmware interface for
other means in the future. In such case we can always use a new DT compatible
and bypass this feature (the current DT string is
'raspberrypi,firmware-poe-pwm', which is specific to this use-case).

My aim here is to be on par feature wise with RPi's downstream implementation.

Just because the downstream kernel does it should not be the (single)
reason to do that. My gut feeling is: For a motor restoring the PWM
config on reboot is bad and for a fan it doesn't really hurt if it
doesn't restart automatically. So I'd prefer to to drop this feature.

Best regards
Uwe

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