On Wed, May 17, 2023 at 1:13 PM 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 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.
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 == 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-tables/sam... (mostly under edison/ folder)