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

[Niyas Sait <niyas.sait@linaro.org>]

 >> +++ 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.

I started with firmware-guide but then moved to driver-api as I wanted 
to cover driver related bits as well. Let me know if it is better at 
firmware-guide.

>> +			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.
> 

I can try but I will have to repeat the same pin number few times to 
describe the pin function and config for different devices to 
demonstrate the pin muxing part.

>> +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)

Hm. Please correct me if I am wrong here. My understanding is that we 
need to do few mapping which only pin controller drivers can do such as 
ACPI function number to internal functional name or selector. I could 
define bindings to do those specific mappings rather than providing the 
current general mapping interface. Would that be better ?

>> +	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?
> 

I hope so. Do you see something odd ?