Re: [PATCH 02/14] pinctrl: pinctrl-single: move suspend/resume to suspend_noirq/resume_noirq

[Thomas Richard]

On 1/15/24 21:02, Andy Shevchenko wrote:
> On Mon, Jan 15, 2024 at 6:16 PM Thomas Richard
> <thomas.richard@xxxxxxxxxxx> wrote:
>>
>> The goal is to extend the active period of pinctrl.
>> Some devices may need active pinctrl after suspend and/or before resume.
>> So move suspend/resume to suspend_noirq/resume_noirq to have active
>> pinctrl until suspend_noirq (included), and from resume_noirq
>> (included).
> 
> ->...callback...()
> (Same comment I have given for the first patch)

fixed

> 
> ...
> 
>>         struct pcs_device *pcs;
>>
>> -       pcs = platform_get_drvdata(pdev);
>> +       pcs = dev_get_drvdata(dev);
>>         if (!pcs)
>>                 return -EINVAL;
> 
> Drop dead code.
> This should be simple one line after your change.
> 
>        struct pcs_device *pcs = dev_get_drvdata(dev);
> 

dead code dropped

> ...
> 
>>         struct pcs_device *pcs;
>>
>> -       pcs = platform_get_drvdata(pdev);
>> +       pcs = dev_get_drvdata(dev);
>>         if (!pcs)
>>                 return -EINVAL;
> 
> Ditto.
> 
> ...

dead code dropped

> 
>> +static const struct dev_pm_ops pinctrl_single_pm_ops = {
>> +       SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pinctrl_single_suspend_noirq,
>> +                                     pinctrl_single_resume_noirq)
>> +};
> 
> Use proper / modern macro.

fixed, use DEFINE_NOIRQ_DEV_PM_OPS now

> 
> ...
> 
>>  #endif
> 
> Why ifdeferry is needed (esp. taking into account pm_ptr() use below)?

We may have an "unused variable" warning for pinctrl_single_pm_ops if
CONFIG_PM is undefined (due to pm_ptr).

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com