On Tue, May 16, 2023 at 10:03:45PM +0300, Andy Shevchenko wrote:
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:
- 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.
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.
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.
I will switch to the callback, but really don't think we can rely on this being in DSD yet.
+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.
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.
Thanks, Charles