Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Mon, 23 Oct 2023 14:34:17 +0100
From:   Daniel Thompson <daniel.thompson@linaro.org>
To:     Sean Young <sean@mess.org>
Cc:     Hans de Goede <hdegoede@redhat.com>,
        Uwe =?iso-8859-1?Q?Kleine-K=F6nig?= 
        <u.kleine-koenig@pengutronix.de>, linux-media@vger.kernel.org,
        linux-pwm@vger.kernel.org,
        Ivaylo Dimitrov <ivo.g.dimitrov.75@gmail.com>,
        Thierry Reding <thierry.reding@gmail.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Jani Nikula <jani.nikula@linux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@linux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>,
        Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        Javier Martinez Canillas <javierm@redhat.com>,
        Jean Delvare <jdelvare@suse.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Support Opensource <support.opensource@diasemi.com>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        Mark Gross <markgross@kernel.org>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Jingoo Han <jingoohan1@gmail.com>,
        Helge Deller <deller@gmx.de>, linux-doc@vger.kernel.org,
        linux-kernel@vger.kernel.org, intel-gfx@lists.freedesktop.org,
        dri-devel@lists.freedesktop.org, linux-hwmon@vger.kernel.org,
        linux-input@vger.kernel.org, linux-leds@vger.kernel.org,
        platform-driver-x86@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-fbdev@vger.kernel.org
Subject: Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in
 atomic context
Message-ID: <20231023133417.GE49511@aspen.lan>
References: <cover.1697534024.git.sean@mess.org>
 <a7fcd19938d5422abc59c968ff7b3d5c275577ed.1697534024.git.sean@mess.org>
 <90728c06-4c6c-b3d2-4723-c24711be2fa5@redhat.com>
 <20231019105118.64gdzzixwqrztjir@pengutronix.de>
 <01a505ac-320f-3819-a58d-2b82c1bf2a86@redhat.com>
 <ZTT9fvEF+lqfzGJ/@gofer.mess.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <ZTT9fvEF+lqfzGJ/@gofer.mess.org>
Precedence: bulk

On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote:
> Hi Hans,
>
> On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> > On 10/19/23 12:51, Uwe Kleine-K?nig wrote:
> > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> > >> On 10/17/23 11:17, Sean Young wrote:
> > > I think it's very subjective if you consider this
> > > churn or not.
> >
> > I consider it churn because I don't think adding a postfix
> > for what is the default/expected behavior is a good idea
> > (with GPIOs not sleeping is the expected behavior).
> >
> > I agree that this is very subjective and very much goes
> > into the territory of bikeshedding. So please consider
> > the above my 2 cents on this and lets leave it at that.
>
> You have a valid point. Let's focus on having descriptive function names.

For a couple of days I've been trying to resist the bikeshedding (esp.
given the changes to backlight are tiny) so I'll try to keep it as
brief as I can:

1. I dislike the do_it() and do_it_cansleep() pairing. It is
   difficult to detect when a client driver calls do_it() by mistake.
   In fact a latent bug of this nature can only be detected by runtime
   testing with the small number of PWMs that do not support
   configuration from an atomic context.

   In contrast do_it() and do_it_atomic()[*] means that although
   incorrectly calling do_it() from an atomic context can be pretty
   catastrophic it is also trivially detected (with any PWM driver)
   simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.

   No objections (beyond churn) to fully spelt out pairings such as
   do_it_cansleep() and do_it_atomic()[*]!


2. If there is an API rename can we make sure the patch contains no
   other changes (e.g. don't introduce any new API in the same patch).
   Seperating renames makes the patches easier to review!
   It makes each one smaller and easier to review!


Daniel.


[*] or do_it_nosleep()... etc.