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

From: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: f.fainelli@gmail.com, linux-kernel@vger.kernel.org,
	linux-pwm@vger.kernel.org, bcm-kernel-feedback-list@broadcom.com,
	linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
	wahrenst@gmx.net, linux-input@vger.kernel.org,
	dmitry.torokhov@gmail.com, gregkh@linuxfoundation.org,
	devel@driverdev.osuosl.org, p.zabel@pengutronix.de,
	linux-gpio@vger.kernel.org, linus.walleij@linaro.org,
	linux-clk@vger.kernel.org, sboyd@kernel.org,
	linux-rpi-kernel@lists.infradead.org, bgolaszewski@baylibre.com,
	andy.shevchenko@gmail.com
Subject: Re: [PATCH v7 11/11] pwm: Add Raspberry Pi Firmware based PWM bus
Date: Thu, 11 Mar 2021 14:01:00 +0100	[thread overview]
Message-ID: <fc60ac5ab9760d791aa5e184258accf53e07ce1e.camel@suse.de> (raw)
In-Reply-To: <20210310115041.s7tzvgdpksws6yss@pengutronix.de>

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

Hi Uwe,
thanks for taking the time to look into this. :)

On Wed, 2021-03-10 at 12:50 +0100, Uwe Kleine-König wrote:
> Hello Nicolas,
> 
> 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.
So as for them to be able to use it as is and avoid duplication. Now, if this
is blocking the driver from being merged, I'd rather remove it. It'll be a
patch for the downstream kernel to maintain, but better than nothing.

> And if we agree it's a good idea: Should raspberrypi_pwm_apply return 0 if
> setting the duty cycle succeeded and only setting the default didn't?

Good point. I don't think so. We'd be also missing on the following by
returning early:

	rpipwm->duty_cycle = duty_cycle;

I propose to change it to a 'best effort' approach, if it fails, log it and
continue successfully.

Regards,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]