pinctrl: Enforce device links
diff mbox series

Message ID 20191212101134.42420-1-linus.walleij@linaro.org
State New
Headers show
Series
  • pinctrl: Enforce device links
Related show

Commit Message

Linus Walleij Dec. 12, 2019, 10:11 a.m. UTC
Instead of opt-in device links, enforce it across the board.
Everyone probably needs this anyway, lest runtime[_pm] suspend
order will be haphazard.

Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
As there is no progress on opting in drivers for this we can just
enforce it.

I am a bit concerned that with every pin control state change the
link reference count will just go up, but does it really matter?
---
 drivers/pinctrl/core.c                | 25 ++++++++++++++-----------
 drivers/pinctrl/pinctrl-stmfx.c       |  1 -
 drivers/pinctrl/stm32/pinctrl-stm32.c |  1 -
 include/linux/pinctrl/pinctrl.h       |  5 -----
 4 files changed, 14 insertions(+), 18 deletions(-)

Comments

Ulf Hansson Dec. 12, 2019, 10:56 a.m. UTC | #1
+ Benjamin

On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Instead of opt-in device links, enforce it across the board.
> Everyone probably needs this anyway, lest runtime[_pm] suspend
> order will be haphazard.
>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> As there is no progress on opting in drivers for this we can just
> enforce it.
>
> I am a bit concerned that with every pin control state change the
> link reference count will just go up, but does it really matter?
> ---
>  drivers/pinctrl/core.c                | 25 ++++++++++++++-----------
>  drivers/pinctrl/pinctrl-stmfx.c       |  1 -
>  drivers/pinctrl/stm32/pinctrl-stm32.c |  1 -
>  include/linux/pinctrl/pinctrl.h       |  5 -----
>  4 files changed, 14 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 2bbd8ee93507..1d2cdeebb316 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
>  }
>  EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>
> -static void pinctrl_link_add(struct pinctrl_dev *pctldev,
> -                            struct device *consumer)
> -{
> -       if (pctldev->desc->link_consumers)
> -               device_link_add(consumer, pctldev->dev,
> -                               DL_FLAG_PM_RUNTIME |
> -                               DL_FLAG_AUTOREMOVE_CONSUMER);
> -}
> -
>  /**
>   * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
>   * @p: the pinctrl handle for the device that requests configuration
> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>                 }
>
>                 /* Do not link hogs (circular dependency) */
> -               if (p != setting->pctldev->p)
> -                       pinctrl_link_add(setting->pctldev, p->dev);
> +               if (p != setting->pctldev->p) {
> +                       /*
> +                        * Create a device link to the consumer such that
> +                        * it will enforce that runtime PM suspend/resume
> +                        * is done first on consumers before we get to
> +                        * the pin controller itself. As some devices get
> +                        * their pin control state even before probe() it is
> +                        * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
> +                        */
> +                       device_link_add(p->dev,
> +                                       setting->pctldev->dev,
> +                                       DL_FLAG_PM_RUNTIME |
> +                                       DL_FLAG_AUTOREMOVE_CONSUMER);

I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
could you please explain some of the reasons behind that?

In regards to adding a new link every time you commit/select a new
state, that sounds wrong to me. Why are we doing this?

Instead, don't you want to add a link when the consumer looks up the
pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
is called?

Additionally, I didn't find any place where the link is removed. I
looks like that should happen when the consumer drops the reference
for the pinctrl cookie, no?

> +               }
>         }
>
>         p->state = state;
> diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
> index 16723797fa7c..4306b8444188 100644
> --- a/drivers/pinctrl/pinctrl-stmfx.c
> +++ b/drivers/pinctrl/pinctrl-stmfx.c
> @@ -638,7 +638,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
>         pctl->pctl_desc.pins = stmfx_pins;
>         pctl->pctl_desc.npins = ARRAY_SIZE(stmfx_pins);
>         pctl->pctl_desc.owner = THIS_MODULE;
> -       pctl->pctl_desc.link_consumers = true;
>
>         ret = devm_pinctrl_register_and_init(pctl->dev, &pctl->pctl_desc,
>                                              pctl, &pctl->pctl_dev);
> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
> index 2d5e0435af0a..ec59a58600ce 100644
> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
> @@ -1439,7 +1439,6 @@ int stm32_pctl_probe(struct platform_device *pdev)
>         pctl->pctl_desc.owner = THIS_MODULE;
>         pctl->pctl_desc.pins = pins;
>         pctl->pctl_desc.npins = pctl->npins;
> -       pctl->pctl_desc.link_consumers = true;
>         pctl->pctl_desc.confops = &stm32_pconf_ops;
>         pctl->pctl_desc.pctlops = &stm32_pctrl_ops;
>         pctl->pctl_desc.pmxops = &stm32_pmx_ops;
> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
> index 7ce23450a1cb..c6159f041f4e 100644
> --- a/include/linux/pinctrl/pinctrl.h
> +++ b/include/linux/pinctrl/pinctrl.h
> @@ -122,10 +122,6 @@ struct pinctrl_ops {
>   *     the hardware description
>   * @custom_conf_items: Information how to print @params in debugfs, must be
>   *     the same size as the @custom_params, i.e. @num_custom_params
> - * @link_consumers: If true create a device link between pinctrl and its
> - *     consumers (i.e. the devices requesting pin control states). This is
> - *     sometimes necessary to ascertain the right suspend/resume order for
> - *     example.
>   */
>  struct pinctrl_desc {
>         const char *name;
> @@ -140,7 +136,6 @@ struct pinctrl_desc {
>         const struct pinconf_generic_params *custom_params;
>         const struct pin_config_item *custom_conf_items;
>  #endif
> -       bool link_consumers;
>  };
>
>  /* External interface to pin controller */
> --
> 2.23.0
>

Kind regards
Uffe
Benjamin GAIGNARD Dec. 12, 2019, 1:19 p.m. UTC | #2
On 12/12/19 11:56 AM, Ulf Hansson wrote:
> + Benjamin
>
> On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Instead of opt-in device links, enforce it across the board.
>> Everyone probably needs this anyway, lest runtime[_pm] suspend
>> order will be haphazard.
>>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> As there is no progress on opting in drivers for this we can just
>> enforce it.
>>
>> I am a bit concerned that with every pin control state change the
>> link reference count will just go up, but does it really matter?
>> ---
>>   drivers/pinctrl/core.c                | 25 ++++++++++++++-----------
>>   drivers/pinctrl/pinctrl-stmfx.c       |  1 -
>>   drivers/pinctrl/stm32/pinctrl-stm32.c |  1 -
>>   include/linux/pinctrl/pinctrl.h       |  5 -----
>>   4 files changed, 14 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>> index 2bbd8ee93507..1d2cdeebb316 100644
>> --- a/drivers/pinctrl/core.c
>> +++ b/drivers/pinctrl/core.c
>> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
>>   }
>>   EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>>
>> -static void pinctrl_link_add(struct pinctrl_dev *pctldev,
>> -                            struct device *consumer)
>> -{
>> -       if (pctldev->desc->link_consumers)
>> -               device_link_add(consumer, pctldev->dev,
>> -                               DL_FLAG_PM_RUNTIME |
>> -                               DL_FLAG_AUTOREMOVE_CONSUMER);
>> -}
>> -
>>   /**
>>    * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
>>    * @p: the pinctrl handle for the device that requests configuration
>> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>>                  }
>>
>>                  /* Do not link hogs (circular dependency) */
>> -               if (p != setting->pctldev->p)
>> -                       pinctrl_link_add(setting->pctldev, p->dev);
>> +               if (p != setting->pctldev->p) {
>> +                       /*
>> +                        * Create a device link to the consumer such that
>> +                        * it will enforce that runtime PM suspend/resume
>> +                        * is done first on consumers before we get to
>> +                        * the pin controller itself. As some devices get
>> +                        * their pin control state even before probe() it is
>> +                        * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
>> +                        */
>> +                       device_link_add(p->dev,
>> +                                       setting->pctldev->dev,
>> +                                       DL_FLAG_PM_RUNTIME |
>> +                                       DL_FLAG_AUTOREMOVE_CONSUMER);
> I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
> could you please explain some of the reasons behind that?
>
> In regards to adding a new link every time you commit/select a new
> state, that sounds wrong to me. Why are we doing this?

If a link already exists it will not add new one so it safe for me.

>
> Instead, don't you want to add a link when the consumer looks up the
> pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
> is called?
Because it was the only place I add found where I get a the same
time pointers on the consumer and the producer devices.

>
> Additionally, I didn't find any place where the link is removed. I
> looks like that should happen when the consumer drops the reference
> for the pinctrl cookie, no?

The link is automatically removed when the consumer device is deleted

thanks to DL_FLAG_AUTOREMOVE_CONSUMER flag.

Benjamin

>
>> +               }
>>          }
>>
>>          p->state = state;
>> diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
>> index 16723797fa7c..4306b8444188 100644
>> --- a/drivers/pinctrl/pinctrl-stmfx.c
>> +++ b/drivers/pinctrl/pinctrl-stmfx.c
>> @@ -638,7 +638,6 @@ static int stmfx_pinctrl_probe(struct platform_device *pdev)
>>          pctl->pctl_desc.pins = stmfx_pins;
>>          pctl->pctl_desc.npins = ARRAY_SIZE(stmfx_pins);
>>          pctl->pctl_desc.owner = THIS_MODULE;
>> -       pctl->pctl_desc.link_consumers = true;
>>
>>          ret = devm_pinctrl_register_and_init(pctl->dev, &pctl->pctl_desc,
>>                                               pctl, &pctl->pctl_dev);
>> diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> index 2d5e0435af0a..ec59a58600ce 100644
>> --- a/drivers/pinctrl/stm32/pinctrl-stm32.c
>> +++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
>> @@ -1439,7 +1439,6 @@ int stm32_pctl_probe(struct platform_device *pdev)
>>          pctl->pctl_desc.owner = THIS_MODULE;
>>          pctl->pctl_desc.pins = pins;
>>          pctl->pctl_desc.npins = pctl->npins;
>> -       pctl->pctl_desc.link_consumers = true;
>>          pctl->pctl_desc.confops = &stm32_pconf_ops;
>>          pctl->pctl_desc.pctlops = &stm32_pctrl_ops;
>>          pctl->pctl_desc.pmxops = &stm32_pmx_ops;
>> diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
>> index 7ce23450a1cb..c6159f041f4e 100644
>> --- a/include/linux/pinctrl/pinctrl.h
>> +++ b/include/linux/pinctrl/pinctrl.h
>> @@ -122,10 +122,6 @@ struct pinctrl_ops {
>>    *     the hardware description
>>    * @custom_conf_items: Information how to print @params in debugfs, must be
>>    *     the same size as the @custom_params, i.e. @num_custom_params
>> - * @link_consumers: If true create a device link between pinctrl and its
>> - *     consumers (i.e. the devices requesting pin control states). This is
>> - *     sometimes necessary to ascertain the right suspend/resume order for
>> - *     example.
>>    */
>>   struct pinctrl_desc {
>>          const char *name;
>> @@ -140,7 +136,6 @@ struct pinctrl_desc {
>>          const struct pinconf_generic_params *custom_params;
>>          const struct pin_config_item *custom_conf_items;
>>   #endif
>> -       bool link_consumers;
>>   };
>>
>>   /* External interface to pin controller */
>> --
>> 2.23.0
>>
> Kind regards
> Uffe
Ulf Hansson Dec. 12, 2019, 1:47 p.m. UTC | #3
On Thu, 12 Dec 2019 at 14:19, Benjamin GAIGNARD
<benjamin.gaignard@st.com> wrote:
>
>
> On 12/12/19 11:56 AM, Ulf Hansson wrote:
> > + Benjamin
> >
> > On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@linaro.org> wrote:
> >> Instead of opt-in device links, enforce it across the board.
> >> Everyone probably needs this anyway, lest runtime[_pm] suspend
> >> order will be haphazard.
> >>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> >> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> >> ---
> >> As there is no progress on opting in drivers for this we can just
> >> enforce it.
> >>
> >> I am a bit concerned that with every pin control state change the
> >> link reference count will just go up, but does it really matter?
> >> ---
> >>   drivers/pinctrl/core.c                | 25 ++++++++++++++-----------
> >>   drivers/pinctrl/pinctrl-stmfx.c       |  1 -
> >>   drivers/pinctrl/stm32/pinctrl-stm32.c |  1 -
> >>   include/linux/pinctrl/pinctrl.h       |  5 -----
> >>   4 files changed, 14 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> >> index 2bbd8ee93507..1d2cdeebb316 100644
> >> --- a/drivers/pinctrl/core.c
> >> +++ b/drivers/pinctrl/core.c
> >> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
> >>   }
> >>   EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
> >>
> >> -static void pinctrl_link_add(struct pinctrl_dev *pctldev,
> >> -                            struct device *consumer)
> >> -{
> >> -       if (pctldev->desc->link_consumers)
> >> -               device_link_add(consumer, pctldev->dev,
> >> -                               DL_FLAG_PM_RUNTIME |
> >> -                               DL_FLAG_AUTOREMOVE_CONSUMER);
> >> -}
> >> -
> >>   /**
> >>    * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
> >>    * @p: the pinctrl handle for the device that requests configuration
> >> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
> >>                  }
> >>
> >>                  /* Do not link hogs (circular dependency) */
> >> -               if (p != setting->pctldev->p)
> >> -                       pinctrl_link_add(setting->pctldev, p->dev);
> >> +               if (p != setting->pctldev->p) {
> >> +                       /*
> >> +                        * Create a device link to the consumer such that
> >> +                        * it will enforce that runtime PM suspend/resume
> >> +                        * is done first on consumers before we get to
> >> +                        * the pin controller itself. As some devices get
> >> +                        * their pin control state even before probe() it is
> >> +                        * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
> >> +                        */
> >> +                       device_link_add(p->dev,
> >> +                                       setting->pctldev->dev,
> >> +                                       DL_FLAG_PM_RUNTIME |
> >> +                                       DL_FLAG_AUTOREMOVE_CONSUMER);
> > I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
> > could you please explain some of the reasons behind that?

Can you please clarify on this part as well?

> >
> > In regards to adding a new link every time you commit/select a new
> > state, that sounds wrong to me. Why are we doing this?
>
> If a link already exists it will not add new one so it safe for me.

Right, but it seems silly to walk the list of links to find out that
it already exist and then bail out.

The point is, it adds unnecessary overhead, every time there is a new
state being selected. Don't you think?

>
> >
> > Instead, don't you want to add a link when the consumer looks up the
> > pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
> > is called?
> Because it was the only place I add found where I get a the same
> time pointers on the consumer and the producer devices.

At least create_pinctrl() have the consumer device, but I am pretty
sure we can lookup the producer device from there, in some way. Linus?

>
> >
> > Additionally, I didn't find any place where the link is removed. I
> > looks like that should happen when the consumer drops the reference
> > for the pinctrl cookie, no?
>
> The link is automatically removed when the consumer device is deleted
>
> thanks to DL_FLAG_AUTOREMOVE_CONSUMER flag.

Ahh, I see. That should be fine, I guess.

[...]

Kind regards
Uffe
Benjamin GAIGNARD Dec. 12, 2019, 3:29 p.m. UTC | #4
On 12/12/19 2:47 PM, Ulf Hansson wrote:
> On Thu, 12 Dec 2019 at 14:19, Benjamin GAIGNARD
> <benjamin.gaignard@st.com> wrote:
>>
>> On 12/12/19 11:56 AM, Ulf Hansson wrote:
>>> + Benjamin
>>>
>>> On Thu, 12 Dec 2019 at 11:11, Linus Walleij <linus.walleij@linaro.org> wrote:
>>>> Instead of opt-in device links, enforce it across the board.
>>>> Everyone probably needs this anyway, lest runtime[_pm] suspend
>>>> order will be haphazard.
>>>>
>>>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>>>> ---
>>>> As there is no progress on opting in drivers for this we can just
>>>> enforce it.
>>>>
>>>> I am a bit concerned that with every pin control state change the
>>>> link reference count will just go up, but does it really matter?
>>>> ---
>>>>    drivers/pinctrl/core.c                | 25 ++++++++++++++-----------
>>>>    drivers/pinctrl/pinctrl-stmfx.c       |  1 -
>>>>    drivers/pinctrl/stm32/pinctrl-stm32.c |  1 -
>>>>    include/linux/pinctrl/pinctrl.h       |  5 -----
>>>>    4 files changed, 14 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
>>>> index 2bbd8ee93507..1d2cdeebb316 100644
>>>> --- a/drivers/pinctrl/core.c
>>>> +++ b/drivers/pinctrl/core.c
>>>> @@ -1220,15 +1220,6 @@ struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
>>>>
>>>> -static void pinctrl_link_add(struct pinctrl_dev *pctldev,
>>>> -                            struct device *consumer)
>>>> -{
>>>> -       if (pctldev->desc->link_consumers)
>>>> -               device_link_add(consumer, pctldev->dev,
>>>> -                               DL_FLAG_PM_RUNTIME |
>>>> -                               DL_FLAG_AUTOREMOVE_CONSUMER);
>>>> -}
>>>> -
>>>>    /**
>>>>     * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
>>>>     * @p: the pinctrl handle for the device that requests configuration
>>>> @@ -1276,8 +1267,20 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>>>>                   }
>>>>
>>>>                   /* Do not link hogs (circular dependency) */
>>>> -               if (p != setting->pctldev->p)
>>>> -                       pinctrl_link_add(setting->pctldev, p->dev);
>>>> +               if (p != setting->pctldev->p) {
>>>> +                       /*
>>>> +                        * Create a device link to the consumer such that
>>>> +                        * it will enforce that runtime PM suspend/resume
>>>> +                        * is done first on consumers before we get to
>>>> +                        * the pin controller itself. As some devices get
>>>> +                        * their pin control state even before probe() it is
>>>> +                        * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
>>>> +                        */
>>>> +                       device_link_add(p->dev,
>>>> +                                       setting->pctldev->dev,
>>>> +                                       DL_FLAG_PM_RUNTIME |
>>>> +                                       DL_FLAG_AUTOREMOVE_CONSUMER);
>>> I understand DL_FLAG_PM_RUNTIME is used even prior $subject patch, but
>>> could you please explain some of the reasons behind that?
> Can you please clarify on this part as well?
It is to indicate to PM runtime framework that it have to use the link
so suspend/resume between consumer and producer is well ordered.
That was the primary goal of this patch.
>
>>> In regards to adding a new link every time you commit/select a new
>>> state, that sounds wrong to me. Why are we doing this?
>> If a link already exists it will not add new one so it safe for me.
> Right, but it seems silly to walk the list of links to find out that
> it already exist and then bail out.
>
> The point is, it adds unnecessary overhead, every time there is a new
> state being selected. Don't you think?
>
>>> Instead, don't you want to add a link when the consumer looks up the
>>> pinctrl cookie/handle (devm_pinctrl_get()), thus when create_pinctrl()
>>> is called?
>> Because it was the only place I add found where I get a the same
>> time pointers on the consumer and the producer devices.
> At least create_pinctrl() have the consumer device, but I am pretty
> sure we can lookup the producer device from there, in some way. Linus?
>
>>> Additionally, I didn't find any place where the link is removed. I
>>> looks like that should happen when the consumer drops the reference
>>> for the pinctrl cookie, no?
>> The link is automatically removed when the consumer device is deleted
>>
>> thanks to DL_FLAG_AUTOREMOVE_CONSUMER flag.
> Ahh, I see. That should be fine, I guess.
>
> [...]
>
> Kind regards
> Uffe

Patch
diff mbox series

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2bbd8ee93507..1d2cdeebb316 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1220,15 +1220,6 @@  struct pinctrl_state *pinctrl_lookup_state(struct pinctrl *p,
 }
 EXPORT_SYMBOL_GPL(pinctrl_lookup_state);
 
-static void pinctrl_link_add(struct pinctrl_dev *pctldev,
-			     struct device *consumer)
-{
-	if (pctldev->desc->link_consumers)
-		device_link_add(consumer, pctldev->dev,
-				DL_FLAG_PM_RUNTIME |
-				DL_FLAG_AUTOREMOVE_CONSUMER);
-}
-
 /**
  * pinctrl_commit_state() - select/activate/program a pinctrl state to HW
  * @p: the pinctrl handle for the device that requests configuration
@@ -1276,8 +1267,20 @@  static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
 		}
 
 		/* Do not link hogs (circular dependency) */
-		if (p != setting->pctldev->p)
-			pinctrl_link_add(setting->pctldev, p->dev);
+		if (p != setting->pctldev->p) {
+			/*
+			 * Create a device link to the consumer such that
+			 * it will enforce that runtime PM suspend/resume
+			 * is done first on consumers before we get to
+			 * the pin controller itself. As some devices get
+			 * their pin control state even before probe() it is
+			 * important to use DL_FLAG_AUTOREMOVE_CONSUMER.
+			 */
+			device_link_add(p->dev,
+					setting->pctldev->dev,
+					DL_FLAG_PM_RUNTIME |
+					DL_FLAG_AUTOREMOVE_CONSUMER);
+		}
 	}
 
 	p->state = state;
diff --git a/drivers/pinctrl/pinctrl-stmfx.c b/drivers/pinctrl/pinctrl-stmfx.c
index 16723797fa7c..4306b8444188 100644
--- a/drivers/pinctrl/pinctrl-stmfx.c
+++ b/drivers/pinctrl/pinctrl-stmfx.c
@@ -638,7 +638,6 @@  static int stmfx_pinctrl_probe(struct platform_device *pdev)
 	pctl->pctl_desc.pins = stmfx_pins;
 	pctl->pctl_desc.npins = ARRAY_SIZE(stmfx_pins);
 	pctl->pctl_desc.owner = THIS_MODULE;
-	pctl->pctl_desc.link_consumers = true;
 
 	ret = devm_pinctrl_register_and_init(pctl->dev, &pctl->pctl_desc,
 					     pctl, &pctl->pctl_dev);
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index 2d5e0435af0a..ec59a58600ce 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -1439,7 +1439,6 @@  int stm32_pctl_probe(struct platform_device *pdev)
 	pctl->pctl_desc.owner = THIS_MODULE;
 	pctl->pctl_desc.pins = pins;
 	pctl->pctl_desc.npins = pctl->npins;
-	pctl->pctl_desc.link_consumers = true;
 	pctl->pctl_desc.confops = &stm32_pconf_ops;
 	pctl->pctl_desc.pctlops = &stm32_pctrl_ops;
 	pctl->pctl_desc.pmxops = &stm32_pmx_ops;
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 7ce23450a1cb..c6159f041f4e 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -122,10 +122,6 @@  struct pinctrl_ops {
  *	the hardware description
  * @custom_conf_items: Information how to print @params in debugfs, must be
  *	the same size as the @custom_params, i.e. @num_custom_params
- * @link_consumers: If true create a device link between pinctrl and its
- *	consumers (i.e. the devices requesting pin control states). This is
- *	sometimes necessary to ascertain the right suspend/resume order for
- *	example.
  */
 struct pinctrl_desc {
 	const char *name;
@@ -140,7 +136,6 @@  struct pinctrl_desc {
 	const struct pinconf_generic_params *custom_params;
 	const struct pin_config_item *custom_conf_items;
 #endif
-	bool link_consumers;
 };
 
 /* External interface to pin controller */