Re: [PATCH v7 11/11] pwm: Add Raspberry Pi Firmware based PWM bus

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 2102 bytes --]

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:
> > On Mon, Jan 18, 2021 at 01:32:44PM +0100, Nicolas Saenz Julienne wrote:
> 
> [...]
> 
> > > +	/*
> > > +	 * 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.
> 
> > 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/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]