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

[Stephen Warren]

On 04/01/2013 12:59 PM, Tony Prisk wrote:
> 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

>>> +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.

Does the pinctrl core guarantee that the map table entries get removed
when unregistering the pincontrol driver? If it does, I guess that
change is safe. If not, perhaps not. I guess it must though.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 3/6] pinctrl: gpio: vt8500: Add pincontrol driver for arch-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 

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(>dev, sizeof(*data), GFP_KERNEL);
+   if (!data) {
+   dev_err(>dev, "failed to allocate data\n");
+   return -ENOMEM;
+   }
+
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   data->base = devm_request_and_ioremap(>dev, res);
+   if (!data->base) {
+   dev_err(>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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Stephen Warren]

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;

> +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?

> +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:

> +static struct gpio_chip wmt_gpio_chip = {
...
> + .of_xlate = wmt_gpio_of_xlate,

Aside from that, this patch,

Reviewed-by: Stephen Warren 

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(>dev, sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + dev_err(>dev, "failed to allocate data\n");
> + return -ENOMEM;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + data->base = devm_request_and_ioremap(>dev, res);
> + if (!data->base) {
> + dev_err(>dev, "failed to map memory resource\n");
> + return -EBUSY;
> + }

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

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

[Stephen Warren]

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;

 +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?

 +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:

 +static struct gpio_chip wmt_gpio_chip = {
...
 + .of_xlate = wmt_gpio_of_xlate,

Aside from that, this patch,

Reviewed-by: Stephen Warren swar...@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;
 + }

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 3/6] pinctrl: gpio: vt8500: Add pincontrol driver for arch-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 swar...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Stephen Warren]

On 04/01/2013 12:59 PM, Tony Prisk wrote:
 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

 +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.

Does the pinctrl core guarantee that the map table entries get removed
when unregistering the pincontrol driver? If it does, I guess that
change is safe. If not, perhaps not. I guess it must though.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/