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> <CAGngYiUELShMgFnvq6XzF0v=2UAwj7gJsPmbdGkmyAbzhM8OLA@mail.gmail.com>
 <X7krI+JkhCO5nYC+@workstation.tuxnet>
In-Reply-To: <X7krI+JkhCO5nYC+@workstation.tuxnet>
From:   Sven Van Asbroeck <thesven73@gmail.com>
Date:   Sat, 21 Nov 2020 17:00:10 -0500
Message-ID: <CAGngYiWKwDoeM+Hgj-ehJBRp16u2_-dDULzvVbGEUQ2ZOY9w4A@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 Sat, Nov 21, 2020 at 9:58 AM Clemens Gruber
<clemens.gruber@pqgruber.com> wrote:
>
> I'd rather continue supporting this driver with !CONFIG_PM. (In our
> company we have a product with a !CONFIG_PM build using this driver)

Absolutely, makes sense. If you do add support for !CONFIG_PM, then it's
important that both PM and !PM cases get tested by you.

>
> I am thinking about the following solution:
> #ifdef CONFIG_PM
>   /* Set runtime PM status according to chip sleep state */
>   if (reg & MODE1_SLEEP)
>     pm_runtime_set_suspended(..);
>   else
>     pm_runtime_set_active(..);
>
>   pm_runtime_enable(..);
> #else
>   /* If in SLEEP state on non-PM environments, wake the chip up */
>   if (reg & MODE1_SLEEP)
>     pca9685_set_sleep_mode(.., false)
> #endif

I don't think we need the #ifdef CONFIG_PM, because all pm_runtime_xxx
functions become no-ops when !CONFIG_PM.

Also, I believe "if (IS_ENABLED(CONFIG_XXX))" is preferred, because
it allows the compiler to syntax-check disabled code.

How about the following? It should be correct, short, and easy to understand.
Yes, there's one single unnecessary register write (+ 500us delay if !PM) when
the chip is already active on probe(). But maybe that's worth it if it makes
the code easier to understand?

probe()
{
    ...
    pm_runtime_set_active(&client->dev);
    pm_runtime_enable(&client->dev);

    if (!IS_ENABLED(CONFIG_PM))
        pca9685_set_sleep_mode(pca, false);

    return 0;
}

remove()
{
    ...
    pm_runtime_disable(&client->dev);

    if (!IS_ENABLED(CONFIG_PM))
        pca9685_set_sleep_mode(pca, true);

    return 0;
}

>
> About the regmap cache: I looked into it and think it is a good idea but
> it's probably best to get these patches merged first and then rework the
> driver to using the regmap cache?

Good suggestion, I agree.