linux-kernel - Re: [PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4FBFBB74.901@wwwdotorg.org>
Date:	Fri, 25 May 2012 11:03:48 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Dong Aisheng <b29396@...escale.com>
CC:	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linus.walleij@...ricsson.com, devicetree-discuss@...ts.ozlabs.org,
	grant.likely@...retlab.ca, rob.herring@...xeda.com
Subject: Re: [PATCH v4 6/6] pinctrl: add pinctrl gpio binding support

On 05/25/2012 07:36 AM, Dong Aisheng wrote:
> From: Dong Aisheng <dong.aisheng@...aro.org>
> 
> This patch implements a standard common binding for pinctrl gpio ranges.
> Each SoC can add gpio ranges through device tree by adding a gpio-maps property
> under their pinctrl devices node with the format:
> <&gpio $gpio-specifier $pin_offset $count>
> while the gpio phandle and gpio-specifier are the standard approach
> to represent a gpio in device tree.
> Then we can cooperate it with the gpio xlate function to get the gpio number
> from device tree to set up the gpio ranges map.
> 
> Then the pinctrl driver can call pinctrl_dt_add_gpio_ranges(pctldev, node)
> to parse and register the gpio ranges from device tree.
> 
> Signed-off-by: Dong Aisheng <dong.aisheng@...aro.org>
> ---
> Personally i'm not very satisfied with current solution due to a few reasons:
> 1) i can not user standard gpio api to get gpio number
> 2) i need to reinvent a new api of_parse_phandles_with_args_ext which i'm not
> sure if it can be accepted by DT maintainer.
> If i did not invent that API, i need to rewrite a lot of duplicated code
> with slight differences with the exist functions like of_get_named_gpio_flags
> and of_parse_phandle_with_args for the special pinctrl gpio maps format.
> 
> So i just sent it out first to see people's comment and if any better solution.
> 
> One alternative solution is that that the gpio-maps into two parts:
> pinctrl-gpios = <&gpio_phandle gpio-specifier ..>
> pinctrl-gpio-maps = <pin_id count ..>
> Then we can reuse the standard gpio api altough it's not better than the
> original one.

The problem I see with that is that it splits what is essentially a
single array with phandle+specifier+pin-id+count into two separate
arrays. Anyone reading/editing the DT needs to fully understand this,
and keep the entries in the two properties in the same order. Putting
everything into a single property makes this much more obvious to me. I
personally don't see any issue with the
of_parse_phandles_with_args_ext() function; it seems pretty clean to me.

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

> +	if (!nranges) {
> +		dev_err(pctldev->dev, "no gpio ranges found\n");
> +		return -ENODEV;
> +	}

In the case of a generic pinctrl IP block that can support an external
GPIO controller but happens not to be hooked up to one within a
particular SoC, that might not be an error. However, that situation is
pretty unlikely, so I think it's find to call dev_err() for now, and we
can change it later if we need.

> +		ranges[i].base = ranges[i].gc->of_xlate(ranges[i].gc, &gpiospec, NULL);

I believe Grant wants to change the of_xlate prototype in order to be
able to return a different gc value, so this will probably need slight
rework work with that change, once they're both approved. Still, I think
this is fine for now.

> +		if (ranges[i].base < 0) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +		ranges[i].base += ranges[i].gc->base;
> +		ranges[i].pin_base = gpiospec.args[gpiospec.args_count - 2];
> +		ranges[i].npins = gpiospec.args[gpiospec.args_count - 1];
> +
> +		gpiochip_put(ranges[i].gc);

I wonder if this shouldn't happen until the pinctrl device is free'd,
and all the GPIO ranges are removed from it?

If we don't do that, I would argue that we shouldn't store ranges[i].gc,
since it might become invalid - I believe the only use of it is within
this function?

> +		of_node_put(gpiospec.np);
> +	}

Aside from the comments I've made, this series all seems reasonable.
There certainly are alternative ways of doing some of it, but I don't
see any other approach having any particular advantage over this one.
So, the series,

Acked-by: Stephen Warren <swarren@...dotorg.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/