Re: [PATCH v3 1/2] pinctrl: add support for ACPI pin function and config resources

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Wed, Nov 30, 2022 at 04:40:26PM +0000, Niyas Sait wrote:
> Add support for following ACPI pin resources
> 
> - PinFunction
> - PinConfig
> 
> Pinctrl-acpi parses the ACPI table and generates list of pin
> descriptors that can be used by pin controller to set and config pin.
> 
> See Documentation/driver-acpi/pin-control-acpi.rst for details

Thank you for an update, my comments below.

...

> +++ b/Documentation/driver-api/pin-control-acpi.rst

We have Documentation/firmware-guide/acpi/, but I'm not sure
which one suits better for this.

...

> +bias to be set to pull up with pull strength of 10K Ohms and for GPIO functionality

I believe the proper way to write units is this 10 kOhms. Same for the rest
similar cases.

...

> +OSPM will have to handle the above resources and select the pin function and configuration
> +through vendor specific interfaces (e.g: memory mapped registers) for the devices to be
> +fully functional.

If it's a copy'n'paste from the specification, we don't need that here.

...

> +			Name (RBUF, ResourceTemplate()
> +			{
> +				Memory32Fixed(ReadWrite, 0x4F800000, 0x20)
> +				Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) {0x55}
> +				PinFunction(Exclusive, PullDefault, 0x5, "\\_SB.GPI0", 0, ResourceConsumer, ) {2, 3}
> +				// Configure 10k Pull up for I2C SDA/SCL pins
> +				PinConfig(Exclusive, 0x01, 10000, "\\_SB.GPI0", 0, ResourceConsumer, ) {2, 3}
> +			})
> +			Return(RBUF)

In all examples the 2, 3 is used as a pin list for all kind of resources,
it's so confusing. Also take into account the difference between GPIO and
pin number spaces as I told before. Examples should cover that.

Also try to compile all ASL with latest ACPICA tools and fix warnings / errors.

...

> +Pin control devices can add callbacks for following pinctrl_ops to handle ACPI
> +pin resources.

Why? What use case requires this?
ACPI specification is more stricter in this than DT if I understand correctly
the state of affairs.  So, can't we parse the tables in the same way for all?

...

> +		case PINCTRL_ACPI_PIN_FUNCTION:

> +		case PINCTRL_ACPI_PIN_CONFIG:

These are definitely what we do not want to see in the individual drivers.
(I understand that it might be that some OEMs will screw up and we would
 need quirks, but not now)

...

>  obj-$(CONFIG_OF)		+= devicetree.o
> +obj-$(CONFIG_ACPI)		+= pinctrl-acpi.o

So, it should be called acpi.c here in analogous with DT.

...

>  #include <linux/pinctrl/consumer.h>
>  #include <linux/pinctrl/pinctrl.h>
>  #include <linux/pinctrl/machine.h>
> +#include <linux/acpi.h>

Ordering?

...

>  #include "devicetree.h"
>  #include "pinmux.h"
>  #include "pinconf.h"

> -
> +#include "pinctrl-acpi.h"

Ditto.

...

> +	status = acpi_get_handle(NULL, pinctrl_acpi, &handle);
> +	if (ACPI_FAILURE(status))
> +		return NULL;
> +
> +	adev = acpi_get_acpi_dev(handle);
> +	if (!adev)
> +		return NULL;
> +
> +	dev_name = acpi_dev_name(adev);
> +	if (!dev_name)
> +		return NULL;
> +
> +	return get_pinctrl_dev_from_devname(dev_name);

Are they all resource leakage-free?

...

> +		ret = acpi_pin_config_to_pinctrl_map(

Indentation.

> +			p, ares->resource_source.string_ptr, ares->pin_table[i],
> +			&config_node.node, 1);
> +		if (ret < 0)
> +			return ret;

-- 
With Best Regards,
Andy Shevchenko