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

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>]

On Thu, Dec 01, 2022 at 09:27:07AM +0000, Niyas Sait wrote:
> >>>> +++ 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.
> >
> > My point is that I don't know. If it's more about ACPI tables and
> properties
> > there, it's related to firmware-guide, if it's about Linux kernel pin
> control
> > subsystem (programming, etc) it's better to have it under its own
> documentation
> > subfolder.
> 
> In that case, we probably should also move existing
> Documentation/driver-api/pin-control.rst to the new subfolder ?

Maybe, I don't know. All depends on the (final) contents of the documentation
you would like to provide.

...

> > > > > +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.
> > 
> > Not sure I understand the use case here. The PinFunction() selects the mode for
> > the pins in the list. But naming is hardware specific, indeed. And it seems
> > there is no name field for the PinFunction().
> > 
> > > I could define
> > > bindings to do those specific mappings rather than providing the current
> > > general mapping interface. Would that be better ?
> > 
> > But that mapping can be provided by the driver at the initialization stage or
> > generated automatically.
> > 
> > For the first we already have pin control APIs. For the second one I don't
> > understand why driver should be involved.
> > 
> Yes ACPI PinFunction() only contains a function number and it is hardware
> specific.
> 
> AFAIA,the only way that would work without extra mapping is if drivers could
> populate the pinctrl function tree with the index matching the function
> number from ACPI table. I wasn't sure if that would work in all cases. We
> can start with that if that would be good enough for now. Let me know if
> there are other existing APIs that could help.

That's why third (?) time I'm asking you to provide a design level
documentation of your approach to understand it better.

The pin control has provider-consumer idea behind. When a *consumer* device
is being probed, it tries to configure its pins based on the data collected
in the pin control device that *provides* the resources. At this point of
time the pin control subsystem parses tables (currently DT and board files)
for that. For the pin control device itself, the registration of it also
parses tables but for the *provider*.

Now, the pin control device driver (*provider*) may or may not have
(a subset of) the pin functions and pin groups hard coded in the driver.
This can be used by the pin control subsystem as well as data that comes
from the tables. The data from the tables, nevertheless, is not really
needs names of the functions, because it's not what is programmed into
HW registers. That's why when driver doesn't have that information, it's
fine to generate it.

Also note, that in ACPI all Pin*() descriptors are optional and we need
to cover all such cases, I already pointed out to my preliminary research,
which I have done 5 years ago, where I tried to understand how it should
be covered for the most used cases.

I think I stopped with my solution in particular due to the problems with
the power management interaction with pin control, i.e. string-based odd
states (4 of which are hard coded in the pin control core code).

-- 
With Best Regards,
Andy Shevchenko