Re: [PATCH RFC v2 2/2] pinctrl: add pinctrl gpio binding support

[Stephen Warren <swarren@wwwdotorg.org>]

On 05/18/2012 07:12 AM, Dong Aisheng wrote:
> The gpio ranges standard dt binding format is
> <&gpio $gpio_offset $pin_offset $npin>
> 
> The core will parse and register the pinctrl gpio ranges
> from device tree.

Overall this conceptually makes sense, and looks good. Some comments though:

> +/*
> + * pinctrl_dt_add_gpio_range() - parse and register GPIO ranges from device tree
> + * @pctldev: pin controller device to add the range to
> + * @np: the device node contains the property @propname
> + * @propname: a list of phandles of gpios and corresponding data.
> + *  The format is: <&gpio0 $gpio_offset $pin_offset $count>
> + */
> +int pinctrl_dt_add_gpio_ranges(struct pinctrl_dev *pctldev,
> +				struct device_node *np,
> +				const char *propname)

If this will be fully standardized, perhaps we should just choose a
standard property name, rather than allowing different drivers to do
different things.

Perhaps "gpio-map"?

> +	nranges = size /4;

Add a space after the /

> +	ranges = devm_kzalloc(pctldev->dev, nranges * sizeof(*ranges),
> +				GFP_KERNEL);
> +	for (i = 0; i < nranges; i++) {
> +			phandle = be32_to_cpup(list++);

Looks like double indentation througout this block?

> +			np_gpio = of_find_node_by_phandle(phandle);
> +			if (!np_gpio) {
> +				dev_err(pctldev->dev,
> +					"failed to find gpio node(%s)\n",
> +					np_gpio->name);

Perhaps devm_kfree(ranges) here so that if this is called multiple times
due to deferred probe, the allocations from the failed attempts don't
accumulate. Same for other error paths.

> +				return -ENODEV;
> +			}
> +
> +			ranges[i].gc = of_node_to_gpiochip(np_gpio);

Of course, there is the outstanding question of whether this API will
exist and how it will work.

I think you can of_node_put(np_gpio) here, before the error-check for
ranges[i].gc, to avoid having to do it in all the error paths below.

Do you need to xxx_get(ranges[i].gc) to prevent it going away, and put()
it when removing the ranges?

> +			if (gpio_offset < 0 ||
> +			    gpio_offset > ranges[i].gc->ngpio ||
> +			    pin_offset < 0 || npins < 0) {

These are all unsigned, so most of those conditions can never be true.

Perhaps replace the remaining check with:

	gpio_offset + npins > ranges[i].gc->ngpio

to make sure the range isn't too long either

> +	pinctrl_add_gpio_ranges(pctldev, ranges, nranges);

I think we should also store ranges somewhere separate in pctldev ...

> +void pinctrl_dt_remove_gpio_ranges(struct pinctrl_dev *pctldev,
> +				   struct pinctrl_gpio_range *ranges,
> +				   unsigned nranges)

... so that this function doesn't need ranges/nranges passed to it; it
can just get them out of pctldec.

Or better yet, what if pinctrl_unregister automatically removed any
ranges registered by pinctrl_dt_add_gpio_ranges, so this function
doesn't even need to exist?