Re: [PATCH 08/10] pinctrl: cs42l43: Add support for the cs42l43

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Mon, May 15, 2023 at 1:13 PM Charles Keepax
<ckeepax@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On Fri, May 12, 2023 at 10:19:14PM +0300, andy.shevchenko@xxxxxxxxx wrote:
> > Fri, May 12, 2023 at 01:28:36PM +0100, Charles Keepax kirjoitti:

...

> > > +   dev_dbg(priv->dev, "Setting gpio%d to %s\n",
> > > +           offset + 1, input ? "input" : "output");
> >
> > How ' + 1' part won't be confusing?
>
> Kinda an un-avoidable confusion somewhere, the GPIOs in the datasheet are
> numbered from one. So this makes the debug print match the
> datasheet name for the pin, which is used in the pinctrl strings
> as well.

The problem here is that the entire Linux pin control and GPIO cores
in their debug/info/error messages will use offset + 0. With the above
invention it will well make users confused a lot. I think you need a
Linus W blessing for this.

...

> > > +   if (!of_property_read_bool(dev_of_node(cs42l43->dev), "gpio-ranges")) {
> > > +           ret = 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 range: %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.

Although it might require moving some code from gpiolib-of.c to
gpiolib.c with replacing OF APIs with agnostic ones.

...

> > > +static int cs42l43_pin_remove(struct platform_device *pdev)
> > > +{
> > > +   pm_runtime_disable(&pdev->dev);
> >
> > This is simply wrong order because it's a mix of non-devm_*() followed by
> > devm_*() calls in the probe.
> >
>
> I had missed there are now devm_pm_runtime calls, 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.

-- 
With Best Regards,
Andy Shevchenko