Re: [PATCHv3 3/6] pinctrl: gpio: vt8500: Add pincontrol driver forarch-vt8500

[Tony Prisk]

On 02/04/13 06:06, Stephen Warren wrote:
> On 03/28/2013 12:10 AM, Tony Prisk wrote:
> > This patch adds support for the GPIO/pinmux controller found on the VIA
> > VT8500 and Wondermedia WM8xxx-series SoCs.
> >
> > Each pin within the controller is capable of operating as a GPIO or as
> > an alternate function. The pins are numbered according to their control
> > bank/bit so that if new pins are added, the existing numbering is maintained.
> >
> > All currently supported SoCs are included: VT8500, WM8505, WM8650, WM8750 and
> > WM8850.
> > diff --git a/drivers/pinctrl/vt8500/pinctrl-wmt.c b/drivers/pinctrl/vt8500/pinctrl-wmt.c
> > +static int wmt_pctl_dt_node_to_map_pull(struct wmt_pinctrl_data *data,
> > +					struct device_node *np,
> > +					u32 pin, u32 pull,
> > +					struct pinctrl_map **maps)
> > +	configs[0] = 0;
> I assume that should be
>
> 	configs[0] = pull;
Err, yeah. Oops.
> > +static int wmt_pctl_dt_node_to_map(struct pinctrl_dev *pctldev,
> > +				   struct device_node *np,
> > +				   struct pinctrl_map **map,
> > +				   unsigned *num_maps)
> > +fail:
> > +	kfree(maps);
> > +	return err;
> > +}
> There, I think you also want to iterate over maps[] and free
> map->data.configs.config for any PIN_MAP_TYPE_CONFIGS_PIN.
>
> Perhaps just call wmt_pctl_dt_free_map() here, with roughly nmaps =
> cur_map - maps?
I have dropped the kfree() and used devm_kzalloc instead. Makes the fail 
path tidier as well.
> > +static int wmt_gpio_of_xlate(struct gpio_chip *gc,
> > +				const struct of_phandle_args *gpiospec,
> > +				u32 *flags)
> > +{
> > +	if (flags)
> > +		*flags = gpiospec->args[1];
> > +
> > +	return gpiospec->args[0];
> > +}
> Can't you use of_gpio_simple_xlate(), and hence just not set .of_xlate in:
I have dropped this - as you pointed out, ..simple_xlate() is an 
equivalent.
> > +static struct gpio_chip wmt_gpio_chip = {
> ...
> > +	.of_xlate = wmt_gpio_of_xlate,
> Aside from that, this patch,
>
> Reviewed-by: Stephen Warren <swarren@xxxxxxxxxx>
>
> Although I didn't review pinctrl-*.c other than pinctrl-wmt.c, since
> they're just big tables of data. Oh, except that the following could
> probably be moved inside wmt_pinctrl_probe()?
>
> > +	struct wmt_pinctrl_data *data;
> > +	struct resource *res;
> > +
> > +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> > +	if (!data) {
> > +		dev_err(&pdev->dev, "failed to allocate data\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	data->base = devm_request_and_ioremap(&pdev->dev, res);
> > +	if (!data->base) {
> > +		dev_err(&pdev->dev, "failed to map memory resource\n");
> > +		return -EBUSY;
> > +	}
I have moved the resource-related portion of this code to the common 
init. The data part needs to be here to assign the per-soc variables to.

Thanks for the review.
Regards
Tony P
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/