Explicit ifdeffery is ugly and theoretically might be not synchronised
with the rest of functions that are assigned via pm_sleep_ptr() macro.
Replace ifdeffery by pm_sleep_ptr() macro to improve this.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/pinctrl/intel/pinctrl-intel.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 7a790c437f68..bfe891522044 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1482,7 +1482,6 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl,
static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
{
-#ifdef CONFIG_PM_SLEEP
const struct intel_pinctrl_soc_data *soc = pctrl->soc;
struct intel_community_context *communities;
struct intel_pad_context *pads;
@@ -1497,7 +1496,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
if (!communities)
return -ENOMEM;
-
for (i = 0; i < pctrl->ncommunities; i++) {
struct intel_community *community = &pctrl->communities[i];
u32 *intmask, *hostown;
@@ -1519,7 +1517,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
pctrl->context.pads = pads;
pctrl->context.communities = communities;
-#endif
return 0;
}
@@ -1649,7 +1646,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
if (irq < 0)
return irq;
- ret = intel_pinctrl_pm_init(pctrl);
+ ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
if (ret)
return ret;
--
2.43.0.rc1.1336.g36b5255a03acHi Andy,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linusw-pinctrl/devel]
[also build test WARNING on linusw-pinctrl/for-next linus/master v6.11-rc6 next-20240904]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/pinctrl-intel-Replace-ifdeffery-by-pm_sleep_ptr-macro/20240904-011041
base: https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
patch link: https://lore.kernel.org/r/20240903170752.3564538-2-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v1 1/3] pinctrl: intel: Replace ifdeffery by pm_sleep_ptr() macro
config: i386-buildonly-randconfig-002-20240904 (https://download.01.org/0day-ci/archive/20240904/202409041756.jHFGLs72-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240904/202409041756.jHFGLs72-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409041756.jHFGLs72-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/pinctrl/intel/pinctrl-intel.c: In function 'intel_pinctrl_probe':
>> drivers/pinctrl/intel/pinctrl-intel.c:1600:51: warning: the address of 'intel_pinctrl_pm_init' will always evaluate as 'true' [-Waddress]
1600 | ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
| ^
vim +1600 drivers/pinctrl/intel/pinctrl-intel.c
1503
1504 int intel_pinctrl_probe(struct platform_device *pdev,
1505 const struct intel_pinctrl_soc_data *soc_data)
1506 {
1507 struct device *dev = &pdev->dev;
1508 struct intel_pinctrl *pctrl;
1509 int i, ret, irq;
1510
1511 pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
1512 if (!pctrl)
1513 return -ENOMEM;
1514
1515 pctrl->dev = dev;
1516 pctrl->soc = soc_data;
1517 raw_spin_lock_init(&pctrl->lock);
1518
1519 /*
1520 * Make a copy of the communities which we can use to hold pointers
1521 * to the registers.
1522 */
1523 pctrl->ncommunities = pctrl->soc->ncommunities;
1524 pctrl->communities = devm_kcalloc(dev, pctrl->ncommunities,
1525 sizeof(*pctrl->communities), GFP_KERNEL);
1526 if (!pctrl->communities)
1527 return -ENOMEM;
1528
1529 for (i = 0; i < pctrl->ncommunities; i++) {
1530 struct intel_community *community = &pctrl->communities[i];
1531 void __iomem *regs;
1532 u32 offset;
1533 u32 value;
1534
1535 *community = pctrl->soc->communities[i];
1536
1537 regs = devm_platform_ioremap_resource(pdev, community->barno);
1538 if (IS_ERR(regs))
1539 return PTR_ERR(regs);
1540
1541 /*
1542 * Determine community features based on the revision.
1543 * A value of all ones means the device is not present.
1544 */
1545 value = readl(regs + REVID);
1546 if (value == ~0u)
1547 return -ENODEV;
1548 if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) {
1549 community->features |= PINCTRL_FEATURE_DEBOUNCE;
1550 community->features |= PINCTRL_FEATURE_1K_PD;
1551 }
1552
1553 /* Determine community features based on the capabilities */
1554 offset = CAPLIST;
1555 do {
1556 value = readl(regs + offset);
1557 switch ((value & CAPLIST_ID_MASK) >> CAPLIST_ID_SHIFT) {
1558 case CAPLIST_ID_GPIO_HW_INFO:
1559 community->features |= PINCTRL_FEATURE_GPIO_HW_INFO;
1560 break;
1561 case CAPLIST_ID_PWM:
1562 community->features |= PINCTRL_FEATURE_PWM;
1563 break;
1564 case CAPLIST_ID_BLINK:
1565 community->features |= PINCTRL_FEATURE_BLINK;
1566 break;
1567 case CAPLIST_ID_EXP:
1568 community->features |= PINCTRL_FEATURE_EXP;
1569 break;
1570 default:
1571 break;
1572 }
1573 offset = (value & CAPLIST_NEXT_MASK) >> CAPLIST_NEXT_SHIFT;
1574 } while (offset);
1575
1576 dev_dbg(dev, "Community%d features: %#08x\n", i, community->features);
1577
1578 /* Read offset of the pad configuration registers */
1579 offset = readl(regs + PADBAR);
1580
1581 community->regs = regs;
1582 community->pad_regs = regs + offset;
1583
1584 if (community->gpps)
1585 ret = intel_pinctrl_add_padgroups_by_gpps(pctrl, community);
1586 else
1587 ret = intel_pinctrl_add_padgroups_by_size(pctrl, community);
1588 if (ret)
1589 return ret;
1590
1591 ret = intel_pinctrl_probe_pwm(pctrl, community);
1592 if (ret)
1593 return ret;
1594 }
1595
1596 irq = platform_get_irq(pdev, 0);
1597 if (irq < 0)
1598 return irq;
1599
> 1600 ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
1601 if (ret)
1602 return ret;
1603
1604 pctrl->pctldesc = intel_pinctrl_desc;
1605 pctrl->pctldesc.name = dev_name(dev);
1606 pctrl->pctldesc.pins = pctrl->soc->pins;
1607 pctrl->pctldesc.npins = pctrl->soc->npins;
1608
1609 pctrl->pctldev = devm_pinctrl_register(dev, &pctrl->pctldesc, pctrl);
1610 if (IS_ERR(pctrl->pctldev)) {
1611 dev_err(dev, "failed to register pinctrl driver\n");
1612 return PTR_ERR(pctrl->pctldev);
1613 }
1614
1615 ret = intel_gpio_probe(pctrl, irq);
1616 if (ret)
1617 return ret;
1618
1619 platform_set_drvdata(pdev, pctrl);
1620
1621 return 0;
1622 }
1623 EXPORT_SYMBOL_NS_GPL(intel_pinctrl_probe, PINCTRL_INTEL);
1624
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote:
> Explicit ifdeffery is ugly and theoretically might be not synchronised
> with the rest of functions that are assigned via pm_sleep_ptr() macro.
> Replace ifdeffery by pm_sleep_ptr() macro to improve this.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/pinctrl/intel/pinctrl-intel.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 7a790c437f68..bfe891522044 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -1482,7 +1482,6 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl,
>
> static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> {
> -#ifdef CONFIG_PM_SLEEP
> const struct intel_pinctrl_soc_data *soc = pctrl->soc;
> struct intel_community_context *communities;
> struct intel_pad_context *pads;
> @@ -1497,7 +1496,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> if (!communities)
> return -ENOMEM;
>
> -
> for (i = 0; i < pctrl->ncommunities; i++) {
> struct intel_community *community = &pctrl->communities[i];
> u32 *intmask, *hostown;
> @@ -1519,7 +1517,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
>
> pctrl->context.pads = pads;
> pctrl->context.communities = communities;
> -#endif
Can't we make this a stub when !PM_SLEEP?
#ifdef CONFIG_PM_SLEEP
static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
{
...
}
#else
static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
{
return 0;
}
#endif
>
> return 0;
> }
> @@ -1649,7 +1646,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
> if (irq < 0)
> return irq;
>
> - ret = intel_pinctrl_pm_init(pctrl);
> + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
Then this still looks like a function call and not like some weird
conditional.
> if (ret)
> return ret;
>
> --
> 2.43.0.rc1.1336.g36b5255a03ac
On Wed, Sep 4, 2024 at 8:05 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote:
> > Explicit ifdeffery is ugly and theoretically might be not synchronised
> > with the rest of functions that are assigned via pm_sleep_ptr() macro.
> > Replace ifdeffery by pm_sleep_ptr() macro to improve this.
...
> Can't we make this a stub when !PM_SLEEP?
>
> #ifdef CONFIG_PM_SLEEP
> static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> {
> ...
> }
> #else
> static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> {
> return 0;
> }
> #endif
There is no benefit. It's actually the opposite, i.e. it expands more ifdeffery.
...
> > - ret = intel_pinctrl_pm_init(pctrl);
> > + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
>
> Then this still looks like a function call and not like some weird
> conditional.
I understand that, but the point is to make all PM callbacks use the
same approach against kernel configuration. Current state of affairs
is simple inconsistency, but it might, however quite unlikely, lead to
desynchronization between two pm_sleep_ptr() and ifdeffery approaches.
Approach that I have before this one (and I kinda agree that ternary
here looks a bit weird) is to typedef the function and do something
like
pinctrl-intel.h:
typedef alloc_fn;
static inline int ctx_alloc(pctrl, alloc_fn)
{
if (alloc_fn)
return alloc_fn(pctrl);
return 0;
}
pinctrl-intel.c:
ret = ctx_alloc(pctrl, pm_sleep_ptr(_pm_init))
if (ret)
return ret;
Altogether it will be ~20+ LoCs in addition to the current codebase.
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 4, 2024 at 10:47 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Sep 4, 2024 at 8:05 AM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote:
> > > Explicit ifdeffery is ugly and theoretically might be not synchronised
> > > with the rest of functions that are assigned via pm_sleep_ptr() macro.
> > > Replace ifdeffery by pm_sleep_ptr() macro to improve this.
...
> > Can't we make this a stub when !PM_SLEEP?
> >
> > #ifdef CONFIG_PM_SLEEP
> > static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> > {
> > ...
> > }
> > #else
> > static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> > {
> > return 0;
> > }
> > #endif
>
> There is no benefit. It's actually the opposite, i.e. it expands more ifdeffery.
>
> ...
>
> > > - ret = intel_pinctrl_pm_init(pctrl);
> > > + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
> >
> > Then this still looks like a function call and not like some weird
> > conditional.
>
> I understand that, but the point is to make all PM callbacks use the
> same approach against kernel configuration. Current state of affairs
> is simple inconsistency, but it might, however quite unlikely, lead to
> desynchronization between two pm_sleep_ptr() and ifdeffery approaches.
>
> Approach that I have before this one (and I kinda agree that ternary
> here looks a bit weird) is to typedef the function and do something
> like
>
> pinctrl-intel.h:
> typedef alloc_fn;
Actually typedef is not needed as it may be embedded in the below
inline as it's used only once.
> static inline int ctx_alloc(pctrl, alloc_fn)
> {
> if (alloc_fn)
> return alloc_fn(pctrl);
>
> return 0;
> }
>
> pinctrl-intel.c:
>
> ret = ctx_alloc(pctrl, pm_sleep_ptr(_pm_init))
> if (ret)
> return ret;
>
> Altogether it will be ~20+ LoCs in addition to the current codebase.
--
With Best Regards,
Andy Shevchenko
On Wed, Sep 04, 2024 at 10:48:42AM +0300, Andy Shevchenko wrote:
> On Wed, Sep 4, 2024 at 10:47 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Wed, Sep 4, 2024 at 8:05 AM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Tue, Sep 03, 2024 at 08:04:49PM +0300, Andy Shevchenko wrote:
> > > > Explicit ifdeffery is ugly and theoretically might be not synchronised
> > > > with the rest of functions that are assigned via pm_sleep_ptr() macro.
> > > > Replace ifdeffery by pm_sleep_ptr() macro to improve this.
>
> ...
>
> > > Can't we make this a stub when !PM_SLEEP?
> > >
> > > #ifdef CONFIG_PM_SLEEP
> > > static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> > > {
> > > ...
> > > }
> > > #else
> > > static inline int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
> > > {
> > > return 0;
> > > }
> > > #endif
> >
> > There is no benefit. It's actually the opposite, i.e. it expands more ifdeffery.
> >
> > ...
> >
> > > > - ret = intel_pinctrl_pm_init(pctrl);
> > > > + ret = pm_sleep_ptr(intel_pinctrl_pm_init) ? intel_pinctrl_pm_init(pctrl) : 0;
> > >
> > > Then this still looks like a function call and not like some weird
> > > conditional.
> >
> > I understand that, but the point is to make all PM callbacks use the
> > same approach against kernel configuration. Current state of affairs
> > is simple inconsistency, but it might, however quite unlikely, lead to
> > desynchronization between two pm_sleep_ptr() and ifdeffery approaches.
> >
> > Approach that I have before this one (and I kinda agree that ternary
> > here looks a bit weird) is to typedef the function and do something
> > like
> >
> > pinctrl-intel.h:
>
> > typedef alloc_fn;
>
> Actually typedef is not needed as it may be embedded in the below
> inline as it's used only once.
>
> > static inline int ctx_alloc(pctrl, alloc_fn)
> > {
> > if (alloc_fn)
> > return alloc_fn(pctrl);
> >
> > return 0;
> > }
> >
> > pinctrl-intel.c:
> >
> > ret = ctx_alloc(pctrl, pm_sleep_ptr(_pm_init))
> > if (ret)
> > return ret;
I don't think this makes it any better :( We want the driver to be
readable for anyone, not just for you.
I prefer the stub and ifdeffery.
© 2016 - 2025 Red Hat, Inc.