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