Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230512122838.243002-1-ckeepax@opensource.cirrus.com>
 <20230512122838.243002-9-ckeepax@opensource.cirrus.com> <ZF6RMqElYZVMpWRt@surfacebook>
 <20230515101350.GS68926@ediswmail.ad.cirrus.com> <CAHp75Vcizrucc-2KFdFNeHNrxCzz4GwX1OzZYyjPH7P9RgnKYQ@mail.gmail.com>
 <20230517101301.GV68926@ediswmail.ad.cirrus.com>
In-Reply-To: <20230517101301.GV68926@ediswmail.ad.cirrus.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 17 May 2023 16:59:50 +0300
Message-ID: <CAHp75VchpbiYcd2yaP1WTjX17P0hg3qON5JGAXu08aDVw6Ydkw@mail.gmail.com>
Subject: Re: [PATCH 08/10] pinctrl: cs42l43: Add support for the cs42l43
To:     Charles Keepax <ckeepax@opensource.cirrus.com>
Cc:     broonie@kernel.org, lee@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
        tglx@linutronix.de, maz@kernel.org, linus.walleij@linaro.org,
        vkoul@kernel.org, lgirdwood@gmail.com,
        yung-chuan.liao@linux.intel.com, sanyog.r.kale@intel.com,
        pierre-louis.bossart@linux.intel.com, alsa-devel@alsa-project.org,
        patches@opensource.cirrus.com, devicetree@vger.kernel.org,
        linux-gpio@vger.kernel.org, linux-spi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, May 17, 2023 at 1:13=E2=80=AFPM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Tue, May 16, 2023 at 10:03:45PM +0300, Andy Shevchenko wrote:
> > On Mon, May 15, 2023 at 1:13=E2=80=AFPM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > On Fri, May 12, 2023 at 10:19:14PM +0300, andy.shevchenko@gmail.com w=
rote:
> > > > Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:
> > > > > +   if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-r=
anges")) {
> > > > > +           ret =3D gpiochip_add_pin_range(&priv->gpio_chip, priv=
->gpio_chip.label,
> > > > > +                                        0, 0, CS42L43_NUM_GPIOS)=
;
> > > > > +           if (ret) {
> > > > > +                   dev_err(priv->dev, "Failed to add GPIO pin ra=
nge: %d\n", ret);
> > > > > +                   goto err_pm;
> > > > > +           }
> > > > > +   }
> > > >
> > > > Besides the fact that we have a callback for this, why GPIO library=
 can't
> > > > handle this for you already?
> > >
> > > Apologies but I am not quite sure I follow you, in the device
> > > tree case this will be handled by the GPIO library. But for ACPI
> > > this information does not exist so has to be called manually, the
> > > library does not necessarily know which values to call with,
> > > although admittedly our case is trivial but not all are.
> >
> > Why can't the firmware provide this information? _DSD() is a part of
> > ACPI v5.1 IIRC.
>
> I am very very far from confident we can guarantee that will be
> present in the ACPI. The ACPI is typically made for and by the
> Windows side.

Why? You may insist firmware vendors / OEMs to use that as a
requirement to the platforms that would like to use your chip. The
_DSD() is part of the specification, I don't see how the above can be
an argument.

The times when ACPI =3D=3D Windows are quite behind.

> > Although it might require moving some code from gpiolib-of.c to
> > gpiolib.c with replacing OF APIs with agnostic ones.
>
> I really think if we want to start doing things that way on ACPI
> platforms someone with a little more clout than us needs to start
> doing it first. If Intel or someone was doing it that way it
> might give us a little more levelage to push it as being the
> "correct" way to do it.

So, we have the meta-acpi [1] project which contains dozens of
examples on how ACPI DSD is being used for real devices, besides some
documentation in the Linux kernel.

> I will switch to the callback, but really don't think we can rely
> on this being in DSD yet.

Why not?

...

> > > I had missed there are now devm_pm_runtime calls,

Btw, even if there is no such API one can always call
devm_add_action() / devm_add_action_or_reset() to open code such a
call.

> > > I will switch
> > > to that. But I would like to understand the wrong order, remove
> > > will be called before the devm bits are destroyed and it seems
> > > reasonable to disable the pm_runtime before destroying the
> > > pinctrl device. What exactly would run in the wrong order here?
> >
> > At the ->remove() stage after this call an IRQ can be fired (or on SMP
> > systems any other APIs can be called), for example. So, would it be a
> > problem to service it with PM disabled?
> >
> > But in any case the shuffling ordering like this is prone to subtle
> > bugs. I prefer to have strict ordering if there is nothing preventing
> > from doing that way.
>
> Yeah happy enough to use devm_ here, just didn't know it existed
> and wanted to better understand your concerns as I was having
> difficulty seeing the issue.

Ah, you are welcome!

...

[1]: https://github.com/westeri/meta-acpi/tree/master/recipes-bsp/acpi-tabl=
es/samples
(mostly under edison/ folder)

--=20
With Best Regards,
Andy Shevchenko