[PATCH] pinctrl: imx: remove unused gpoup_index field

[PATCH] pinctrl: imx: remove unused gpoup_index field

[Vladimir Zapolskiy]

The group_index field of struct imx_pinctrl_soc_info does not serve
any particular purpose and its usage can be safely replaced by
a preexisting local variable.

Also Stefan Agner reports that the usage of the group_index field
without reinitialization may lead to an oops on a repeated driver
probe, this is found with DEBUG_TEST_DRIVER_REMOVE option enabled.

Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
---
The change is supposed to obsolete Stefan's change:

   https://patchwork.ozlabs.org/patch/683885/

The change is tested on i.MX6Q platform only.

 drivers/pinctrl/freescale/pinctrl-imx.c | 2 +-
 drivers/pinctrl/freescale/pinctrl-imx.h | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 47613201269a..3425d10c1478 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -628,7 +628,7 @@ static int imx_pinctrl_parse_functions(struct device_node *np,
 
 	for_each_child_of_node(np, child) {
 		func->groups[i] = child->name;
-		grp = &info->groups[info->group_index++];
+		grp = &info->groups[i];
 		imx_pinctrl_parse_groups(child, grp, info, i++);
 	}
 
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h b/drivers/pinctrl/freescale/pinctrl-imx.h
index 8af8aa2897ab..eaf893967453 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.h
+++ b/drivers/pinctrl/freescale/pinctrl-imx.h
@@ -78,7 +78,6 @@ struct imx_pinctrl_soc_info {
 	struct imx_pin_reg *pin_regs;
 	struct imx_pin_group *groups;
 	unsigned int ngroups;
-	unsigned int group_index;
 	struct imx_pmx_func *functions;
 	unsigned int nfunctions;
 	unsigned int flags;
-- 
2.8.1

Re: [PATCH] pinctrl: imx: remove unused gpoup_index field

[Stefan Agner]

Hi Vladimir,

On 2016-10-18 15:47, Vladimir Zapolskiy wrote:
> The group_index field of struct imx_pinctrl_soc_info does not serve
> any particular purpose and its usage can be safely replaced by
> a preexisting local variable.
> 
> Also Stefan Agner reports that the usage of the group_index field
> without reinitialization may lead to an oops on a repeated driver
> probe, this is found with DEBUG_TEST_DRIVER_REMOVE option enabled.

The change is a good idea, and it works for flat pinctrl configurations.
However it does not work when there are multiple sub nodes (or function
nodes).

I am all in for the new flat format, but we should at least make sure
that all device trees are flat or have only one subnode. If that is the
case, we also should add a warning... All that makes me wonder if it is
worth the effort...

Also added Robin Gon which added the field not long ago.

--
Stefan


> 
> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> ---
> The change is supposed to obsolete Stefan's change:
> 
>    https://patchwork.ozlabs.org/patch/683885/
> 
> The change is tested on i.MX6Q platform only.
> 
>  drivers/pinctrl/freescale/pinctrl-imx.c | 2 +-
>  drivers/pinctrl/freescale/pinctrl-imx.h | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c
> b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 47613201269a..3425d10c1478 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -628,7 +628,7 @@ static int imx_pinctrl_parse_functions(struct
> device_node *np,
>  
>  	for_each_child_of_node(np, child) {
>  		func->groups[i] = child->name;
> -		grp = &info->groups[info->group_index++];
> +		grp = &info->groups[i];
>  		imx_pinctrl_parse_groups(child, grp, info, i++);
>  	}
>  
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.h
> b/drivers/pinctrl/freescale/pinctrl-imx.h
> index 8af8aa2897ab..eaf893967453 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.h
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.h
> @@ -78,7 +78,6 @@ struct imx_pinctrl_soc_info {
>  	struct imx_pin_reg *pin_regs;
>  	struct imx_pin_group *groups;
>  	unsigned int ngroups;
> -	unsigned int group_index;
>  	struct imx_pmx_func *functions;
>  	unsigned int nfunctions;
>  	unsigned int flags;

Re: [PATCH] pinctrl: imx: remove unused gpoup_index field

[Linus Walleij]

On Thu, Oct 20, 2016 at 10:17 PM, Stefan Agner <stefan@agner.ch> wrote:
> Hi Vladimir,
>
> On 2016-10-18 15:47, Vladimir Zapolskiy wrote:
>> The group_index field of struct imx_pinctrl_soc_info does not serve
>> any particular purpose and its usage can be safely replaced by
>> a preexisting local variable.
>>
>> Also Stefan Agner reports that the usage of the group_index field
>> without reinitialization may lead to an oops on a repeated driver
>> probe, this is found with DEBUG_TEST_DRIVER_REMOVE option enabled.
>
> The change is a good idea, and it works for flat pinctrl configurations.
> However it does not work when there are multiple sub nodes (or function
> nodes).
>
> I am all in for the new flat format, but we should at least make sure
> that all device trees are flat or have only one subnode. If that is the
> case, we also should add a warning... All that makes me wonder if it is
> worth the effort...
>
> Also added Robin Gon which added the field not long ago.

I think I need Shawn's or Sascha's input here before applying.

My opinion is certainly not very useful, all of you know
these systems better than me.

Yours,
Linus Walleij