diff mbox series

[v2] pinctrl-single: fix pcs_parse_pinconf() return value

Message ID 20200608125143.GA2789203@x1 (mailing list archive)
State Mainlined
Commit f46fe79ff1b65692a65266a5bec6dbe2bf7fc70f
Headers show
Series [v2] pinctrl-single: fix pcs_parse_pinconf() return value | expand

Commit Message

Drew Fustini June 8, 2020, 12:51 p.m. UTC
This patch causes pcs_parse_pinconf() to return -ENOTSUPP when no
pinctrl_map is added.  The current behavior is to return 0 when
!PCS_HAS_PINCONF or !nconfs.  Thus pcs_parse_one_pinctrl_entry()
incorrectly assumes that a map was added and sets num_maps = 2.

Analysis:
=========
The function pcs_parse_one_pinctrl_entry() calls pcs_parse_pinconf()
if PCS_HAS_PINCONF is enabled.  The function pcs_parse_pinconf()
returns 0 to indicate there was no error and num_maps is then set to 2:

 980 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 981                                                 struct device_node *np,
 982                                                 struct pinctrl_map **map,
 983                                                 unsigned *num_maps,
 984                                                 const char **pgnames)
 985 {
<snip>
1053         (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
1054         (*map)->data.mux.group = np->name;
1055         (*map)->data.mux.function = np->name;
1056
1057         if (PCS_HAS_PINCONF && function) {
1058                 res = pcs_parse_pinconf(pcs, np, function, map);
1059                 if (res)
1060                         goto free_pingroups;
1061                 *num_maps = 2;
1062         } else {
1063                 *num_maps = 1;
1064         }

However, pcs_parse_pinconf() will also return 0 if !PCS_HAS_PINCONF or
!nconfs.  I believe these conditions should indicate that no map was
added by returning -ENOTSUPP. Otherwise pcs_parse_one_pinctrl_entry()
will set num_maps = 2 even though no maps were successfully added, as
it does not reach "m++" on line 940:

 895 static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
 896                              struct pcs_function *func,
 897                              struct pinctrl_map **map)
 898
 899 {
 900         struct pinctrl_map *m = *map;
<snip>
 917         /* If pinconf isn't supported, don't parse properties in below. */
 918         if (!PCS_HAS_PINCONF)
 919                 return 0;
 920
 921         /* cacluate how much properties are supported in current node */
 922         for (i = 0; i < ARRAY_SIZE(prop2); i++) {
 923                 if (of_find_property(np, prop2[i].name, NULL))
 924                         nconfs++;
 925         }
 926         for (i = 0; i < ARRAY_SIZE(prop4); i++) {
 927                 if (of_find_property(np, prop4[i].name, NULL))
 928                         nconfs++;
 929         }
 930         if (!nconfs)
 919                 return 0;
 932
 933         func->conf = devm_kcalloc(pcs->dev,
 934                                   nconfs, sizeof(struct pcs_conf_vals),
 935                                   GFP_KERNEL);
 936         if (!func->conf)
 937                 return -ENOMEM;
 938         func->nconfs = nconfs;
 939         conf = &(func->conf[0]);
 940         m++;

This situtation will cause a boot failure [0] on the BeagleBone Black
(AM3358) when am33xx_pinmux node in arch/arm/boot/dts/am33xx-l4.dtsi
has compatible = "pinconf-single" instead of "pinctrl-single".

The patch fixes this issue by returning -ENOSUPP when !PCS_HAS_PINCONF
or !nconfs, so that pcs_parse_one_pinctrl_entry() will know that no
map was added.  

Logic is also added to pcs_parse_one_pinctrl_entry() to distinguish
between -ENOSUPP and other errors.  In the case of -ENOSUPP, num_maps
is set to 1 as it is valid for pinconf to be enabled and a given pin
group to not any pinconf properties.

[0] https://lore.kernel.org/linux-omap/20200529175544.GA3766151@x1/

Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")
Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
changes from V1 [0]:
- if pcs_parse_pinconf() returns -ENOSUPP, then set num_maps to 1 and
  proceed normally as it is valid for group to have no pinconf props
- added Fixes: tag thanks to Gustavo A. R. Silva

[0] https://lore.kernel.org/linux-omap/20200531204147.GA664833@x1/

 drivers/pinctrl/pinctrl-single.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Tony Lindgren June 9, 2020, 6:06 p.m. UTC | #1
* Drew Fustini <drew@beagleboard.org> [200608 12:52]:
> This patch causes pcs_parse_pinconf() to return -ENOTSUPP when no
> pinctrl_map is added.  The current behavior is to return 0 when
> !PCS_HAS_PINCONF or !nconfs.  Thus pcs_parse_one_pinctrl_entry()
> incorrectly assumes that a map was added and sets num_maps = 2.
...

> Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")

It would be good to get an ack/tested-by from Haojian for this.

The patch looks right to me:

Acked-by: Tony Lindgren <tony@atomide.com>
Linus Walleij June 16, 2020, 8:31 a.m. UTC | #2
On Mon, Jun 8, 2020 at 2:51 PM Drew Fustini <drew@beagleboard.org> wrote:

> This patch causes pcs_parse_pinconf() to return -ENOTSUPP when no
> pinctrl_map is added.  The current behavior is to return 0 when
> !PCS_HAS_PINCONF or !nconfs.  Thus pcs_parse_one_pinctrl_entry()
> incorrectly assumes that a map was added and sets num_maps = 2.
>
> Analysis:
> =========
> The function pcs_parse_one_pinctrl_entry() calls pcs_parse_pinconf()
> if PCS_HAS_PINCONF is enabled.  The function pcs_parse_pinconf()
> returns 0 to indicate there was no error and num_maps is then set to 2:
>
>  980 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>  981                                                 struct device_node *np,
>  982                                                 struct pinctrl_map **map,
>  983                                                 unsigned *num_maps,
>  984                                                 const char **pgnames)
>  985 {
> <snip>
> 1053         (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> 1054         (*map)->data.mux.group = np->name;
> 1055         (*map)->data.mux.function = np->name;
> 1056
> 1057         if (PCS_HAS_PINCONF && function) {
> 1058                 res = pcs_parse_pinconf(pcs, np, function, map);
> 1059                 if (res)
> 1060                         goto free_pingroups;
> 1061                 *num_maps = 2;
> 1062         } else {
> 1063                 *num_maps = 1;
> 1064         }
>
> However, pcs_parse_pinconf() will also return 0 if !PCS_HAS_PINCONF or
> !nconfs.  I believe these conditions should indicate that no map was
> added by returning -ENOTSUPP. Otherwise pcs_parse_one_pinctrl_entry()
> will set num_maps = 2 even though no maps were successfully added, as
> it does not reach "m++" on line 940:
>
>  895 static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
>  896                              struct pcs_function *func,
>  897                              struct pinctrl_map **map)
>  898
>  899 {
>  900         struct pinctrl_map *m = *map;
> <snip>
>  917         /* If pinconf isn't supported, don't parse properties in below. */
>  918         if (!PCS_HAS_PINCONF)
>  919                 return 0;
>  920
>  921         /* cacluate how much properties are supported in current node */
>  922         for (i = 0; i < ARRAY_SIZE(prop2); i++) {
>  923                 if (of_find_property(np, prop2[i].name, NULL))
>  924                         nconfs++;
>  925         }
>  926         for (i = 0; i < ARRAY_SIZE(prop4); i++) {
>  927                 if (of_find_property(np, prop4[i].name, NULL))
>  928                         nconfs++;
>  929         }
>  930         if (!nconfs)
>  919                 return 0;
>  932
>  933         func->conf = devm_kcalloc(pcs->dev,
>  934                                   nconfs, sizeof(struct pcs_conf_vals),
>  935                                   GFP_KERNEL);
>  936         if (!func->conf)
>  937                 return -ENOMEM;
>  938         func->nconfs = nconfs;
>  939         conf = &(func->conf[0]);
>  940         m++;
>
> This situtation will cause a boot failure [0] on the BeagleBone Black
> (AM3358) when am33xx_pinmux node in arch/arm/boot/dts/am33xx-l4.dtsi
> has compatible = "pinconf-single" instead of "pinctrl-single".
>
> The patch fixes this issue by returning -ENOSUPP when !PCS_HAS_PINCONF
> or !nconfs, so that pcs_parse_one_pinctrl_entry() will know that no
> map was added.
>
> Logic is also added to pcs_parse_one_pinctrl_entry() to distinguish
> between -ENOSUPP and other errors.  In the case of -ENOSUPP, num_maps
> is set to 1 as it is valid for pinconf to be enabled and a given pin
> group to not any pinconf properties.
>
> [0] https://lore.kernel.org/linux-omap/20200529175544.GA3766151@x1/
>
> Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")
> Signed-off-by: Drew Fustini <drew@beagleboard.org>

Patch applied as non-critical (for-next) fix.

If there is no hurry let's merge it this way with lots of testing
along the way.

Yours,
Linus Walleij
Drew Fustini June 16, 2020, 11:59 a.m. UTC | #3
On Tue, Jun 16, 2020 at 10:31:54AM +0200, Linus Walleij wrote:
> On Mon, Jun 8, 2020 at 2:51 PM Drew Fustini <drew@beagleboard.org> wrote:
> 
> > This patch causes pcs_parse_pinconf() to return -ENOTSUPP when no
> > pinctrl_map is added.  The current behavior is to return 0 when
> > !PCS_HAS_PINCONF or !nconfs.  Thus pcs_parse_one_pinctrl_entry()
> > incorrectly assumes that a map was added and sets num_maps = 2.
> >
> > Analysis:
> > =========
> > The function pcs_parse_one_pinctrl_entry() calls pcs_parse_pinconf()
> > if PCS_HAS_PINCONF is enabled.  The function pcs_parse_pinconf()
> > returns 0 to indicate there was no error and num_maps is then set to 2:
> >
> >  980 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
> >  981                                                 struct device_node *np,
> >  982                                                 struct pinctrl_map **map,
> >  983                                                 unsigned *num_maps,
> >  984                                                 const char **pgnames)
> >  985 {
> > <snip>
> > 1053         (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> > 1054         (*map)->data.mux.group = np->name;
> > 1055         (*map)->data.mux.function = np->name;
> > 1056
> > 1057         if (PCS_HAS_PINCONF && function) {
> > 1058                 res = pcs_parse_pinconf(pcs, np, function, map);
> > 1059                 if (res)
> > 1060                         goto free_pingroups;
> > 1061                 *num_maps = 2;
> > 1062         } else {
> > 1063                 *num_maps = 1;
> > 1064         }
> >
> > However, pcs_parse_pinconf() will also return 0 if !PCS_HAS_PINCONF or
> > !nconfs.  I believe these conditions should indicate that no map was
> > added by returning -ENOTSUPP. Otherwise pcs_parse_one_pinctrl_entry()
> > will set num_maps = 2 even though no maps were successfully added, as
> > it does not reach "m++" on line 940:
> >
> >  895 static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
> >  896                              struct pcs_function *func,
> >  897                              struct pinctrl_map **map)
> >  898
> >  899 {
> >  900         struct pinctrl_map *m = *map;
> > <snip>
> >  917         /* If pinconf isn't supported, don't parse properties in below. */
> >  918         if (!PCS_HAS_PINCONF)
> >  919                 return 0;
> >  920
> >  921         /* cacluate how much properties are supported in current node */
> >  922         for (i = 0; i < ARRAY_SIZE(prop2); i++) {
> >  923                 if (of_find_property(np, prop2[i].name, NULL))
> >  924                         nconfs++;
> >  925         }
> >  926         for (i = 0; i < ARRAY_SIZE(prop4); i++) {
> >  927                 if (of_find_property(np, prop4[i].name, NULL))
> >  928                         nconfs++;
> >  929         }
> >  930         if (!nconfs)
> >  919                 return 0;
> >  932
> >  933         func->conf = devm_kcalloc(pcs->dev,
> >  934                                   nconfs, sizeof(struct pcs_conf_vals),
> >  935                                   GFP_KERNEL);
> >  936         if (!func->conf)
> >  937                 return -ENOMEM;
> >  938         func->nconfs = nconfs;
> >  939         conf = &(func->conf[0]);
> >  940         m++;
> >
> > This situtation will cause a boot failure [0] on the BeagleBone Black
> > (AM3358) when am33xx_pinmux node in arch/arm/boot/dts/am33xx-l4.dtsi
> > has compatible = "pinconf-single" instead of "pinctrl-single".
> >
> > The patch fixes this issue by returning -ENOSUPP when !PCS_HAS_PINCONF
> > or !nconfs, so that pcs_parse_one_pinctrl_entry() will know that no
> > map was added.
> >
> > Logic is also added to pcs_parse_one_pinctrl_entry() to distinguish
> > between -ENOSUPP and other errors.  In the case of -ENOSUPP, num_maps
> > is set to 1 as it is valid for pinconf to be enabled and a given pin
> > group to not any pinconf properties.
> >
> > [0] https://lore.kernel.org/linux-omap/20200529175544.GA3766151@x1/
> >
> > Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")
> > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> 
> Patch applied as non-critical (for-next) fix.
> 
> If there is no hurry let's merge it this way with lots of testing
> along the way.
> 
> Yours,
> Linus Walleij

Thanks, I agree more testing is a good idea.

In particular, do you have a way to followup with Haojian Zhuang within
Linaro?

Haojian added the generic pinconf support back in 2013, so it would be
good to get his review.

Also, neither Tony or I have the Hikey hardware to test.

The most important to test would be those which include a .dtsi with
"pinconf-single" compatible.  I do plan to add pinconf-single to the
AM3358 BeagleBone (which is what I am testing on), but the current
mainline users are:

arch/arm/boot/dts/hi3620.dtsi
|- arch/arm/boot/dts/hi3620-hi4511.dts

arch/arm/boot/dts/pxa3xx.dtsi
|- arch/arm/boot/dts/pxa300-raumfeld-common.dtsi
   |- arch/arm/boot/dts/pxa300-raumfeld-connector.dts
      arch/arm/boot/dts/pxa300-raumfeld-controller.dts
      arch/arm/boot/dts/pxa300-raumfeld-speaker-l.dts
      arch/arm/boot/dts/pxa300-raumfeld-speaker-m.dts
      arch/arm/boot/dts/pxa300-raumfeld-speaker-one.dts
      arch/arm/boot/dts/pxa300-raumfeld-speaker-s.dts

I am cc'ing Daniel Mack and Robert Jarzmi as they are listed as
maintainers for the pxa300 related dts files.

Those platforms using compatible = "pinctrl-single" should not be
effected by this patch:

arch/arm/boot/dts/am33xx-l4.dtsi (I have changed to pinconf for test)
arch/arm/boot/dts/da850.dtsi
arch/arm/boot/dts/dm814x.dtsi
arch/arm/boot/dts/dm816x.dtsi
arch/arm/boot/dts/hi3620.dtsi
arch/arm/boot/dts/keystone-k2g.dtsi
arch/arm/boot/dts/keystone-k2l.dtsi
arch/arm/boot/dts/omap3-gta04.dtsi


Thanks,
Drew
Drew Fustini June 26, 2020, 7:19 p.m. UTC | #4
On Tue, Jun 16, 2020 at 01:59:51PM +0200, Drew Fustini wrote:
> On Tue, Jun 16, 2020 at 10:31:54AM +0200, Linus Walleij wrote:
> > On Mon, Jun 8, 2020 at 2:51 PM Drew Fustini <drew@beagleboard.org> wrote:
> > 
> > > This patch causes pcs_parse_pinconf() to return -ENOTSUPP when no
> > > pinctrl_map is added.  The current behavior is to return 0 when
> > > !PCS_HAS_PINCONF or !nconfs.  Thus pcs_parse_one_pinctrl_entry()
> > > incorrectly assumes that a map was added and sets num_maps = 2.
> > >
> > > Analysis:
> > > =========
> > > The function pcs_parse_one_pinctrl_entry() calls pcs_parse_pinconf()
> > > if PCS_HAS_PINCONF is enabled.  The function pcs_parse_pinconf()
> > > returns 0 to indicate there was no error and num_maps is then set to 2:
> > >
> > >  980 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
> > >  981                                                 struct device_node *np,
> > >  982                                                 struct pinctrl_map **map,
> > >  983                                                 unsigned *num_maps,
> > >  984                                                 const char **pgnames)
> > >  985 {
> > > <snip>
> > > 1053         (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> > > 1054         (*map)->data.mux.group = np->name;
> > > 1055         (*map)->data.mux.function = np->name;
> > > 1056
> > > 1057         if (PCS_HAS_PINCONF && function) {
> > > 1058                 res = pcs_parse_pinconf(pcs, np, function, map);
> > > 1059                 if (res)
> > > 1060                         goto free_pingroups;
> > > 1061                 *num_maps = 2;
> > > 1062         } else {
> > > 1063                 *num_maps = 1;
> > > 1064         }
> > >
> > > However, pcs_parse_pinconf() will also return 0 if !PCS_HAS_PINCONF or
> > > !nconfs.  I believe these conditions should indicate that no map was
> > > added by returning -ENOTSUPP. Otherwise pcs_parse_one_pinctrl_entry()
> > > will set num_maps = 2 even though no maps were successfully added, as
> > > it does not reach "m++" on line 940:
> > >
> > >  895 static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
> > >  896                              struct pcs_function *func,
> > >  897                              struct pinctrl_map **map)
> > >  898
> > >  899 {
> > >  900         struct pinctrl_map *m = *map;
> > > <snip>
> > >  917         /* If pinconf isn't supported, don't parse properties in below. */
> > >  918         if (!PCS_HAS_PINCONF)
> > >  919                 return 0;
> > >  920
> > >  921         /* cacluate how much properties are supported in current node */
> > >  922         for (i = 0; i < ARRAY_SIZE(prop2); i++) {
> > >  923                 if (of_find_property(np, prop2[i].name, NULL))
> > >  924                         nconfs++;
> > >  925         }
> > >  926         for (i = 0; i < ARRAY_SIZE(prop4); i++) {
> > >  927                 if (of_find_property(np, prop4[i].name, NULL))
> > >  928                         nconfs++;
> > >  929         }
> > >  930         if (!nconfs)
> > >  919                 return 0;
> > >  932
> > >  933         func->conf = devm_kcalloc(pcs->dev,
> > >  934                                   nconfs, sizeof(struct pcs_conf_vals),
> > >  935                                   GFP_KERNEL);
> > >  936         if (!func->conf)
> > >  937                 return -ENOMEM;
> > >  938         func->nconfs = nconfs;
> > >  939         conf = &(func->conf[0]);
> > >  940         m++;
> > >
> > > This situtation will cause a boot failure [0] on the BeagleBone Black
> > > (AM3358) when am33xx_pinmux node in arch/arm/boot/dts/am33xx-l4.dtsi
> > > has compatible = "pinconf-single" instead of "pinctrl-single".
> > >
> > > The patch fixes this issue by returning -ENOSUPP when !PCS_HAS_PINCONF
> > > or !nconfs, so that pcs_parse_one_pinctrl_entry() will know that no
> > > map was added.
> > >
> > > Logic is also added to pcs_parse_one_pinctrl_entry() to distinguish
> > > between -ENOSUPP and other errors.  In the case of -ENOSUPP, num_maps
> > > is set to 1 as it is valid for pinconf to be enabled and a given pin
> > > group to not any pinconf properties.
> > >
> > > [0] https://lore.kernel.org/linux-omap/20200529175544.GA3766151@x1/
> > >
> > > Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")
> > > Signed-off-by: Drew Fustini <drew@beagleboard.org>
> > 
> > Patch applied as non-critical (for-next) fix.
> > 
> > If there is no hurry let's merge it this way with lots of testing
> > along the way.
> > 
> > Yours,
> > Linus Walleij
> 
> Thanks, I agree more testing is a good idea.
> 
> In particular, do you have a way to followup with Haojian Zhuang within
> Linaro?
> 

Linus - do you have a way to contact Haojian Zhuan?

I found them on Freenode but they have been idle since beginning of
June.  Last email I can find on the mailing list is from March.

Tony and I would both like someone familiar with Hisilicon to comment on
this patch as it is using pinctrl-single.

Thanks,
Drew
Haojian Zhuang July 5, 2020, 8:58 a.m. UTC | #5
On Mon, 8 Jun 2020 at 20:51, Drew Fustini <drew@beagleboard.org> wrote:
>
> This patch causes pcs_parse_pinconf() to return -ENOTSUPP when no
> pinctrl_map is added.  The current behavior is to return 0 when
> !PCS_HAS_PINCONF or !nconfs.  Thus pcs_parse_one_pinctrl_entry()
> incorrectly assumes that a map was added and sets num_maps = 2.
>
> Analysis:
> =========
> The function pcs_parse_one_pinctrl_entry() calls pcs_parse_pinconf()
> if PCS_HAS_PINCONF is enabled.  The function pcs_parse_pinconf()
> returns 0 to indicate there was no error and num_maps is then set to 2:
>
>  980 static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>  981                                                 struct device_node *np,
>  982                                                 struct pinctrl_map **map,
>  983                                                 unsigned *num_maps,
>  984                                                 const char **pgnames)
>  985 {
> <snip>
> 1053         (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
> 1054         (*map)->data.mux.group = np->name;
> 1055         (*map)->data.mux.function = np->name;
> 1056
> 1057         if (PCS_HAS_PINCONF && function) {
> 1058                 res = pcs_parse_pinconf(pcs, np, function, map);
> 1059                 if (res)
> 1060                         goto free_pingroups;
> 1061                 *num_maps = 2;
> 1062         } else {
> 1063                 *num_maps = 1;
> 1064         }
>
> However, pcs_parse_pinconf() will also return 0 if !PCS_HAS_PINCONF or
> !nconfs.  I believe these conditions should indicate that no map was
> added by returning -ENOTSUPP. Otherwise pcs_parse_one_pinctrl_entry()
> will set num_maps = 2 even though no maps were successfully added, as
> it does not reach "m++" on line 940:
>
>  895 static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
>  896                              struct pcs_function *func,
>  897                              struct pinctrl_map **map)
>  898
>  899 {
>  900         struct pinctrl_map *m = *map;
> <snip>
>  917         /* If pinconf isn't supported, don't parse properties in below. */
>  918         if (!PCS_HAS_PINCONF)
>  919                 return 0;
>  920
>  921         /* cacluate how much properties are supported in current node */
>  922         for (i = 0; i < ARRAY_SIZE(prop2); i++) {
>  923                 if (of_find_property(np, prop2[i].name, NULL))
>  924                         nconfs++;
>  925         }
>  926         for (i = 0; i < ARRAY_SIZE(prop4); i++) {
>  927                 if (of_find_property(np, prop4[i].name, NULL))
>  928                         nconfs++;
>  929         }
>  930         if (!nconfs)
>  919                 return 0;
>  932
>  933         func->conf = devm_kcalloc(pcs->dev,
>  934                                   nconfs, sizeof(struct pcs_conf_vals),
>  935                                   GFP_KERNEL);
>  936         if (!func->conf)
>  937                 return -ENOMEM;
>  938         func->nconfs = nconfs;
>  939         conf = &(func->conf[0]);
>  940         m++;
>
> This situtation will cause a boot failure [0] on the BeagleBone Black
> (AM3358) when am33xx_pinmux node in arch/arm/boot/dts/am33xx-l4.dtsi
> has compatible = "pinconf-single" instead of "pinctrl-single".
>
> The patch fixes this issue by returning -ENOSUPP when !PCS_HAS_PINCONF
> or !nconfs, so that pcs_parse_one_pinctrl_entry() will know that no
> map was added.
>
> Logic is also added to pcs_parse_one_pinctrl_entry() to distinguish
> between -ENOSUPP and other errors.  In the case of -ENOSUPP, num_maps
> is set to 1 as it is valid for pinconf to be enabled and a given pin
> group to not any pinconf properties.
>
> [0] https://lore.kernel.org/linux-omap/20200529175544.GA3766151@x1/
>
> Fixes: 9dddb4df90d1 ("pinctrl: single: support generic pinconf")
> Signed-off-by: Drew Fustini <drew@beagleboard.org>
> ---
> changes from V1 [0]:
> - if pcs_parse_pinconf() returns -ENOSUPP, then set num_maps to 1 and
>   proceed normally as it is valid for group to have no pinconf props
> - added Fixes: tag thanks to Gustavo A. R. Silva
>
> [0] https://lore.kernel.org/linux-omap/20200531204147.GA664833@x1/
>
>  drivers/pinctrl/pinctrl-single.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 1e0614daee9b..a9d511982780 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -916,7 +916,7 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
>
>         /* If pinconf isn't supported, don't parse properties in below. */
>         if (!PCS_HAS_PINCONF)
> -               return 0;
> +               return -ENOTSUPP;
>
>         /* cacluate how much properties are supported in current node */
>         for (i = 0; i < ARRAY_SIZE(prop2); i++) {
> @@ -928,7 +928,7 @@ static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
>                         nconfs++;
>         }
>         if (!nconfs)
> -               return 0;
> +               return -ENOTSUPP;
>
>         func->conf = devm_kcalloc(pcs->dev,
>                                   nconfs, sizeof(struct pcs_conf_vals),
> @@ -1056,9 +1056,12 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>
>         if (PCS_HAS_PINCONF && function) {
>                 res = pcs_parse_pinconf(pcs, np, function, map);
> -               if (res)
> +               if (res == 0)
> +                       *num_maps = 2;
> +               else if (res == -ENOTSUPP)
> +                       *num_maps = 1;
> +               else
>                         goto free_pingroups;
> -               *num_maps = 2;
>         } else {
>                 *num_maps = 1;
>         }
> --
> 2.25.1


Tested-by: Haojian Zhuang <haojian.zhuang@linaro.org>
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 1e0614daee9b..a9d511982780 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -916,7 +916,7 @@  static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
 
 	/* If pinconf isn't supported, don't parse properties in below. */
 	if (!PCS_HAS_PINCONF)
-		return 0;
+		return -ENOTSUPP;
 
 	/* cacluate how much properties are supported in current node */
 	for (i = 0; i < ARRAY_SIZE(prop2); i++) {
@@ -928,7 +928,7 @@  static int pcs_parse_pinconf(struct pcs_device *pcs, struct device_node *np,
 			nconfs++;
 	}
 	if (!nconfs)
-		return 0;
+		return -ENOTSUPP;
 
 	func->conf = devm_kcalloc(pcs->dev,
 				  nconfs, sizeof(struct pcs_conf_vals),
@@ -1056,9 +1056,12 @@  static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 
 	if (PCS_HAS_PINCONF && function) {
 		res = pcs_parse_pinconf(pcs, np, function, map);
-		if (res)
+		if (res == 0)
+			*num_maps = 2;
+		else if (res == -ENOTSUPP)
+			*num_maps = 1;
+		else
 			goto free_pingroups;
-		*num_maps = 2;
 	} else {
 		*num_maps = 1;
 	}