Re: [PATCH v6] pinctrl: add a pin config interface

[Thomas Abraham <thomas.abraham@linaro.org>]

Hi Linus,

On 19 January 2012 23:58, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jan 19, 2012 at 6:55 PM, Thomas Abraham
> <thomas.abraham@linaro.org> wrote:
>> Hi Linus,
>> On 19 January 2012 22:28, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Wed, Jan 18, 2012 at 8:16 AM, Thomas Abraham
>>> <thomas.abraham@linaro.org> wrote:
>>>
>>>> In case of runtime pinmuxing, the pin configuration would also be
>>>> required to be setup in some cases. pin_config_set() is suitable to be
>>>> called from the platform code. In case of runtime pinmuxing in driver
>>>> code, is there any way to set the pin config also at runtime in driver
>>>> code?
>>>
>>> Yes that is already possible today with the pin_config_set() and
>>> pin_config_group_set() calls already merged for 3.3.
>>>
>>> However there is no relation between the struct device and these
>>> config settings so I feel that this is a bit hack-ish, but it was
>>> atleast something we could agree upon.
>>
>> Using pin_config_set() from drivers did not seem correct. The concern
>> here is that all there parameters of pin_config_set() are specific to
>> a particular platform. Hence, using it in driver means that the driver
>> will no more be usable across multiple different platforms.
>
> Yes. But drivers can define a callback function into their
> platform data, which in turn has the knowledge of what to do
> for different scenarios, and calls pin_config_set() on the
> pin controller for that system.
>
> But no, it's not elegant :-)

Ok. platform_data is a possibility. So for non-dt driver probe, the
driver would call the platform callback and for dt based driver probe,
we still need to find a suitable way.

>
>> But that was not the case with pinmux_get() and pinmux_put().
>
> True. pinmuxes are completely platform agnostic...
>
> What would be elegant?
>
> pinconf_get() + pinconf_put() doesn't seem right since
> configs are not simple "on or off" states.
>
> So we need something like the named config states, but
> mapped down per device.
>
> How about a driver does this:
>
> ret = pinconf_activate(&dev, "foo");
>
> Where "foo" is a state string like "active", "idle", "sleep",
> "off" and could correspons to preset strings in pinconf.h
> like those found in the other patch

Yes, the ability to set the pinconfig from the driver would be very
helpful for Samsung platforms.


> "[PATCH] pinctrl: pin configuration states" I sent out
> two days ago?
>
> Then we can have both system-wide and per-device
> mapped pin configuration states named by strings.

Ok. Support for both would be nice to have.

>
> Note that pinconf_get()/pinconf_put() is completely
> superfluous in this scheme. Since the configurations are
> stateless settings (not e.g. allocating pins) this is all fine.
>
> Passing the string with a named config indicates that
> the driver knows exactly what it is doing anyway.
>
> Shall I recook pin configuration states like this for
> a try?

Yes, that will be very helpful. Thanks.

It would be easier if pinconf_register_pin_states() need not be called
for each pin state instance. There should be something similar to the
existing pinmux configuration where is no need to explicitly register
a pinmux.

Regards,
Thomas.

>
>> Will
>> there be support added in pinctrl subsystem to allow drivers to
>> configure pin-config settings from drivers and be compatible for
>> multiple platforms?
>
> Please provide feedback on my patch entitled:
> "[PATCH 2/2 v5] pinctrl: introduce generic pin config"
>
> If I get a few ACKs for something like that we have the
> infrastructure for letting drivers config their pins.
>
> Yours,
> Linus Walleij