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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 11 Mar 2021 14:18:45 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Nicolas Saenz Julienne <nsaenzjulienne@...e.de>
Cc:     f.fainelli@...il.com, linux-kernel@...r.kernel.org,
        linux-pwm@...r.kernel.org, bcm-kernel-feedback-list@...adcom.com,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org,
        wahrenst@....net, linux-input@...r.kernel.org,
        dmitry.torokhov@...il.com, gregkh@...uxfoundation.org,
        devel@...verdev.osuosl.org, p.zabel@...gutronix.de,
        linux-gpio@...r.kernel.org, linus.walleij@...aro.org,
        linux-clk@...r.kernel.org, sboyd@...nel.org,
        linux-rpi-kernel@...ts.infradead.org, bgolaszewski@...libre.com,
        andy.shevchenko@...il.com
Subject: Re: [PATCH v7 11/11] pwm: Add Raspberry Pi Firmware based PWM bus

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)