| Message ID | TYAP286MB0315FB4EAD83E36FA371F119BC38A@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | pinctrl: mtmips: do not log when repeating the same pinctrl request | expand |
On Tue, Jul 18, 2023 at 5:16 PM Shiji Yang <yangshiji66@outlook.com> wrote: > Sometimes when driver fails to probe a device, the kernel will retry > it later. However, this will result in duplicate requests for the > same pinctrl configuration. In this case, we should not throw error > logs. This patch adds extra check for the pin group function. Now the > pinctrl driver only prints error log when attempting to configure the > same group as different functions. > > Signed-off-by: Shiji Yang <yangshiji66@outlook.com> > --- > drivers/pinctrl/mediatek/pinctrl-mtmips.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtmips.c b/drivers/pinctrl/mediatek/pinctrl-mtmips.c > index efd77b6c5..8f5493220 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtmips.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtmips.c > @@ -125,8 +125,9 @@ static int mtmips_pmx_group_enable(struct pinctrl_dev *pctrldev, > > /* dont allow double use */ > if (p->groups[group].enabled) { > - dev_err(p->dev, "%s is already enabled\n", > - p->groups[group].name); > + if (!p->func[func]->enabled) > + dev_err(p->dev, "%s is already enabled\n", > + p->groups[group].name); Why is the driver not backing out properly and setting this .enabled back to false when probing fails for some requesting driver? Or am I getting something wrong here? Yours, Linus Walleij
Hi, Thank you for your review. On Sun, 23 Jul 2023 21:49:52 +0200 Linus Walleij wrote: >On Tue, Jul 18, 2023 at 5:16 PM Shiji Yang <yangshiji66@outlook.com> wrote: > >> Sometimes when driver fails to probe a device, the kernel will retry >> it later. However, this will result in duplicate requests for the >> same pinctrl configuration. In this case, we should not throw error >> logs. This patch adds extra check for the pin group function. Now the >> pinctrl driver only prints error log when attempting to configure the >> same group as different functions. >> >> Signed-off-by: Shiji Yang <yangshiji66@outlook.com> >> --- >> drivers/pinctrl/mediatek/pinctrl-mtmips.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtmips.c b/drivers/pinctrl/mediatek/pinctrl-mtmips.c >> index efd77b6c5..8f5493220 100644 >> --- a/drivers/pinctrl/mediatek/pinctrl-mtmips.c >> +++ b/drivers/pinctrl/mediatek/pinctrl-mtmips.c >> @@ -125,8 +125,9 @@ static int mtmips_pmx_group_enable(struct pinctrl_dev *pctrldev, >> >> /* dont allow double use */ >> if (p->groups[group].enabled) { >> - dev_err(p->dev, "%s is already enabled\n", >> - p->groups[group].name); >> + if (!p->func[func]->enabled) >> + dev_err(p->dev, "%s is already enabled\n", >> + p->groups[group].name); > >Why is the driver not backing out properly and setting this .enabled back >to false when probing fails for some requesting driver? > >Or am I getting something wrong here? > >Yours, >Linus Walleij > We use pinctrl_select_state() to request the pinctrl and set the pin function. After searching "include/linux/pinctrl/consumer.h", I don't find a relevant API to reverse this operation so we can't set .enabled back to the false state. If I'm wrong please correct me, I don't know much about the pinctrl architecture. At least I can sure pinctrl-mtmips doesn't have an implementation to unset func[]->enabled and groups[].enabled status. ref: https://elixir.bootlin.com/linux/latest/source/include/linux/pinctrl/consumer.h Regards, Shiji Yang
This patch is outdated and has been suppressed by https://lore.kernel.org/all/TYAP286MB0315FB4EAD83E36FA371F119BC38A@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/
>This patch is outdated and has been suppressed by >https://lore.kernel.org/all/TYAP286MB0315FB4EAD83E36FA371F119BC38A@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/ Correct URL: https://lore.kernel.org/all/TYAP286MB0315A9671B4BA0347E70D9E0BC00A@TYAP286MB0315.JPNP286.PROD.OUTLOOK.COM/
On Mon, Jul 24, 2023 at 3:31 AM Shiji Yang <yangshiji66@outlook.com> wrote: > We use pinctrl_select_state() to request the pinctrl and set the pin > function. After searching "include/linux/pinctrl/consumer.h", I don't > find a relevant API to reverse this operation so we can't set .enabled > back to the false state. If I'm wrong please correct me, I don't know > much about the pinctrl architecture. I don't think that has much to do with the pinctrl core. I expect the driver to set this .enabled variable to false wherever the enablement fails, note that this is inside struct mtmips_priv *p, i.e. nothing to do with the pin control core. This patch appears to paper over the real issue. Yours, Linus Walleij
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtmips.c b/drivers/pinctrl/mediatek/pinctrl-mtmips.c index efd77b6c5..8f5493220 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtmips.c +++ b/drivers/pinctrl/mediatek/pinctrl-mtmips.c @@ -125,8 +125,9 @@ static int mtmips_pmx_group_enable(struct pinctrl_dev *pctrldev, /* dont allow double use */ if (p->groups[group].enabled) { - dev_err(p->dev, "%s is already enabled\n", - p->groups[group].name); + if (!p->func[func]->enabled) + dev_err(p->dev, "%s is already enabled\n", + p->groups[group].name); return 0; }
Sometimes when driver fails to probe a device, the kernel will retry it later. However, this will result in duplicate requests for the same pinctrl configuration. In this case, we should not throw error logs. This patch adds extra check for the pin group function. Now the pinctrl driver only prints error log when attempting to configure the same group as different functions. Signed-off-by: Shiji Yang <yangshiji66@outlook.com> --- drivers/pinctrl/mediatek/pinctrl-mtmips.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)