Re: [PATCH V2 6/6] pinctrl: tegra: Add complete device tree support

[Stephen Warren <swarren@wwwdotorg.org>]

On 03/21/2012 03:35 AM, Dong Aisheng wrote:
> On Wed, Mar 21, 2012 at 01:44:39AM +0800, Stephen Warren wrote:
>> Implement pinctrl_ops dt_node_to_map() and dt_free_map(). These allow
>> complete specification of the desired pinmux configuration using device
>> tree.
>>
>> Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
>> ---
>> v2: Rebase on of_property_for_each_string() API changes.
>> ---
> Nice code and a good example to people.
> 
> A small suggestion below:
>> +static int add_map_configs(struct pinctrl_map **map, unsigned *num_maps,
>> +			   const char *group, unsigned long *configs,
>> +			   unsigned num_configs)
>> +{
>> +	unsigned i = *num_maps;
>> +	unsigned long *dup_configs;
>> +	int ret;
>> +
>> +	dup_configs = kmemdup(configs, num_configs * sizeof(*dup_configs),
>> +			      GFP_KERNEL);
>> +	if (!dup_configs)
>> +		return -ENOMEM;
>> +
>> +	ret = add_map(map, num_maps);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	(*map)[i].type = PIN_MAP_TYPE_CONFIGS_GROUP;
>
> It still does not support PIN_MAP_TYPE_CONFIGS_PIN, right?

Yes.

This is mainly due to a pinctrl core limitation. The core only supports
muxing on groups, so even though the Tegra30 HW supports muxing per pin,
the driver must define a group for each pin. Given that, it's simplest
just to do all the pin config on those same groups.

If/when the pinctrl core supports muxing per pin, we can take advantage
of this within the Tegra pinctrl driver without affecting the binding at
all.

>> +	for_each_child_of_node(np_config, np) {
>> +		ret = of_property_read_string(np, "nvidia,function", &function);
>> +		if (ret < 0)
>> +			function = NULL;
>> +
>> +		for (i = 0; i < ARRAY_SIZE(cfg_params); i++) {
>> +			ret = of_property_read_u32(np, cfg_params[i].property,
>> +						   &val);
>> +			if (!ret) {
>> +				config = TEGRA_PINCONF_PACK(
>> +						cfg_params[i].param, val);
>> +				ret = add_config(&configs, &num_configs,
>> +						 config);
>> +				if (ret < 0)
>> +					goto error;
>> +			}
>> +		}
>> +
>> +		of_property_for_each_string(np, "nvidia,pins", prop, group) {
>
> If we calculate out the strings count and allocate corresponding size array, we may not
> need to keep krealloc the maps and configs array size for each entry.
> And this may be a little higher efficient.

That's true. However, it'd require the code to loop once to determine
how many properties are present and how many entries there are in the
pin list. Then, loop again to actually construct the mapping table
array. This is all added complexity that doesn't affect correctness. I'd
rather get the simple code going first, and then refine it later if
there turns out to be a performance issue.