On 09/11/2012 08:44 AM, Thomas Petazzoni wrote:

Hello Sebastian,

Sorry for getting back to you so late about this patch set. I have been
very busy with other things.

Le Mon, 10 Sep 2012 10:39:38 +0200,
Sebastian Hesselbarth [off-list ref] a ?crit :

quoted

v3:
- list of functions is now built out of pin groups passed to core driver
  instead of parsing DT node.

Even though I have gone through your discussion with Stephen Warren on
this, I don't get what you have done exactly, and I'm even more puzzled
by the following piece of code:

quoted

+static int __devinit _add_function(const char **funcs, const char *name)
+{
+	int n = 0;
+
+	while (funcs[n]) {
+		/* function already there */
+		if (strcmp(funcs[n], name) == 0)
+			return -EEXIST;
+		n++;
+	}
+	funcs[n] = name;
+	return 0;
+}
+
+static int __devinit mvebu_pinctrl_build_functions(struct platform_device *pdev,
+						   struct mvebu_pinctrl *pctl)
+{
+	const char **prefunc = kzalloc(sizeof(char *), GFP_KERNEL);
+	int num = 0;
+	int n, s;
+
+	for (n = 0; n < pctl->num_groups; n++) {
+		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
+		for (s = 0; s < grp->num_settings; s++) {
+			/* skip unsupported settings on this variant */
+			if (pctl->variant &&
+			    !(pctl->variant & grp->settings[s].variant))
+				continue;
+
+			/* check for unique functions */
+			if (_add_function(prefunc, grp->settings[s].name))
+				continue;
+
+			num++;
+		}
+	}
+	return 0;
+}

What is this supposed to do? It allocates an array prefunc, whose
reference is only stored in a local variable, and anywhere else, so
basically it does nothing except leaking memory unless I got it wrong.

I imagine this is related to the way that the SoC-specific drivers
provide their configuration to the generic core driver. I'm not sure the
data structures used for this purpose are the best design.

The pinctrl core expects lists of:

* Pins

* Groups of pins (each being a name and an array of pins in the group)

* Functions that can be muxed onto the groups (each function being a
global entity rather than something with a pin or group, and each
function being a name, and an array of groups where the function can be
selected).

However, the drivers in this patch seem to invert the data-structures a
little - in other words, instead of defining a global list of functions,
they define a list of groups, and for each group, list the functions
that can be selected on to it.

In turn, that probably requires the core mvebu driver to invert these
data-structures at run-time in order to provide the data the pinctrl
core needs. I think it'd be better to just have each SoC-specific driver
store the data tables in the same format that the pinctrl core needs it,
so that the mvebu pinctrl core won't have to process the data-structures
at all.

In particular, the following data structure is what I'm talking about:

+static struct mvebu_mpp_mode dove_mpp_modes[] = {
+	MPP_MODE(0,
+		MPP_FUNCTION(0x00, "gpio", NULL),
+		MPP_FUNCTION(0x02, "uart2", "rts"),
+		MPP_FUNCTION(0x03, "sdio0", "cd"),
+		MPP_FUNCTION(0x0f, "lcd0", "pwm"),
+		MPP_FUNCTION(0x10, "pmu", NULL)),

it's defining the functions within the context of a particular group
("mode" in the drivers terminology, I think...) rather than defining
functions and groups as separate top-level tables.