diff mbox series

[v2,1/2] pinctrl: renesas: Make sure the pin type is updated after setting the MUX

Message ID 20201106180110.4256-2-prabhakar.mahadev-lad.rj@bp.renesas.com
State Superseded
Delegated to: Marek Vasut
Headers show
Series pinctrl: renesas: trivial fixes and enhancements | expand

Commit Message

Prabhakar Mahadev Lad Nov. 6, 2020, 6:01 p.m. UTC
By default on startup all the pin types are configured to
PINMUX_TYPE_NONE (in sh_pfc_map_pins()), when pin is set as GPIO the
pin type is updated to PINMUX_TYPE_GPIO. But the type is not updated
when the pin is set as a function in sh_pfc_pinctrl_pin_set() or
sh_pfc_pinctrl_group_set() calls (these calls only set the MUX if
the pin type is PINMUX_TYPE_NONE ie unused).

So with the current implementation pin functionality could be overwritten
silently, for example if the same pin is added for SPI and serial.

This patch makes sure of updating pin type after every successful call to
sh_pfc_config_mux() and thus fixing from pin functionality to be
overwritten.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/pinctrl/renesas/pfc.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Marek Vasut Nov. 15, 2020, 1:17 p.m. UTC | #1
On 11/6/20 7:01 PM, Lad Prabhakar wrote:
> By default on startup all the pin types are configured to
> PINMUX_TYPE_NONE (in sh_pfc_map_pins()), when pin is set as GPIO the
> pin type is updated to PINMUX_TYPE_GPIO. But the type is not updated
> when the pin is set as a function in sh_pfc_pinctrl_pin_set() or
> sh_pfc_pinctrl_group_set() calls (these calls only set the MUX if
> the pin type is PINMUX_TYPE_NONE ie unused).
> 
> So with the current implementation pin functionality could be overwritten
> silently, for example if the same pin is added for SPI and serial.

Shouldn't the pinctrl core rather warn about such a collision and abort?
Prabhakar Mahadev Lad Nov. 15, 2020, 5:01 p.m. UTC | #2
Hi Marek,

Thank you for the review.

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: 15 November 2020 13:18
> To: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; Simon Glass <sjg@chromium.org>;
> Masahiro Yamada <yamada.masahiro@socionext.com>; u-boot@lists.denx.de
> Cc: Adam Ford <aford173@gmail.com>; Tom Rini <trini@konsulko.com>; Prabhakar
> <prabhakar.csengg@gmail.com>; Biju Das <biju.das.jz@bp.renesas.com>
> Subject: Re: [PATCH v2 1/2] pinctrl: renesas: Make sure the pin type is updated after setting the MUX
> 
> On 11/6/20 7:01 PM, Lad Prabhakar wrote:
> > By default on startup all the pin types are configured to
> > PINMUX_TYPE_NONE (in sh_pfc_map_pins()), when pin is set as GPIO the
> > pin type is updated to PINMUX_TYPE_GPIO. But the type is not updated
> > when the pin is set as a function in sh_pfc_pinctrl_pin_set() or
> > sh_pfc_pinctrl_group_set() calls (these calls only set the MUX if
> > the pin type is PINMUX_TYPE_NONE ie unused).
> >
> > So with the current implementation pin functionality could be overwritten
> > silently, for example if the same pin is added for SPI and serial.
> 
> Shouldn't the pinctrl core rather warn about such a collision and abort?
>
As mentioned in the commit message this does fix the pin functionality from being overwritten (ie it aborts). Ill post a v3 adding a warning message.

Cheers,
Prabhakar
diff mbox series

Patch

diff --git a/drivers/pinctrl/renesas/pfc.c b/drivers/pinctrl/renesas/pfc.c
index fb811a95bc..275702d13a 100644
--- a/drivers/pinctrl/renesas/pfc.c
+++ b/drivers/pinctrl/renesas/pfc.c
@@ -537,11 +537,18 @@  static int sh_pfc_pinctrl_pin_set(struct udevice *dev, unsigned pin_selector,
 	const struct sh_pfc_pin *pin = &priv->pfc.info->pins[pin_selector];
 	int idx = sh_pfc_get_pin_index(pfc, pin->pin);
 	struct sh_pfc_pin_config *cfg = &pmx->configs[idx];
+	int ret;
 
 	if (cfg->type != PINMUX_TYPE_NONE)
 		return -EBUSY;
 
-	return sh_pfc_config_mux(pfc, pin->enum_id, PINMUX_TYPE_FUNCTION);
+	ret = sh_pfc_config_mux(pfc, pin->enum_id, PINMUX_TYPE_FUNCTION);
+	if (ret)
+		return ret;
+
+	cfg->type = PINMUX_TYPE_FUNCTION;
+
+	return 0;
 }
 
 static int sh_pfc_pinctrl_group_set(struct udevice *dev, unsigned group_selector,
@@ -551,12 +558,14 @@  static int sh_pfc_pinctrl_group_set(struct udevice *dev, unsigned group_selector
 	struct sh_pfc_pinctrl *pmx = &priv->pmx;
 	struct sh_pfc *pfc = &priv->pfc;
 	const struct sh_pfc_pin_group *grp = &priv->pfc.info->groups[group_selector];
+	struct sh_pfc_pin_config *cfg;
 	unsigned int i;
 	int ret = 0;
+	int idx;
 
 	for (i = 0; i < grp->nr_pins; ++i) {
-		int idx = sh_pfc_get_pin_index(pfc, grp->pins[i]);
-		struct sh_pfc_pin_config *cfg = &pmx->configs[idx];
+		idx = sh_pfc_get_pin_index(pfc, grp->pins[i]);
+		cfg = &pmx->configs[idx];
 
 		if (cfg->type != PINMUX_TYPE_NONE) {
 			ret = -EBUSY;
@@ -568,6 +577,10 @@  static int sh_pfc_pinctrl_group_set(struct udevice *dev, unsigned group_selector
 		ret = sh_pfc_config_mux(pfc, grp->mux[i], PINMUX_TYPE_FUNCTION);
 		if (ret < 0)
 			break;
+
+		idx = sh_pfc_get_pin_index(pfc, grp->pins[i]);
+		cfg = &pmx->configs[idx];
+		cfg->type = PINMUX_TYPE_FUNCTION;
 	}
 
 done: