Re: [PATCH 1/8 v3] pinctrl: add a generic pin config interface

[Linus Walleij]

On Tue, Nov 22, 2011 at 11:52 AM, Thomas Abraham
<thomas.abraham@xxxxxxxxxx> wrote:
> On 22 November 2011 01:17, Linus Walleij <linus.walleij@xxxxxxxxxxxxxx> wrote:

>>(...)
>> @@ -61,6 +67,10 @@ struct pin_desc {
>>  #ifdef CONFIG_PINMUX
>>        const char *mux_function;
>>  #endif
>> +#ifdef CONFIG_PINCONF
>> +       struct mutex config_lock;
>> +       struct pin_config config;
>
> Some platforms (like exynos) can read back the current pin_config from
> the hardware registers. For such platforms, 'config' would consume
> additional space which will not be used. Can 'config' be made a
> pointer and memory for it allocated in 'pinctrl_register_one_pin' only
> if the pin-controller requires it. Maybe, 'struct pinctrl_desc' can
> have a additional member to describe some of its characteristics such
> as ability to read back pin configuration from registers.

Yes. And I also do realize that most controllers at hand does not have
this limitation. I had second thoughts about it yesterday, so I'll
refactor this to assume that we can read back values from the
hardware instead, if a hardware has the mentioned limitation it
can do the caching in the driver instead. I can't quite remember
why I did it this way anyway, stupid me :-(

> EXPORT_SYMBOL(pin_config); here ?
> EXPORT_SYMBOL(pin_config_group); here ?

Yep added them.

>> +enum pin_config_param {
>> +       PIN_CONFIG_BIAS_DISABLE,
>> +       PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
>> +       PIN_CONFIG_BIAS_PULL_UP,
>> +       PIN_CONFIG_BIAS_PULL_DOWN,
>> +       PIN_CONFIG_BIAS_HIGH,
>> +       PIN_CONFIG_BIAS_GROUND,
>> +       PIN_CONFIG_DRIVE_PUSH_PULL,
>> +       PIN_CONFIG_DRIVE_OPEN_DRAIN,
>> +       PIN_CONFIG_DRIVE_OPEN_SOURCE,
>> +       PIN_CONFIG_DRIVE_OFF,
>> +       PIN_CONFIG_INPUT_SCHMITT,
>> +       PIN_CONFIG_INPUT_DEBOUNCE,
>> +       PIN_CONFIG_SLEW_RATE_RISING,
>> +       PIN_CONFIG_SLEW_RATE_FALLING,
>> +       PIN_CONFIG_POWER_SOURCE,
>> +       PIN_CONFIG_LOW_POWER_MODE,
>> +       PIN_CONFIG_WAKEUP,
>> +       PIN_CONFIG_END,
>> +};
>
> For all Samsung SoC's, PIN_CONFIG_BIAS_PULL_UP,
> PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_DRIVE_PUSH_PULL would be
> sufficient. So the above list of config options is fine for all
> Samsung platforms.

Thanks! That's what I needed to know.

>> +extern int pin_config(struct pinctrl_dev *pctldev, int pin,
>> +                     const struct pin_config_tuple *configs,
>> +                     unsigned num_configs);
>
>
> Is the 'pin' number expected to be local to the pin-controller
> represented by 'pctldev' ?

Yes, the entire subsystem is designed like that.

>  There can be multiple pin-controllers in a
> system, so it would be easier if 'pin' parameter above is a
> system-wide pin number, and the caller of 'pin_config' need not know
> which pin-controller manages 'pin'. Ideally, 'pctldev' should not be
> included in the parameter list.

I did that in the initial pinctrl patch set and Grant NACK:ed it
and requested that the pin controller pin space be local to each
pin controller.

The reason is that the global GPIO pin space is nothing but one
big mess, it's arbitrarily roofed at ARCH_NR_GPIOS and what
is that number in a single zImage for multiple platforms anyway?

Global GPIO 15 in a multi-platform binary would mean totally
different things depending on where it boots and strange numbers
have to be encoded everywhere.

Actually the plan is to kill the global number space from GPIO
too, one day. So the GPIO maintainer would like GPIO to be
like pinctrl not the other way around...

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/