[PATCH v3 1/9] pinctrl: mvebu: pinctrl driver core

[swarren@wwwdotorg.org (Stephen Warren)]

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 <sebastian.hesselbarth@gmail.com> a ?crit :
> 
>> 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:
> 
>> +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.