On Mon, May 15, 2023 at 1:13 PM Charles Keepax ckeepax@opensource.cirrus.com wrote:
On Fri, May 12, 2023 at 10:19:14PM +0300, andy.shevchenko@gmail.com 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.