Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201118174417.278011-1-clemens.gruber@pqgruber.com>
 <CAGngYiV+oDeagaCfpeACMzQyDHVzk9ERbSBjW_fW5hoQANHqog@mail.gmail.com>
 <20201119100005.GA703@workstation.tuxnet> <CAGngYiU7+X1AbadQ0kFBQOqxK-adowg6CTOMx260fyF1-rpO-Q@mail.gmail.com>
 <20201119160013.GA217674@workstation.tuxnet>
In-Reply-To: <20201119160013.GA217674@workstation.tuxnet>
From:   Sven Van Asbroeck <thesven73@gmail.com>
Date:   Thu, 19 Nov 2020 12:29:26 -0500
Message-ID: <CAGngYiUELShMgFnvq6XzF0v=2UAwj7gJsPmbdGkmyAbzhM8OLA@mail.gmail.com>
Subject: Re: [PATCH 1/3] pwm: pca9685: Switch to atomic API
To:     Clemens Gruber <clemens.gruber@pqgruber.com>
Cc:     linux-pwm@vger.kernel.org,
        Thierry Reding <thierry.reding@gmail.com>,
        =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>, Lee Jones <lee.jones@linaro.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        David Jander <david@protonic.nl>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Nov 19, 2020 at 11:00 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> One thing I noticed: The driver currently assumes that it comes out of
> POR in "active" state (comment at end of probe and PM calls).
> However, the SLEEP bit is set by default / after POR.

Very good point, the comment is probably not correct.

It would be wrong to assume that the chip is in any particular
state when probe() runs. There is no reset pin, so the CPU running
Linux could have reset while manipulating the chip, there could even
be leftover state from a bootloader talking to the chip, etc...

I remember running into this myself at the time.

However, we want to make sure that the runtime pm puts the chip to sleep,
no matter its initial state. So the current code is correct, but the
comment can be changed to:

/*
 * Chip activity state unknown. Tell the runtime pm that the chip is
 * active, so pm_runtime_enable() will force it into suspend.
 * Which is what we want as all outputs are disabled at this point.
 */

> 2) If !CONFIG_PM: Clear the SLEEP bit in .probe

Is anyone likely to use this driver without CONFIG_PM? My kernel won't even
build without it...

If you personally have no use for it, then I would not bother with the
!CONFIG_PM case. Just formalize in Kconfig that the driver needs PM.

I think we can add "depends on PM" or "select PM", I am not sure which one
to use here.

Sven