lkml.org 
[lkml]   [2013]   [Apr]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCHv3 3/6] pinctrl: gpio: vt8500: Add pincontrol driver for arch-vt8500
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@nvidia.com>
>
> 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


\
 
 \ /
  Last update: 2013-04-01 21:41    [W:0.058 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site