[PATCH 2/2 v4] pinctrl: introduce generic pin config

[PATCH 2/2 v4] pinctrl: introduce generic pin config

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

This is a split-off from the earlier patch set which adds generic
pin configuration for the pin controllers that want it. Since
we may have a system with mixed generic and custom pin controllers,
we pass a boolean in the pin controller ops vtable to indicate
if it is generic.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/pinctrl.txt               |   11 +++
 drivers/pinctrl/Kconfig                 |    4 +
 drivers/pinctrl/Makefile                |    1 +
 drivers/pinctrl/pinconf-generic.c       |   89 +++++++++++++++++++
 drivers/pinctrl/pinconf.c               |    2 +
 drivers/pinctrl/pinconf.h               |   20 +++++
 include/linux/pinctrl/pinconf-generic.h |  142 +++++++++++++++++++++++++++++++
 include/linux/pinctrl/pinconf.h         |    5 +
 8 files changed, 274 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pinctrl/pinconf-generic.c
 create mode 100644 include/linux/pinctrl/pinconf-generic.h

diff --git a/Documentation/pinctrl.txt b/Documentation/pinctrl.txt
index 68ef515..720dfea 100644
--- a/Documentation/pinctrl.txt
+++ b/Documentation/pinctrl.txt
@@ -269,6 +269,17 @@ case each individual pin will be treated by separate pin_config_set() calls as
 well.
 
 
+Generic pin configuration
+=========================
+
+The pin control system supports an interface optionally abstracting the
+pin properties. It considers the things a controller may want configure as
+enumerable, and thus the parameters such as PIN_CONFIG_BIAS_PULL_UP are defined
+by the core, whereas the arguments to the parameter may need to be on a custom
+format only understandable by the driver. This may apply to some pin
+controllers, especially simple ones.
+
+
 Interaction with the GPIO subsystem
 ===================================
 
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index c63c721..2ba2746 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -17,6 +17,10 @@ config PINMUX
 config PINCONF
 	bool "Support pin configuration controllers"
 
+config GENERIC_PINCONF
+	bool
+	select PINCONF
+
 config DEBUG_PINCTRL
 	bool "Debug PINCTRL calls"
 	depends on DEBUG_KERNEL
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 97b2b9f..c7c5ff2 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -5,6 +5,7 @@ ccflags-$(CONFIG_DEBUG_PINMUX)	+= -DDEBUG
 obj-$(CONFIG_PINCTRL)		+= core.o
 obj-$(CONFIG_PINMUX)		+= pinmux.o
 obj-$(CONFIG_PINCONF)		+= pinconf.o
+obj-$(CONFIG_GENERIC_PINCONF)	+= pinconf-generic.o
 obj-$(CONFIG_PINMUX_SIRF)	+= pinmux-sirf.o
 obj-$(CONFIG_PINMUX_U300)	+= pinmux-u300.o
 obj-$(CONFIG_PINCTRL_COH901)	+= pinctrl-coh901.o
diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
new file mode 100644
index 0000000..06d0ef2
--- /dev/null
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -0,0 +1,89 @@
+/*
+ * Core driver for the generic pin config portions of the pin control subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#define pr_fmt(fmt) "generic pinconfig core: " fmt
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include "core.h"
+#include "pinconf.h"
+
+#ifdef CONFIG_DEBUG_FS
+
+struct pin_config_item {
+	const enum pin_config_param param;
+	const char * const display;
+	const char * const format;
+};
+
+#define PCONFDUMP(a, b, c) { .param = a, .display = b, .format = c }
+
+struct pin_config_item conf_items[] = {
+	PCONFDUMP(PIN_CONFIG_BIAS_DISABLE, "input bias disabled", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_HIGH_IMPEDANCE, "input bias high impedance", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", "Ohm"),
+	PCONFDUMP(PIN_CONFIG_BIAS_PULL_DOWN, "input bias pull down", "Ohm"),
+	PCONFDUMP(PIN_CONFIG_BIAS_HIGH, "input bias high", NULL),
+	PCONFDUMP(PIN_CONFIG_BIAS_GROUND, "input bias ground", NULL),
+	PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", "X stages"),
+	PCONFDUMP(PIN_CONFIG_DRIVE_OFF, "output drive off", NULL),
+	PCONFDUMP(PIN_CONFIG_INPUT_SCHMITT, "input schmitt trigger", NULL),
+	PCONFDUMP(PIN_CONFIG_INPUT_DEBOUNCE, "input debounce", "time units"),
+	PCONFDUMP(PIN_CONFIG_SLEW_RATE_RISING, "output slew rate rising", "of max"),
+	PCONFDUMP(PIN_CONFIG_SLEW_RATE_FALLING, "output slew rate falling", "of max"),
+	PCONFDUMP(PIN_CONFIG_POWER_SOURCE, "pin power source", "selector"),
+	PCONFDUMP(PIN_CONFIG_LOW_POWER_MODE, "pin low power", "mode"),
+	PCONFDUMP(PIN_CONFIG_WAKEUP, "input wakeup", NULL),
+};
+
+void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, int pin)
+{
+	const struct pinconf_ops *ops = pctldev->desc->confops;
+	int i;
+
+	if (!ops->is_generic)
+		return;
+
+	for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
+		unsigned long config;
+		int ret;
+
+		/* We want to check out this parameter */
+		config = (unsigned long) conf_items[i].param;
+		ret = pin_config_get(pctldev, pin, &config);
+		/* These are legal errors */
+		if (ret == -EINVAL || ret == -ENOTSUPP)
+			continue;
+		if (ret) {
+			seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
+			continue;
+		}
+		/* Space between multiple configs */
+		seq_puts(s, " ");
+		seq_puts(s, conf_items[i].display);
+		/* Print unit if available */
+		if (conf_items[i].format && config != 0)
+			seq_printf(s, " (%lu %s)", config,
+				   conf_items[i].format);
+	}
+}
+
+#endif
diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c
index e4e997a..7fd9fc5 100644
--- a/drivers/pinctrl/pinconf.c
+++ b/drivers/pinctrl/pinconf.c
@@ -169,6 +169,8 @@ static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
 {
 	const struct pinconf_ops *ops = pctldev->desc->confops;
 
+	/* no-op when not using generic pin config */
+	pinconf_generic_dump_pin(pctldev, s, pin);
 	if (ops && ops->pin_config_dbg_show)
 		ops->pin_config_dbg_show(pctldev, s, pin);
 }
diff --git a/drivers/pinctrl/pinconf.h b/drivers/pinctrl/pinconf.h
index eb1657a..b6886be 100644
--- a/drivers/pinctrl/pinconf.h
+++ b/drivers/pinctrl/pinconf.h
@@ -30,3 +30,23 @@ static inline void pinconf_init_device_debugfs(struct dentry *devroot,
 }
 
 #endif
+
+/*
+ * The following functions are available if the driver uses the generic
+ * pin config.
+ */
+
+#ifdef CONFIG_GENERIC_PINCONF
+
+void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+			      struct seq_file *s, int pin);
+
+#else
+
+static inline void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
+					    struct seq_file *s, int pin)
+{
+	return;
+}
+
+#endif
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
new file mode 100644
index 0000000..d444055
--- /dev/null
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -0,0 +1,142 @@
+/*
+ * Interface the generic pinconfig portions of the pinctrl subsystem
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * This interface is used in the core to keep track of pins.
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef __LINUX_PINCTRL_PINCONF_GENERIC_H
+#define __LINUX_PINCTRL_PINCONF_GENERIC_H
+
+/*
+ * You shouldn't even be able to compile with these enums etc unless you're
+ * using generic pin config. That is why this is defined out.
+ */
+#ifdef CONFIG_GENERIC_PINCONF
+
+/**
+ * enum pin_config_param - possible pin configuration parameters
+ * @PIN_CONFIG_BIAS_DISABLE: disable any pin bias on the pin, a
+ *	transition from say pull-up to pull-down implies that you disable
+ *	pull-up in the process, this setting disables all biasing.
+ * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance
+ *	mode, also know as "third-state" (tristate) or "high-Z" or "floating".
+ *	On output pins this effectively disconnects the pin, which is useful
+ *	if for example some other pin is going to drive the signal connected
+ *	to it for a while. Pins used for input are usually always high
+ *	impedance.
+ * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high
+ *	impedance to VDD), if the controller supports specifying a certain
+ *	pull-up resistance, this is given as an argument (in Ohms) when
+ *	setting this parameter.
+ * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high
+ *	impedance to GROUND), if the controller supports specifying a certain
+ *	pull-down resistance, this is given as an argument (in Ohms) when
+ *	setting this parameter.
+ * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD
+ * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND
+ * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and
+ *	low, this is the most typical case and is typically achieved with two
+ *	active transistors on the output. If the pin can support different
+ *	drive strengths for push/pull, the strength is given in the argument
+ *	as the number of driving stages vs nominal load impedance, so say
+ *	quadruple driving stages (usually 8 transistors rather than two) will
+ *	be configured with the 8 passed as argument.
+ * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open
+ *	collector) which means it is usually wired with other output ports
+ *	which are then pulled up with an external resistor. If the pin can
+ *	support different drive strengths for the open drain pin, the format
+ *	is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
+ * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain
+ *	(open emitter) which is the same as open drain but pulled to ground.
+ *	If the pin can support different drive strengths for the open drain
+ *	pin, the format is the same as for PIN_CONFIG_DRIVE_PUSH_PULL.
+ * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off.
+ * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
+ *	schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
+ *	the threshold value is given on a custom format as argument when
+ *	setting pins to this mode. The argument zero turns the schmitt trigger
+ *	off.
+ * @PIN_CONFIG_INPUT_DEBOUNCE: this will configure the pin to debounce mode,
+ *	which means it will wait for signals to settle when reading inputs. The
+ *	argument gives the debounce time on a custom format. Setting the
+ *	argument to zero turns debouncing off.
+ * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising
+ *	signals on the pin. The argument gives the rise time in fractions
+ *	compared to maximum rise time, 0 means nominal rise time. If you can
+ *	control slew rate in 4 steps these will likely be equidistant like
+ *	1/4, 1/2, 3/4 or full nominal slew rate, which means argument 4 gives
+ *	you 1/4 of nominal slew rate and the argument 4 has the same meaning
+ *	as 0 - nominal slew rate (fastest possible, steep edges). You may want
+ *	to adjust slew rates so that signal edges don't get too steep, causing
+ *	disturbances in surrounding electronics known as electromagnetic
+ *	interference (EMI) for example.
+ * @PIN_CONFIG_SLEW_RATE_FALLING: this will configure the slew rate for falling
+ *	signals on the pin. The argument gives the fall time in fractions
+ *	compared to nominal fall time.
+ * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
+ *	supplies, the argument to this parameter (on a custom format) tells
+ *	the driver which alternative power source to use.
+ * @PIN_CONFIG_LOW_POWER_MODE: this will configure the pin for low power
+ *	operation, if several modes of operation are supported these can be
+ *	passed in the argument on a custom form, else just use argument 1
+ *	to indicate low power mode, argument 0 turns low power mode off.
+ * @PIN_CONFIG_WAKEUP: this will configure an input pin such that if a
+ *	signal transition arrives at the pin when the pin controller/system
+ *	is sleeping, it will wake up the system if argument 1 is passed along.
+ *	Pass argument 0 to turn wakeup enablement off.
+ * @PIN_CONFIG_END: this is the last enumerator for pin configurations, if
+ *	you need to pass in custom configurations to the pin controller, use
+ *	PIN_CONFIG_END+1 as the base offset.
+ */
+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,
+};
+
+/*
+ * The following inlines stuffs a configuration parameter and data value
+ * into and out of an unsigned long argument, as used by the generic pin config
+ * system. We put the parameter in the lower 16 bits and the argument in the
+ * upper 16 bits.
+ */
+
+static inline enum pin_config_param to_config_param(unsigned long config)
+{
+	return (enum pin_config_param) (config & 0xffffUL);
+}
+
+static inline u16 to_config_argument(unsigned long config)
+{
+	return (enum pin_config_param) ((config >> 16) & 0xffffUL);
+}
+
+static inline unsigned long to_config_packed(enum pin_config_param param,
+					     u16 argument)
+{
+	return (argument << 16) | ((unsigned long) param & 0xffffUL);
+}
+
+#endif /* CONFIG_GENERIC_PINCONF */
+
+#endif /* __LINUX_PINCTRL_PINCONF_GENERIC_H */
diff --git a/include/linux/pinctrl/pinconf.h b/include/linux/pinctrl/pinconf.h
index b914847..c050a1f 100644
--- a/include/linux/pinctrl/pinconf.h
+++ b/include/linux/pinctrl/pinconf.h
@@ -19,6 +19,8 @@ struct pinctrl_dev;
 /**
  * struct pinconf_ops - pin config operations, to be implemented by
  * pin configuration capable drivers.
+ * @is_generic: for pin controllers that want to use the generic interface,
+ *	this flag tells the framework that it's generic.
  * @pin_config_get: get the config of a certain pin, if the requested config
  *	is not available on this controller this should return -ENOTSUPP
  *	and if it is available but disabled it should return -EINVAL
@@ -28,6 +30,9 @@ struct pinctrl_dev;
  *	per-device info for a certain pin in debugfs
  */
 struct pinconf_ops {
+#ifdef CONFIG_GENERIC_PINCONF
+	bool is_generic;
+#endif
 	int (*pin_config_get) (struct pinctrl_dev *pctldev,
 			       unsigned pin,
 			       unsigned long *config);
-- 
1.7.3.2

RE: [PATCH 2/2 v4] pinctrl: introduce generic pin config

[Stephen Warren]

Linus Walleij wrote at Thursday, November 24, 2011 11:46 AM:
> This is a split-off from the earlier patch set which adds generic
> pin configuration for the pin controllers that want it. Since
> we may have a system with mixed generic and custom pin controllers,
> we pass a boolean in the pin controller ops vtable to indicate
> if it is generic.

> diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c

> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
...
> +	for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
...
> +		/* We want to check out this parameter */
> +		config = (unsigned long) conf_items[i].param;

Don't you need to use to_config_packed() here?

> +		ret = pin_config_get(pctldev, pin, &config);
> +		/* These are legal errors */
> +		if (ret == -EINVAL || ret == -ENOTSUPP)
> +			continue;

Ah, I guess I see why you consider -ENOTSUPP an error that you don't want
to print to syslog. Maybe you should call _pin_config_get() here which
doesn't spew messages on -ENOTSUPP, but have the public pin_config_get()
function spew errors.

> +		if (ret) {
> +			seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
> +			continue;
> +		}
> +		/* Space between multiple configs */
> +		seq_puts(s, " ");
> +		seq_puts(s, conf_items[i].display);
> +		/* Print unit if available */
> +		if (conf_items[i].format && config != 0)
> +			seq_printf(s, " (%lu %s)", config,
> +				   conf_items[i].format);

Don't you need to use to_config_argument() here?

> diff --git a/drivers/pinctrl/pinconf.c b/drivers/pinctrl/pinconf.c

> @@ -169,6 +169,8 @@ static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
>  {
>  	const struct pinconf_ops *ops = pctldev->desc->confops;
> 
> +	/* no-op when not using generic pin config */
> +	pinconf_generic_dump_pin(pctldev, s, pin);
>  	if (ops && ops->pin_config_dbg_show)
>  		ops->pin_config_dbg_show(pctldev, s, pin);
>  }

This is really a comment on the previous patch, but what about config
params that only apply to groups?

> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h

> +/*
> + * You shouldn't even be able to compile with these enums etc unless you're
> + * using generic pin config. That is why this is defined out.
> + */
> +#ifdef CONFIG_GENERIC_PINCONF

Hmm. I'd prefer to have drivers able to use both generic values and
extend them with custom values. Can't we just use the top bit of the
param value to indicate 0:standard (from the enum below) 1:custom
(something meaningful to only the individual pinctrl driver). This
could then trigger calling pinconf_generic_dump_pin() or not for
example.

> +#ifdef CONFIG_GENERIC_PINCONF
> +	bool is_generic;
> +#endif

... and get rid of that flag.

-- 
nvpublic

Re: [PATCH 2/2 v4] pinctrl: introduce generic pin config

[Linus Walleij]

On Wed, Nov 30, 2011 at 8:45 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Thursday, November 24, 2011 11:46 AM:

>> +void pinconf_generic_dump_pin(struct pinctrl_dev *pctldev,
> ...
>> +     for(i = 0; i < ARRAY_SIZE(conf_items); i++) {
> ...
>> +             /* We want to check out this parameter */
>> +             config = (unsigned long) conf_items[i].param;
>
> Don't you need to use to_config_packed() here?

Yep! Fixed it.

>> +             ret = pin_config_get(pctldev, pin, &config);
>> +             /* These are legal errors */
>> +             if (ret == -EINVAL || ret == -ENOTSUPP)
>> +                     continue;
>
> Ah, I guess I see why you consider -ENOTSUPP an error that you don't want
> to print to syslog. Maybe you should call _pin_config_get() here which
> doesn't spew messages on -ENOTSUPP, but have the public pin_config_get()
> function spew errors.

I just deleted the error check for now since it's not necessary.
Better keep the code compact and simple.

>> +             if (ret) {
>> +                     seq_printf(s, "ERROR READING CONFIG SETTING %d ", i);
>> +                     continue;
>> +             }
>> +             /* Space between multiple configs */
>> +             seq_puts(s, " ");
>> +             seq_puts(s, conf_items[i].display);
>> +             /* Print unit if available */
>> +             if (conf_items[i].format && config != 0)
>> +                     seq_printf(s, " (%lu %s)", config,
>> +                                conf_items[i].format);
>
> Don't you need to use to_config_argument() here?

Yes. I'm too sloppy :-(

>> @@ -169,6 +169,8 @@ static void pinconf_dump_pin(struct pinctrl_dev *pctldev,
>>  {
>>       const struct pinconf_ops *ops = pctldev->desc->confops;
>>
>> +     /* no-op when not using generic pin config */
>> +     pinconf_generic_dump_pin(pctldev, s, pin);
>>       if (ops && ops->pin_config_dbg_show)
>>               ops->pin_config_dbg_show(pctldev, s, pin);
>>  }
>
> This is really a comment on the previous patch, but what about config
> params that only apply to groups?

Oh! I need to create a separate debugfs file for that.
Thanks for this nice catch!

>> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
>
>> +/*
>> + * You shouldn't even be able to compile with these enums etc unless you're
>> + * using generic pin config. That is why this is defined out.
>> + */
>> +#ifdef CONFIG_GENERIC_PINCONF
>
> Hmm. I'd prefer to have drivers able to use both generic values and
> extend them with custom values. Can't we just use the top bit of the
> param value to indicate 0:standard (from the enum below) 1:custom
> (something meaningful to only the individual pinctrl driver). This
> could then trigger calling pinconf_generic_dump_pin() or not for
> example.

Hm...

That will get messy when if I refactor this stuff, add new enums
and whatever.

>> +#ifdef CONFIG_GENERIC_PINCONF
>> +     bool is_generic;
>> +#endif
>
> ... and get rid of that flag.

This is for the case wher you have both generic and non-generic
config controllers onboard a system.

Like in the generic debugfs dump function:

	if (!ops->is_generic)
		return;

If I take this out, the generic debugfs code will be used for
everything.

And then the generic sematics which you didn't like in the
previous patch:

if (ret == -EINVAL || ret == -ENOTSUPP)

Need to go back in, else the generic debugfs stuff won't
work.

I'm really not sure about that kind of extension, it feels to
me like a hard-to grasp unstable middleground between two
clean-cut solutions "all custom" or "all generic".

Yours,
Linus Walleij

RE: [PATCH 2/2 v4] pinctrl: introduce generic pin config

[Stephen Warren]

Linus Walleij wrote at Thursday, December 01, 2011 3:12 AM:
> On Wed, Nov 30, 2011 at 8:45 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Linus Walleij wrote at Thursday, November 24, 2011 11:46 AM:
> 
> >> diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
> >
> >> +/*
> >> + * You shouldn't even be able to compile with these enums etc unless you're
> >> + * using generic pin config. That is why this is defined out.
> >> + */
> >> +#ifdef CONFIG_GENERIC_PINCONF
> >
> > Hmm. I'd prefer to have drivers able to use both generic values and
> > extend them with custom values. Can't we just use the top bit of the
> > param value to indicate 0:standard (from the enum below) 1:custom
> > (something meaningful to only the individual pinctrl driver). This
> > could then trigger calling pinconf_generic_dump_pin() or not for
> > example.
> 
> Hm...
> 
> That will get messy when if I refactor this stuff, add new enums
> and whatever.

I don't understand what'd be difficult about that.

New standardized enums could be added with values without the top bit
set. No existing driver would need modification, since their switch(param)
would not have a case for that new value, and would just return an error.

> >> +#ifdef CONFIG_GENERIC_PINCONF
> >> +     bool is_generic;
> >> +#endif
> >
> > ... and get rid of that flag.
> 
> This is for the case wher you have both generic and non-generic
> config controllers onboard a system.
> 
> Like in the generic debugfs dump function:
> 
> 	if (!ops->is_generic)
> 		return;
> 
> If I take this out, the generic debugfs code will be used for
> everything.

I think that'd be fine; the generic code would do all the debug prints
for the standardized enums, then the core would call into the pinctrl
driver to perform any additional debug prints for any driver-defined
custom parameters.

> And then the generic sematics which you didn't like in the
> previous patch:
> 
> if (ret == -EINVAL || ret == -ENOTSUPP)
> 
> Need to go back in, else the generic debugfs stuff won't
> work.

That'd be fine, provided the loop only checked the standardized parameters,
or only enabled that special error-checking case for standardized parameters.

-- 
nvpublic

Re: [PATCH 2/2 v4] pinctrl: introduce generic pin config

[Linus Walleij]

On Thu, Dec 1, 2011 at 6:16 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Thursday, December 01, 2011 3:12 AM:

>> That will get messy when if I refactor this stuff, add new enums
>> and whatever.
>
> I don't understand what'd be difficult about that.
>
> New standardized enums could be added with values without the top bit
> set. No existing driver would need modification, since their switch(param)
> would not have a case for that new value, and would just return an error.

That would be mixing binary #defines and enums in an unholy
manner, that sounds bad to me.

If you want to do things like that I should replace the current

enum pin_config_param {
        PIN_CONFIG_BIAS_DISABLE,
        PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
       ...
}

With

#define PIN_CONFIG_BIAS_DISABLE 0x00000001
#define PIN_CONFIG_BIAS_HIGH_IMPEDANCE 0x00000002
...

Else it seems like a bit wicked mix-up. So is this what we
should do?

I am reluctant since it mirrors the problem in the GPIO
numberspace where you have no clue what, say GPIO
164 is on a multi-platform binary. It depends on which
platform it booted on. The same will be true for such
enums/defines above the predefined range, totally
depending on the system at hand.

>> Like in the generic debugfs dump function:
>>
>>       if (!ops->is_generic)
>>               return;
>>
>> If I take this out, the generic debugfs code will be used for
>> everything.
>
> I think that'd be fine; the generic code would do all the debug prints
> for the standardized enums, then the core would call into the pinctrl
> driver to perform any additional debug prints for any driver-defined
> custom parameters.

I think Marks point earlier was that he wanted the possibility
to cut out *all* the generic stuff and have only custom config
enumerators for a certain pin controller.

>> And then the generic sematics which you didn't like in the
>> previous patch:
>>
>> if (ret == -EINVAL || ret == -ENOTSUPP)
>>
>> Need to go back in, else the generic debugfs stuff won't
>> work.
>
> That'd be fine, provided the loop only checked the standardized parameters,
> or only enabled that special error-checking case for standardized parameters.

Same comment as above. This won't cut it for all-custom
pin controllers, they will get unwanted semantics pushed at
them.

Thanks,
Linus Walleij

Re: [PATCH 2/2 v4] pinctrl: introduce generic pin config

[Mark Brown]

On Mon, Dec 05, 2011 at 02:35:07PM +0100, Linus Walleij wrote:
> On Thu, Dec 1, 2011 at 6:16 PM, Stephen Warren <swarren@nvidia.com> wrote:

> > I think that'd be fine; the generic code would do all the debug prints
> > for the standardized enums, then the core would call into the pinctrl
> > driver to perform any additional debug prints for any driver-defined
> > custom parameters.

> I think Marks point earlier was that he wanted the possibility
> to cut out *all* the generic stuff and have only custom config
> enumerators for a certain pin controller.

Pretty much.  Or at least have a way of doing that.  But I do think this
should have a way of coexisting with the standard properties where
that's possible.

My main concern here is that going through too much of an abstraction
layer looks like it could be an awful lot more work and I'm not entirely
clear what it'd buy us - things like readback debug decode into user
readable values are an example of a useful thing, though.

RE: [PATCH 2/2 v4] pinctrl: introduce generic pin config

[Stephen Warren]

Linus Walleij wrote at Monday, December 05, 2011 6:35 AM:
> On Thu, Dec 1, 2011 at 6:16 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Linus Walleij wrote at Thursday, December 01, 2011 3:12 AM:
> 
> >> That will get messy when if I refactor this stuff, add new enums
> >> and whatever.
> >
> > I don't understand what'd be difficult about that.
> >
> > New standardized enums could be added with values without the top bit
> > set. No existing driver would need modification, since their switch(param)
> > would not have a case for that new value, and would just return an error.
> 
> That would be mixing binary #defines and enums in an unholy
> manner, that sounds bad to me.

I don't undertand that; what defines and enums would be mixed together?

> If you want to do things like that I should replace the current
> 
> enum pin_config_param {
>         PIN_CONFIG_BIAS_DISABLE,
>         PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
>        ...
> }
> 
> With
> 
> #define PIN_CONFIG_BIAS_DISABLE 0x00000001
> #define PIN_CONFIG_BIAS_HIGH_IMPEDANCE 0x00000002
> ...
> 
> Else it seems like a bit wicked mix-up. So is this what we
> should do?

That seems fine to me.

> I am reluctant since it mirrors the problem in the GPIO
> numberspace where you have no clue what, say GPIO
> 164 is on a multi-platform binary. It depends on which
> platform it booted on. The same will be true for such
> enums/defines above the predefined range, totally
> depending on the system at hand.

Custom params always mean something driver-specific, irrespective of
whether you can expose both standard and custom params in the same driver
or whether you're in a multi-SoC binary or not.

If we did something like use the top bit (0x8000000) to indicate standard-
vs-custom, it should be pretty obvious whether you're dealing with a
standard property or not, and when you have to go look at the driver.

In fact, this seems better than having a flag in the pinctrl driver
registration for this, since in that case, you /always/ have to go look
at the driver to interpret /any/ param, whereas with an explicit range
of standard params, you can at least know it's safe to least interpret
standard params without consulting the driver code.

> >> Like in the generic debugfs dump function:
> >>
> >>       if (!ops->is_generic)
> >>               return;
> >>
> >> If I take this out, the generic debugfs code will be used for
> >> everything.
> >
> > I think that'd be fine; the generic code would do all the debug prints
> > for the standardized enums, then the core would call into the pinctrl
> > driver to perform any additional debug prints for any driver-defined
> > custom parameters.
> 
> I think Marks point earlier was that he wanted the possibility
> to cut out *all* the generic stuff and have only custom config
> enumerators for a certain pin controller.

You can still have that. Drivers which use standard params can select
support for them, and drivers which only use custom params don't have to.
Irrespective, having a driver expose standard params doesn't mean that
the core need to include the debugfs code to dump those params anyway;
as far as param get/set, it should simply be passing the values to the
driver without munging them anyway.

-- 
nvpublic